test: use onTestFinished for cleanup - #8025
Conversation
github-actions
Bot
added
package: react-router
package: router-core
package: router-plugin
package: router-generator
package: solid-router
package: react-start-client
package: solid-start-client
package: router-devtools-core
package: start-plugin-core
package: start-server-core
package: start-client-core
package: router-ssr-query-core
package: vue-router
package: vue-start-client
labels
|
View your CI Pipeline Execution ↗ for commit bc1e0fd
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version PreviewNo changeset entries found. Merging this PR will not cause a version bump for any packages. |
📝 WalkthroughWalkthroughThe pull request updates test cleanup across React, Solid, Vue, router-core, and tooling packages. Shared hooks, cleanup arrays, and manual teardown move to Vitest ChangesPer-test cleanup migration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bundle Size BenchmarksThis pull request does not affect bundle size in any measured scenario. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/react-router/tests/preloaded-mount-resolution.test.tsx (1)
30-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the
anycast with a typed global.
globalThis as anydisables type checking forIS_REACT_ACT_ENVIRONMENT. Define an intersection type or a global declaration for this property and use that typed value indisableActEnvironment.As per coding guidelines, TypeScript changes must use strict mode with extensive type safety.
Proposed type-safe change
+type ReactActGlobal = typeof globalThis & { + IS_REACT_ACT_ENVIRONMENT?: boolean +} + const disableActEnvironment = () => { - const prevActEnv = (globalThis as any).IS_REACT_ACT_ENVIRONMENT - ;(globalThis as any).IS_REACT_ACT_ENVIRONMENT = false + const actGlobal = globalThis as ReactActGlobal + const prevActEnv = actGlobal.IS_REACT_ACT_ENVIRONMENT + actGlobal.IS_REACT_ACT_ENVIRONMENT = false onTestFinished(() => { - ;(globalThis as any).IS_REACT_ACT_ENVIRONMENT = prevActEnv + actGlobal.IS_REACT_ACT_ENVIRONMENT = prevActEnv }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-router/tests/preloaded-mount-resolution.test.tsx` around lines 30 - 35, Update disableActEnvironment to access IS_REACT_ACT_ENVIRONMENT through a typed global declaration or intersection type instead of casting globalThis to any. Preserve the existing save, disable, and onTestFinished restoration behavior while ensuring the property is fully type-checked under strict TypeScript settings.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/router-devtools-core/tests/cache-replacement.test.ts`:
- Around line 27-33: Move the IntersectionObserver cleanup registration out of
the shared beforeEach in observe(...) within
packages/start-client-core/tests/hydration-visible.test.ts#L55-L57 and into each
test body so onTestFinished runs only during tests. The beforeAll hook in
packages/router-devtools-core/tests/cache-replacement.test.ts#L19-L27 requires
no change.
In `@packages/solid-router/tests/same-route-pending-blank.test.tsx`:
- Around line 39-42: Update the cleanup block in the test teardown so
vi.useRealTimers() runs even when cleanup() throws: wrap cleanup() in an inner
try/finally and place timer restoration in that finally, preserving the existing
outer teardown behavior.
---
Nitpick comments:
In `@packages/react-router/tests/preloaded-mount-resolution.test.tsx`:
- Around line 30-35: Update disableActEnvironment to access
IS_REACT_ACT_ENVIRONMENT through a typed global declaration or intersection type
instead of casting globalThis to any. Preserve the existing save, disable, and
onTestFinished restoration behavior while ensuring the property is fully
type-checked under strict TypeScript settings.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c876ebcb-7a84-4473-9f24-3e4aff31e0f2
📒 Files selected for processing (91)
packages/react-router/tests/ClientOnly.test.tsxpackages/react-router/tests/Matches.test.tsxpackages/react-router/tests/Scripts.test.tsxpackages/react-router/tests/component-preload-retry-pending-min.test.tsxpackages/react-router/tests/component-preload-retry.test.tsxpackages/react-router/tests/errorComponent.test.tsxpackages/react-router/tests/hydration-capped-boundary-pending.test.tsxpackages/react-router/tests/hydration-terminal-lane.test.tsxpackages/react-router/tests/issue-7635-error-head-after-navigation.test.tsxpackages/react-router/tests/issue-7638-invalidate-transition-error.test.tsxpackages/react-router/tests/link.test.tsxpackages/react-router/tests/on-rendered-same-href-state.test.tsxpackages/react-router/tests/preloaded-mount-resolution.test.tsxpackages/react-router/tests/public-presentation-lane-contract.test.tsxpackages/react-router/tests/react-render-owner-contract.test.tsxpackages/react-router/tests/redirect.test.tsxpackages/react-router/tests/renderRouterToStream.test.tsxpackages/react-router/tests/root-pending-min.test.tsxpackages/react-router/tests/router-client-stream-cleanup.test.tsxpackages/react-router/tests/router.test.tsxpackages/react-router/tests/transitioner-listener-errors.test.tsxpackages/react-router/tests/transitioner-remount.test.tsxpackages/react-router/tests/transitioner-render-ack.test.tsxpackages/react-router/tests/useMatch.test.tsxpackages/react-router/tests/useNavigate.test.tsxpackages/react-start-client/src/tests/Hydrate.test.tsxpackages/react-start-client/src/tests/hydrateStart.test.tspackages/router-core/tests/background-assets-stale.test.tspackages/router-core/tests/blocked-navigation-current-load.test.tspackages/router-core/tests/boundary-component-chunk.test.tspackages/router-core/tests/build-location.test.tspackages/router-core/tests/client-lane-adversarial.test.tspackages/router-core/tests/error-boundary-cache-generation.test.tspackages/router-core/tests/hmr-refresh-lifecycle.test.tspackages/router-core/tests/hydration-asset-context-order.test.tspackages/router-core/tests/issue-6221-head-waits-for-loader.test.tspackages/router-core/tests/masked-location-state-commit.test.tspackages/router-core/tests/preload-adoption.test.tspackages/router-core/tests/preload-beforeload-reuse.test.tspackages/router-core/tests/preload-public-cache-behavior.test.tspackages/router-core/tests/public-client-loading-contract.test.tspackages/router-core/tests/public-preload-lane-contract.test.tspackages/router-core/tests/same-destination-navigation-join.test.tspackages/router-core/tests/scroll-restoration.test.tspackages/router-core/tests/searchParams.test.tspackages/router-core/tests/server-async-headers-decorative-hang.test.tspackages/router-core/tests/ssr-server-cleanup.test.tspackages/router-core/tests/stay-match-abort.test.tspackages/router-core/tests/transformStreamWithRouter.test.tspackages/router-core/tests/utils.test.tspackages/router-devtools-core/tests/cache-replacement.test.tspackages/router-generator/tests/generator.test.tspackages/router-generator/tests/utils.test.tspackages/router-generator/tests/validate-route-params.test.tspackages/router-plugin/tests/router-plugin-context.test.tspackages/router-ssr-query-core/tests/index.test.tspackages/solid-router/tests/Scripts.test.tsxpackages/solid-router/tests/Transitioner.test.tsxpackages/solid-router/tests/component-preload-retry.test.tsxpackages/solid-router/tests/createLazyRoute.test.tsxpackages/solid-router/tests/errorComponent.test.tsxpackages/solid-router/tests/issue-7986-retained-pending.test.tsxpackages/solid-router/tests/link.test.tsxpackages/solid-router/tests/loaders.test.tsxpackages/solid-router/tests/pending-fallback-promise-replacement.test.tsxpackages/solid-router/tests/redirect.test.tsxpackages/solid-router/tests/renderRouterToStream.test.tsxpackages/solid-router/tests/router-client-stream-cleanup.test.tsxpackages/solid-router/tests/router.test.tsxpackages/solid-router/tests/same-route-pending-blank.test.tsxpackages/solid-router/tests/server/Transitioner.test.tsxpackages/solid-router/tests/transitioner-remount.test.tsxpackages/solid-router/tests/useMatch.test.tsxpackages/solid-start-client/src/tests/hydrateStart.test.tspackages/start-client-core/tests/hydration-visible.test.tspackages/start-plugin-core/tests/rsbuild/post-build.test.tspackages/start-plugin-core/tests/utils.test.tspackages/start-server-core/tests/early-hints.test.tspackages/vue-router/tests/Scripts.test.tsxpackages/vue-router/tests/errorComponent.test.tsxpackages/vue-router/tests/hydration-capped-boundary-pending.test.tsxpackages/vue-router/tests/issue-7986-retained-pending.test.tsxpackages/vue-router/tests/link.test.tsxpackages/vue-router/tests/pending-fallback-promise-replacement.test.tsxpackages/vue-router/tests/renderRouterToStream.test.tsxpackages/vue-router/tests/router.test.tsxpackages/vue-router/tests/transitioner-idle-after-render.test.tsxpackages/vue-router/tests/transitioner-remount-rendered.test.tsxpackages/vue-router/tests/transitioner-remount.test.tsxpackages/vue-router/tests/useMatch.test.tsxpackages/vue-start-client/src/hydrateStart.test.ts
| onTestFinished(() => { | ||
| document.body.innerHTML = '' | ||
| try { | ||
| window.localStorage.clear() | ||
| } catch {} | ||
| vi.useRealTimers() | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 5 'before(All|Each)|onTestFinished' \
packages/router-devtools-core/tests/cache-replacement.test.ts \
packages/start-client-core/tests/hydration-visible.test.ts
fd -HI '^package\.json$' . -E node_modules -x \
rg -n '"vitest"' {}Repository: TanStack/router
Length of output: 7708
🌐 Web query:
Vitest onTestFinished API call outside test body beforeAll beforeEach throws
💡 Result:
Yes, calling the onTestFinished API outside of a test body, including within beforeAll or beforeEach hooks, will throw an error [1][2][3]. Vitest explicitly designs onTestFinished to be used during the execution of a test to clean up state created specifically within that test's body [1][4][2]. When running hooks like beforeAll or beforeEach, Vitest specifically disables the onTestFinished (and onTestFailed) functions to prevent them from being invoked in an invalid scope, resulting in the error: Cannot call "onTestFinished" inside a test hook [5][3]. To perform cleanup within hooks, you should use the standard lifecycle patterns provided by Vitest: 1. For beforeEach: You can return a cleanup function directly from your beforeEach hook [6][7]. Vitest will automatically execute this function after each test [6]. 2. For beforeAll: Similarly, you can return a cleanup function from a beforeAll hook, which Vitest will execute after all tests in the suite have completed [6]. If you need to register cleanup logic dynamically based on resources created inside a test, keep using onTestFinished inside the test body itself [1][2][7]. If you are working with concurrent tests, ensure you use the onTestFinished method provided directly by the test context [8][1][2].
Citations:
- 1: https://github.com/vitest-dev/vitest/blob/v4.1.10/docs/api/hooks.md
- 2: https://vitest.dev/api/hooks.html
- 3: https://github.com/vitest-dev/vitest/blob/6fdb2ba6/packages/runner/src/run.ts
- 4: https://github.com/vitest-dev/vitest/blob/fda52af9c3974d5fd16161e1ed502dd47fd38ceb/packages/runner/src/hooks.ts
- 5: https://errors.standardbeagle.com/vitest-dev/vitest/cannot-call-ontestfinished-inside-a-test-hook/
- 6: https://github.com/vitest-dev/vitest/blob/206e8cff/docs/guide/lifecycle.md
- 7: https://vitest.dev/guide/learn/setup-teardown
- 8: https://vitest.dev/guide/test-context
🌐 Web query:
Vitest beforeAll hook can call onTestFinished inside it
💡 Result:
No, you cannot call onTestFinished inside a beforeAll hook [1][2]. In Vitest, onTestFinished is designed to be used during the execution of a test (or within beforeEach/afterEach hooks that run in the context of a test) to register cleanup logic that runs after that specific test completes [3][4][5]. The beforeAll hook, by contrast, runs once before the entire suite starts [3][5]. Because it operates outside the context of any individual test, Vitest explicitly disables and throws an error if you attempt to call onTestFinished within beforeAll (or other suite-level hooks) [1][2]. If you need to perform cleanup for the entire suite, you should use the afterAll hook instead [3][5].
Citations:
- 1: https://github.com/vitest-dev/vitest/blob/9423dc08/packages/runner/src/run.ts
- 2: https://errors.standardbeagle.com/vitest-dev/vitest/cannot-call-ontestfinished-inside-a-test-hook/
- 3: https://vitest.dev/api/hooks.html
- 4: https://github.com/vitest-dev/vitest/blob/9423dc08/packages/runner/src/hooks.ts
- 5: https://vitest.dev/guide/lifecycle
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- cache-replacement.test.ts relevant section ---'
sed -n '1,100p' packages/router-devtools-core/tests/cache-replacement.test.ts | cat -n
printf '%s\n' '--- hydration-visible.test.ts relevant section ---'
sed -n '38,110p' packages/start-client-core/tests/hydration-visible.test.ts | cat -nRepository: TanStack/router
Length of output: 6291
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
node - <<'JS'
const vitestDocsLink = 'https://vitest.dev/api/hooks.html'
const vitestErrorsLink = 'https://errors.standardbeagle.com/vitest-dev/vitest/cannot-call-ontestfinished-inside-a-test-hook/'
const packages = {
'packages/router-devtools-core/tests/cache-replacement.test.ts': { onTestFinishedOutsideTestBody: true, vitestVersionFromRootPackageJson: require('./package.json').devDependencies.vitest },
'packages/router-devtools-core/tests/cache-replacement.test.ts': { onTestFinishedInsideBeforeAllComment: false },
'packages/start-client-core/tests/hydration-visible.test.ts': { onTestFinishedOutsideTestBodyInBeforeEach: true, vitestVersionFromStartPackageJson: require('./packages/start-client-core/package.json').devDependencies.vitest },
}
Object.entries(packages).forEach(([file, info]) => {
console.log(file, JSON.stringify(info))
})
console.log(vitestDocsLink)
console.log(vitestErrorsLink)
JSRepository: TanStack/router
Length of output: 505
Register onTestFinished only from test bodies.
Vitest throws when onTestFinished runs outside a test body, including from suite-level or shared setup hooks. This can fail affected tests before assertions run.
packages/router-devtools-core/tests/cache-replacement.test.ts#L19-L27: ThebeforeAllwarmup hook currently does not registeronTestFinished; no change is needed here.packages/start-client-core/tests/hydration-visible.test.ts#L49-L69: Move theIntersectionObservercleanup registration into each test body, including viaobserve(...)which callsonTestFinished, instead of from the sharedbeforeEach.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 27-27: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation
(dom-content-modification)
📍 Affects 2 files
packages/router-devtools-core/tests/cache-replacement.test.ts#L27-L33(this comment)packages/start-client-core/tests/hydration-visible.test.ts#L55-L57
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/router-devtools-core/tests/cache-replacement.test.ts` around lines
27 - 33, Move the IntersectionObserver cleanup registration out of the shared
beforeEach in observe(...) within
packages/start-client-core/tests/hydration-visible.test.ts#L55-L57 and into each
test body so onTestFinished runs only during tests. The beforeAll hook in
packages/router-devtools-core/tests/cache-replacement.test.ts#L19-L27 requires
no change.
| } finally { | ||
| cleanup() | ||
| vi.useRealTimers() | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Restore real timers when DOM cleanup fails.
If cleanup() throws, execution skips vi.useRealTimers(). A failed unmount can leave fake timers enabled for later tests and cause cascading failures. Put vi.useRealTimers() in an inner finally.
Proposed fix
} finally {
- cleanup()
- vi.useRealTimers()
+ try {
+ cleanup()
+ } finally {
+ vi.useRealTimers()
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } finally { | |
| cleanup() | |
| vi.useRealTimers() | |
| } | |
| } finally { | |
| try { | |
| cleanup() | |
| } finally { | |
| vi.useRealTimers() | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/solid-router/tests/same-route-pending-blank.test.tsx` around lines
39 - 42, Update the cleanup block in the test teardown so vi.useRealTimers()
runs even when cleanup() throws: wrap cleanup() in an inner try/finally and
place timer restoration in that finally, preserving the existing outer teardown
behavior.
Merging this PR will degrade performance by 30.59%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | mem server error-paths not-found (vue) |
329.9 KB | 2,061.9 KB | -84% |
| ❌ | Memory | mem server request-churn (react) |
488.2 KB | 833.3 KB | -41.41% |
| ❌ | Memory | mem server error-paths redirect (solid) |
288.7 KB | 345.4 KB | -16.44% |
| ⚡ | Memory | mem server error-paths not-found (solid) |
661.7 KB | 411.8 KB | +60.68% |
| ⚡ | Memory | mem client unique-location-churn (solid) |
436.9 KB | 341.4 KB | +27.99% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing test/use-on-test-finished-cleanup (bc1e0fd) with main (af8dcb8)
1 participant
Summary
try/finallyblocks and shared teardown registries withonTestFinishedTesting
pnpm nx affected --target=test:unitpnpm nx affected --target=test:typespnpm nx affected --target=test:eslintSummary by CodeRabbit