test(auth-server): clean up test mocking leaked state by nshirley · Pull Request #20657 · mozilla/fxa

nshirley · 2026-05-28T17:37:00Z

Because:

Container.set() without paired cleanup and delete require.cache[...] leak state across tests and bypass Jest's module tracking, making the suite fragile under runner changes.

This commit:

Pairs Container.set() in test/ with matching cleanup in the corresponding teardown hook.
Replaces 6 delete require.cache[...] with jest.resetModules().
Restructures subscription_tests to require Container tokens inside beforeAll after resetModules, fixing latent identity drift between describe-scope captures and key_server's re-required deps.
Removes 3 redundant jest.clearAllMocks() calls (clearMocks: true already runs between tests).
Deletes Mocha-era test/lib/server.js duplicate.
Documents conditional cleanup nuance in .claude/rules/testing/base.md Rule 7.

Closes: FXA-13443

Checklist

Put an x in the boxes that apply

My commit is GPG signed.
If applicable, I have modified or added tests which pass locally.
I have added necessary documentation (if appropriate).
I have verified that my changes render correctly in RTL (if appropriate).
I have manually reviewed all AI generated code.

How to review (Optional)

Key files/areas to focus on:
Suggested review order:
Risky or complex parts:

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.

nshirley · 2026-06-01T14:17:46Z

