test(auth-server): clean up test mocking leaked state#20657
Draft
nshirley wants to merge 2 commits into
Draft
Conversation
Because: - Container.set() calls without paired cleanup leak state across test files; today this is masked by Jest's per-file worker isolation, but is fragile under runner changes (--runInBand, parallelism, future migrations) and makes individual tests harder to reason about. - delete require.cache[...] bypasses Jest's module tracking and can desync with jest.mock() registrations. - jest.clearAllMocks() in beforeEach is redundant when the package's Jest config sets clearMocks: true (which fxa-auth-server does). This commit: - Pairs each Container.set() in the test/ tree with a matching Container.remove() in the corresponding teardown hook across oauth_token_route, totp, mfa_totp, subscription_tests, and the test/lib/server.ts helper. - Hoists the Container reference to module scope in oauth_token_route so afterAll's remove targets the same Container instance as beforeAll's set (jest.resetModules() in beforeAll would otherwise hand back a fresh Container, making the remove a no-op). - Replaces 6 delete require.cache[...] sites with jest.resetModules() so module reloads go through Jest's tracking and respect mock state. - Removes 3 redundant jest.clearAllMocks() calls (the package's clearMocks: true already clears mocks between every test). - Deletes the legacy test/lib/server.js (Mocha-era duplicate of server.ts; the .ts file resolves first via jest.setup-resolve.js, leaving the .js unreachable). - Updates GUIDELINES.md Rule 7 (and CLAUDE.md §8) with the nuance that Container cleanup is conditional — afterEach for per-call consumers, afterAll for boot-captured deps, optional for empty-stub satisfy-DI sets — plus the jest.resetModules() Container identity gotcha. Closes: FXA-13443
…identity safety Because: - The describe-scope captures of Container token classes (CapabilityService, StripeHelper, AuthLogger, AppConfig, etc.) loaded before the in-test jest.resetModules() calls. After the reset, key_server's deps re-require the same modules and get FRESH class references — different identity than what Container.set() registered. Container.get() lookups inside the in-process server were silently missing on identity. - The pre-FXA-13443 code used scoped delete require.cache[...] for just config and key_server, preserving cache for the token-class modules and keeping identity stable between test setup and server runtime. FXA-13443's broader jest.resetModules() conversion lost that invariant; this restores it the right way. This commit: - Moves the 12 token-class requires (Container, CapabilityService, StripeHelper, AuthLogger, AppConfig, ProfileClient, PlaySubscriptions, AppStoreSubscriptions, CapabilityManager, BackupCodeManager, RecoveryPhoneService, PriceManager, ProductConfigurationManager) from describe scope into beforeAll, after the first jest.resetModules(). Captures into describe-scope let bindings so afterAll can use them for paired Container.remove(). - Removes the second jest.resetModules() in beforeAll. With the requires now after the single reset, key_server's later require shares the same module graph — token identity is preserved. Not verified by running the test suite locally (worktree has no node_modules); CI is the verification.
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
1 participant
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.
Because
Container.set()without paired cleanup leaks state across test files. Masked today by Jest's per-file worker isolation, but fragile under runner changes (--runInBand, parallelism, future migrations).delete require.cache[...]bypasses Jest's module tracking and desyncs fromjest.mock()registrations.jest.clearAllMocks()calls exist where the package'sclearMocks: truealready runs between every test.This pull request
Container.set()infxa-auth-server's test tree with matching cleanup in the corresponding teardown hook.delete require.cache[...]sites withjest.resetModules(). Restructuressubscription_tests.in.spec.tsso token-class requires share key_server's module graph. Fixes a class-identity issue the original surgicaldelete require.cachewas inadvertently masking.jest.clearAllMocks()calls.test/lib/server.js(replaced with a typescript version during Jest migration).GUIDELINES.mdRule 7 (andCLAUDE.mdsection 8) noting Container cleanup is conditional.afterEachfor per-call consumers,afterAllfor boot-captured deps, optional for empty-stub satisfy-DI setsIssue that this pull request solves
Closes: FXA-13443
Checklist
Put an
xin the boxes that applyHow to review (Optional)
Screenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)
Any other information that is important to this pull request.