fix(vitest-angular): reinitialize testbed per setup call#2244
Open
benpsnyder wants to merge 6 commits intoanalogjs:alphafrom
Open
fix(vitest-angular): reinitialize testbed per setup call#2244benpsnyder wants to merge 6 commits intoanalogjs:alphafrom
benpsnyder wants to merge 6 commits intoanalogjs:alphafrom
Conversation
✅ Deploy Preview for analog-blog ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for analog-app ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
github-actions
Bot
added
the
scope:vitest-angular
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughA global setup symbol was made specific (Symbol.for('@analogjs/vitest-angular/testbed-setup')). setupTestBed now always obtains a TestBed instance, computes Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Open
2 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/docs-app/docs/features/testing/vitest.md (1)
158-159: Documentation accurately reflects the reconfiguration behavior.The explanation correctly describes how subsequent
setupTestBed()calls reset and replace the Angular test environment. This directly addresses the bug fix from issue#2222where later calls were being ignored. The placement after the configuration options and the clear statement about preventing leakage across test projects is helpful.Optional: Consider adding a concrete example reference
To help users visualize the multi-setup-file scenario, you could add a brief inline example or reference:
Each Vitest setup file can call `setupTestBed()` with its own configuration. If multiple setup files run in the same Vitest process, the latest call replaces the active Angular test environment so project-specific providers and options do not leak across test projects. +For example, if `setup-a.ts` and `setup-b.ts` both call `setupTestBed()` within the same Vitest worker, the configuration from `setup-b.ts` will be active.However, the current concise explanation is perfectly adequate for most users.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/docs-app/docs/features/testing/vitest.md` around lines 158 - 159, The documentation correctly describes that subsequent calls to setupTestBed() replace the Angular test environment, but to make this clearer add a short concrete example or reference showing two Vitest setup files each calling setupTestBed() with different providers/options and demonstrating that the later call wins; update the paragraph following the configuration options to include this example and reference to setupTestBed() and Vitest so readers can quickly visualize the multi-setup-file scenario.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/docs-app/docs/features/testing/vitest.md`:
- Around line 158-159: The documentation correctly describes that subsequent
calls to setupTestBed() replace the Angular test environment, but to make this
clearer add a short concrete example or reference showing two Vitest setup files
each calling setupTestBed() with different providers/options and demonstrating
that the later call wins; update the paragraph following the configuration
options to include this example and reference to setupTestBed() and Vitest so
readers can quickly visualize the multi-setup-file scenario.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 65dbe651-89d9-4e08-9c42-5edaf5cac59f
📒 Files selected for processing (2)
apps/docs-app/docs/features/testing/vitest.mdpackages/vitest-angular/setup-testbed.ts
✅ Files skipped from review due to trivial changes (1)
- packages/vitest-angular/setup-testbed.ts
Member
|
This needs to be validated against the Angular TestBed source code for compatibility and performance impact. I don't think this solution is a trivial one. |
Contributor
Author
I validated this against Angular's
Compatibility:
Performance:
Results:
So the change is source-backed for compatibility, and the measured impact on the code path it changes is a small but real performance improvement rather than a regression. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/vitest-angular/setup-testbed.ts`:
- Around line 105-124: The assignment to the ANGULAR_TESTBED_SETUP flag is done
before testBed.initTestEnvironment(), risking a stale true flag if
initTestEnvironment() throws; move the (globalThis as
any)[ANGULAR_TESTBED_SETUP] = true assignment so it runs only after
testBed.initTestEnvironment(...) completes successfully, and ensure any error
from testBed.initTestEnvironment lets the flag stay unset so subsequent calls
will attempt initialization (and avoid calling resetTestEnvironment() when no
successful init occurred).
- Around line 84-85: The beforeEach/afterEach cleanup hooks are always
registered, causing duplicate handlers on repeated setupTestBed() calls; guard
the hook registration using the same Symbol guard used for testbed
initialization: check the initialization Symbol (the one defined alongside
setupTestBed) on globalThis, and only call beforeEach(getCleanupHook(false)) and
afterEach(getCleanupHook(true)) when that Symbol is not set, then set the Symbol
to true after registering to prevent future duplicates; reference getCleanupHook
and setupTestBed so you add the guard in the same module and reuse the existing
Symbol used for testbed initialization.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1dbe8977-d68b-4a45-adb9-27cb182ad137
📒 Files selected for processing (2)
packages/vitest-angular/setup-testbed.tstests/vitest-angular/vitest.config.ts
✅ Files skipped from review due to trivial changes (1)
- tests/vitest-angular/vitest.config.ts
Contributor
Author
|
@coderabbitai I verified this against the current code and did not apply the suggested fix.
Verification:
Conclusion:
|
|
Tip For best results, initiate chat on the files or code changes.
For reference, a safe deduplication path (if ever desired) would require a separate module-scoped or global guard symbol solely for tracking whether hooks have been registered for the current setup-file context — completely independent of ✏️ Learnings added
🧠 Learnings used |
|
Tip For best results, initiate chat on the files or code changes.
I've already captured this constraint in the knowledge base (learning If the duplicate registration in a single setup file ever becomes a concern worth addressing, the right path is a new global symbol scoped exclusively to hook deduplication — something like ✏️ Learnings added
🧠 Learnings used |
|
Tip For best results, initiate chat on the files or code changes.
No additional action needed here; the knowledge base already reflects the correct constraint, and the current implementation passing all 10 tests confirms the PR is in good shape. 🧠 Learnings used |
brandonroberts
requested changes
Apr 13, 2026
Member
brandonroberts
left a comment
brandonroberts
left a comment
There was a problem hiding this comment.
Having one test does not validate that the TestBed doesn't leak across tests. This needs to be validated as a solution for Angular. Reconfiguring the TestBed environment everytime is not a proposed solution in Angular
Contributor
Author
I am a bit confused as to how to proceed. May I recommend ... let's take this to discord chat and maybe put this up against our internal repo and run a batter of tests. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Checklist
Closes #2222
Affected scope
vitest-angulartests,docsRecommended merge strategy for maintainer [optional]
Commit preservation note [optional]
What is the new behavior?
This PR hardens
setupTestBed()against Vitest project-order leakage inside a shared worker/process and adds a supported debugging path for follow-up reports.Before this change,
setupTestBed()initialized Angular's test environment only once per process. If another setup file calledsetupTestBed()later with different options such as additional providers, those options were silently ignored because the global singleton guard skipped reinitialization.This PR changes
setupTestBed()to reset the current Angular test environment before reinitializing it on later calls. That keeps each setup file's configuration authoritative instead of depending on whichever project initialized first.The PR also:
tests/vitest-angular/src/reconfigure-test-bedthat verifies repeatedsetupTestBed()calls in one Vitest process actually reconfigure the environmentsetupTestBed()independently without leaking providers/options across Vitest projectsanalog:vitest:testbedandanalog:vitest:testbed:vso future reports can capture setup/reconfigure behavior without patching local codeTest plan
nx format:checkpnpm nx build vitest-angularpnpm testManual verification:
pnpm nx build vitest-angularpnpm exec vitest run --config tests/vitest-angular/vitest.config.ts --project reconfigure-test-bedpnpm exec vitest run --config tests/vitest-angular/vitest.config.tsDoes this PR introduce a breaking change?
Other information
The existing
reset-test-bed-between-testsfixture still passes. The new regression covers the different failure mode here: latersetupTestBed()calls with different configuration being ignored in the same Vitest process.For future issue reports, users can now run Vitest with:
DEBUG=analog:vitest:testbed pnpm exec vitest run ...DEBUG=analog:vitest:testbed:v pnpm exec vitest run ...