@@ -236,6 +236,16 @@ No shared mutable state between tests. Each test owns its setup; use `beforeEach

 **Mid-test mock resets are a smell.** Calling `mockClear()`, `mockReset()`, `jest.clearAllMocks()`, or `jest.resetAllMocks()` inside an `it()` body is almost always a band-aid hiding state leakage from a prior test or from shared setup — fix the leak (scope the mock per-test, move it into `beforeEach`, stop sharing module-level mocks across describes) instead of clearing inline. The narrow legitimate case is asserting "no further calls happened after this point" in a multi-phase test; in that case, leave a comment explaining why.

+**TypeDI Container cleanup is conditional**, not mechanical. Pair `Container.set()` with cleanup *when leakage could change another test's observable behavior*:


While digging through these changes, Claude had flagged that the existing rules were too strict so it suggested expanding this. the short version is, calling Container.reset() or .remove() is more nuanced than just "always do it". Namely for our integration tests where we setup the container before integration server starts. If the server is a long running process then calling reset between tests doesn't do anything

nshirley · 2026-06-01T14:17:56Z

This was replaced by a ts version

nshirley · 2026-06-01T14:27:37Z

  }
  const { server, close } = await createServer(testConfig);
-  return wrapServer(server, close);
+  const wrappedClose = async () => {


Related to the changes to the testing GUIDELINES, if test stat sets the CapabilityService when starting up, we want to explicitly remove it when we're done.

nshirley · 2026-06-01T15:17:12Z


 afterAll(async () => {
  await server.stop();
+  Container.remove(PlaySubscriptions);


This is more or less the largest change in the branch. We are using this surgical approach as a work around for the fact that container is a global process and we use the shared mocks. At some point it would be ideal to move toward always using Container.reset() which clears everything between tests but that is blocked by the mocks refactor. The long term correct change would be using scoped containers, but that's a significant refactor of production code to support that.

Copilot

Pull request overview

This PR hardens fxa-auth-server’s Jest test suite against cross-test/module state leakage by standardizing module resets, pairing TypeDI Container.set() with teardown cleanup, and removing redundant mock resets (given clearMocks: true).

Changes:

Replaces targeted delete require.cache[...] patterns with jest.resetModules() for safer Jest-aware module reloading.
Adds/adjusts TypeDI Container.remove(...) cleanup in integration tests and test server helpers to avoid leaked DI bindings.
Removes redundant jest.clearAllMocks() calls, deletes legacy test/lib/server.js, and updates testing guidelines/docs to document the new patterns.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
packages/fxa-auth-server/test/support/helpers/test-utils.ts	Uses `jest.resetModules()` before re-reading config for metrics context generation.
packages/fxa-auth-server/test/support/helpers/test-server.ts	Uses `jest.resetModules()` before reading base config for spawned/shared server helpers.
packages/fxa-auth-server/test/scripts/stripe-products-and-plans-converter.in.spec.ts	Removes redundant `jest.clearAllMocks()` given package `clearMocks: true`.
packages/fxa-auth-server/test/remote/totp.in.spec.ts	Adds TypeDI token cleanup in `afterAll` for IAP subscription tokens.
packages/fxa-auth-server/test/remote/subscription_tests.in.spec.ts	Restructures module loading around `resetModules()` to preserve token identity; adds explicit Container cleanup.
packages/fxa-auth-server/test/remote/payments/configuration/manager.in.spec.ts	Removes redundant `jest.clearAllMocks()` and relies on Container reset/clearMocks.
packages/fxa-auth-server/test/remote/passkeys.in.spec.ts	Removes redundant per-test `jest.clearAllMocks()` hook.
packages/fxa-auth-server/test/remote/oauth_token_route.in.spec.ts	Captures `Container` once and adds cleanup for a string token after tests.
packages/fxa-auth-server/test/remote/mfa_totp.in.spec.ts	Adds TypeDI token cleanup in `afterAll` for IAP subscription tokens.
packages/fxa-auth-server/test/lib/server.ts	Adds conditional TypeDI cleanup on server shutdown to avoid leaking `CapabilityService`.
packages/fxa-auth-server/test/lib/server.js	Removes the legacy JS helper (replaced by TS variant).
packages/fxa-auth-server/jest.oauth-api.config.js	Updates comment to reference `test/lib/server.ts`.
CLAUDE.md	Documents conditional TypeDI cleanup guidance and `resetModules()` Container identity gotchas.
.claude/skills/fxa-testing-shared/GUIDELINES.md	Expands Rule 7 with concrete guidance on when/how to clean up TypeDI tokens.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+  const wrappedClose = async () => {
+    await close();
+    if (didSetCapabilityService) {
+      Container.remove(CapabilityService);
+    }
+  };


nshirley · 2026-06-01T16:59:04Z

  afterAll(async () => {
    if (server) {
      await server.close();
    }
    if (profileServer) {
      await profileServer.close();
    }
+    Container.remove(AppConfig);
+    Container.remove(AuthLogger);
+    Container.remove(StripeHelper);
+    Container.remove(PlaySubscriptions);
+    Container.remove(AppStoreSubscriptions);
+    Container.remove(ProfileClient);
+    Container.remove(CapabilityManager);
+    Container.remove(BackupCodeManager);
+    Container.remove(RecoveryPhoneService);
+    Container.remove(PriceManager);
+    Container.remove(ProductConfigurationManager);
+    Container.remove(CapabilityService);
  });


I mean, valid, but also if server.close() fails we have a bigger problem

 afterAll(async () => {
  await server.stop();
+  Container.remove(PlaySubscriptions);
+  Container.remove(AppStoreSubscriptions);
 });


 afterAll(async () => {
  await server.stop();
+  Container.remove(PlaySubscriptions);
+  Container.remove(AppStoreSubscriptions);
 });


toufali

LGTM! Thanks for the cleanup and your inline comments were helpful.

Because: - Container.set() without paired cleanup and delete require.cache[...] leak state across tests and bypass Jest's module tracking, making the suite fragile under runner changes. This commit: - Pairs Container.set() in test/ with matching cleanup in the corresponding teardown hook. - Replaces 6 delete require.cache[...] with jest.resetModules(). - Restructures subscription_tests to require Container tokens inside beforeAll after resetModules, fixing latent identity drift between describe-scope captures and key_server's re-required deps. - Removes 3 redundant jest.clearAllMocks() calls (clearMocks: true already runs between tests). - Deletes Mocha-era test/lib/server.js duplicate. - Documents conditional cleanup nuance in .claude/rules/testing/base.md Rule 7. Closes: FXA-13443

nshirley · 2026-06-03T02:50:26Z

Latest force push was to fix merge conflicts in claude specific files

nshirley commented Jun 1, 2026

View reviewed changes

Comment thread packages/fxa-auth-server/test/lib/server.js Outdated

Copy link
Copy Markdown

Contributor Author

nshirley Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was replaced by a ts version

nshirley commented Jun 1, 2026

View reviewed changes

nshirley force-pushed the worktree-FXA-13443 branch from 0a6932c to 76db480 Compare June 1, 2026 15:39

nshirley marked this pull request as ready for review June 1, 2026 15:40

nshirley requested a review from a team as a code owner June 1, 2026 15:40

Copilot AI review requested due to automatic review settings June 1, 2026 15:40

Copilot started reviewing on behalf of nshirley June 1, 2026 15:40 View session

Copilot AI reviewed Jun 1, 2026

View reviewed changes

toufali approved these changes Jun 2, 2026

View reviewed changes

nshirley force-pushed the worktree-FXA-13443 branch from 76db480 to 9472eea Compare June 3, 2026 02:50

nshirley merged commit 67fbdc9 into main Jun 3, 2026
20 checks passed

nshirley deleted the worktree-FXA-13443 branch June 3, 2026 03:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(auth-server): clean up test mocking leaked state#20657

test(auth-server): clean up test mocking leaked state#20657
nshirley merged 1 commit into
mainfrom
worktree-FXA-13443

nshirley commented May 28, 2026 •

edited

Loading

Uh oh!

nshirley Jun 1, 2026

Uh oh!

nshirley Jun 1, 2026

Uh oh!

nshirley Jun 1, 2026

Uh oh!

nshirley Jun 1, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

nshirley Jun 1, 2026

Uh oh!

toufali left a comment

Uh oh!

nshirley commented Jun 3, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		@@ -236,6 +236,16 @@ No shared mutable state between tests. Each test owns its setup; use `beforeEach

		Mid-test mock resets are a smell. Calling `mockClear()`, `mockReset()`, `jest.clearAllMocks()`, or `jest.resetAllMocks()` inside an `it()` body is almost always a band-aid hiding state leakage from a prior test or from shared setup — fix the leak (scope the mock per-test, move it into `beforeEach`, stop sharing module-level mocks across describes) instead of clearing inline. The narrow legitimate case is asserting "no further calls happened after this point" in a multi-phase test; in that case, leave a comment explaining why.

		TypeDI Container cleanup is conditional, not mechanical. Pair `Container.set()` with cleanup when leakage could change another test's observable behavior:

Conversation

nshirley commented May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

How to review (Optional)

Screenshots (Optional)

Other information (Optional)

Uh oh!

nshirley Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

nshirley Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

nshirley Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

nshirley Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

nshirley Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

toufali left a comment

Choose a reason for hiding this comment

Uh oh!

nshirley commented Jun 3, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nshirley commented May 28, 2026 •

edited

Loading