test(auth-server): drop per-test resetModules to fix Jest worker OOM by vbudhram · Pull Request #20465 · mozilla/fxa

vbudhram · 2026-04-27T16:46:57Z

Because

lib/metrics/context.spec.ts was SIGKILL'ing a Jest worker
Root cause is per-test jest.resetModules() + jest.doMock(...) + require('./context') in a shared beforeEach. That reloaded the ./context → validators → fxa-shared → joi graph ~94 times per file. Eight sibling specs followed the same anti-pattern.

This pull request

Refactors lib/metrics/context.spec.ts to mock ../metricsCache once at file scope, load ./context once, and recreate metricsContext via the factory per-test. 33.5 s → 2.6 s.
Applies the same pattern to eight sibling specs: lib/log.spec.ts (28 tests), lib/monitoring.spec.ts (8), lib/geodb.spec.ts (2), lib/oauth/jwt.spec.ts (11), lib/oauth/jwt_access_token.spec.ts (11), lib/oauth/grant.spec.ts (13), lib/push.spec.ts (28), lib/serverJWT.spec.ts (5).

Issue that this pull request solves

Closes: https://mozilla-hub.atlassian.net/browse/FXA-13473

Checklist

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.

Other information

How to test:

cd packages/fxa-auth-server
yarn jest lib/metrics/context.spec.ts lib/log.spec.ts lib/monitoring.spec.ts \
          lib/geodb.spec.ts lib/oauth/jwt.spec.ts lib/oauth/jwt_access_token.spec.ts \
          lib/oauth/grant.spec.ts lib/push.spec.ts lib/serverJWT.spec.ts
# 9 suites, 145 tests, ~5s
yarn jest  # full suite: 150 suites, 3379 tests, ~55s

Verified clean across 5 consecutive full-suite runs. No production code changed; refactor is test-only.

Copilot

Pull request overview

Refactors several fxa-auth-server Jest unit specs to avoid per-test jest.resetModules() / jest.doMock() patterns that repeatedly reload heavy dependency graphs and can trigger Jest worker OOMs.

Changes:

Convert test suites to file-scope jest.mock(...) and single module require(...), resetting mock implementations per-test instead of resetting the module registry.
Rework tests to create fresh instances via factories (or fresh config objects) while reusing the already-loaded module graph.
Update assertions to use shared mock functions and, where needed, snapshot module-load side-effect call arguments to accommodate clearMocks: true.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
packages/fxa-auth-server/lib/serverJWT.spec.ts	Mock `jsonwebtoken` once and reset `sign`/`verify` mocks per test.
packages/fxa-auth-server/lib/push.spec.ts	Mock `web-push.sendNotification` at file scope and reuse the push factory.
packages/fxa-auth-server/lib/oauth/jwt_access_token.spec.ts	Mock `./jwt` once and switch tests to reconfigure `sign`/`verify` mocks instead of re-requiring.
packages/fxa-auth-server/lib/oauth/jwt.spec.ts	Mock `./keys.publicPEM` via a delegating jest fn to allow per-test overrides without resetModules.
packages/fxa-auth-server/lib/oauth/grant.spec.ts	Move config/db/jwt mocks to file scope and reuse the already-required `./grant` module.
packages/fxa-auth-server/lib/monitoring.spec.ts	Mock dependencies at file scope and snapshot one-shot module-load calls for assertions.
packages/fxa-auth-server/lib/metrics/context.spec.ts	Mock `../metricsCache` once and create fresh contexts via the factory per test.
packages/fxa-auth-server/lib/log.spec.ts	Mock config and dependencies at file scope; avoid logger name collisions without resetting modules.
packages/fxa-auth-server/lib/geodb.spec.ts	Mock config once and mutate captured config object per test instead of reloading the module.

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

Copilot · 2026-04-27T16:54:25Z

+const grantModule = require('./grant');
+const { validateRequestedGrant, generateTokens, setStripeHelper } = grantModule;
+// eslint-disable-next-line @typescript-eslint/no-require-imports
+const Container = require('typedi').default;


grant.js imports Typedi via const { Container } = require('typedi'), but this spec is using require('typedi').default. In CommonJS this is often undefined (or a different value than the named Container export), which would make Container.set(...) fail or target a different container than grant.js uses (so setStripeHelper() would not pick up the mock). Use the same import shape as production code (e.g. const { Container } = require('typedi')).

Suggested change

const Container = require('typedi').default;

const { Container } = require('typedi');

vpomerleau · 2026-04-27T17:45:24Z

+}));
+
+// eslint-disable-next-line @typescript-eslint/no-require-imports
+const { MetricsRedis } = require('../metricsCache');


Is there a reason for using require vs plain import? Switching would avoid the eslint-disable comments (same comment for all require in these files)

I originally thought this was ok since other tests used require but looks like can they can use import, updating.

vpomerleau · 2026-04-27T17:49:34Z

-    // Get ScopeSet from the same module registry as the freshly-required grant
-    // module, so that instanceof checks inside grant.js work correctly.
-    FreshScopeSet = require('fxa-shared').oauth.scopes;
+    mockDB.getScope.mockReset();


Should we also reset the Container to avoid leaks between test blocks?

…XA-13473) Because: `lib/metrics/context.spec.ts` was reloading the ./context -> validators -> fxa-shared -> joi module graph in every `beforeEach` via `jest.resetModules()` + `jest.doMock('../metricsCache', ...)` + `require('./context')`. With ts-jest, --coverage, and 4 workers, the retained heap from ~94 reloads per file SIGKILL'd a Jest worker on the Unit Test (PR) job. Eight sibling specs followed the same anti-pattern. This commit: * Refactors lib/metrics/context.spec.ts to mock ../metricsCache once at file scope, load ./context once, and recreate metricsContext via the factory per-test. The validate tests now call the same factory with per-test config instead of re-requiring the module. 33.5s -> 2.6s. * Applies the same pattern to eight sibling specs that called jest.resetModules() in beforeEach or in helper fns invoked per-test: lib/log.spec.ts (28 tests), lib/monitoring.spec.ts (8), lib/geodb.spec.ts (2), lib/oauth/jwt.spec.ts (11), lib/oauth/jwt_access_token.spec.ts (11), lib/oauth/grant.spec.ts (13), lib/push.spec.ts (28), lib/serverJWT.spec.ts (5). * Captures three non-obvious gotchas surfaced by removing the resets: - log.js keeps a module-level _registered map for logger-name dedup, so each test now uses a unique name (nextLoggerName helper). - clearMocks: true in the global jest config wipes mock.calls between tests; monitoring.spec.ts snapshots the one-shot initMonitoring() args at file scope before clearMocks fires. - geodb.js captures the config object by reference at module load, so the spec mutates properties on a stable object instead of reassigning the variable. Tests: 145 tests across the 9 refactored specs pass; full auth-server suite (150 suites / 3379 tests) clean across 5 consecutive runs.

Copilot AI review requested due to automatic review settings April 27, 2026 16:46

vbudhram requested a review from a team as a code owner April 27, 2026 16:46

Copilot started reviewing on behalf of vbudhram April 27, 2026 16:47 View session

vbudhram mentioned this pull request Apr 27, 2026

chore(ci): bump Unit Test (PR) to xlarge to complete FXA-13473 mitigation #20454

Closed

3 tasks

Copilot AI reviewed Apr 27, 2026

View reviewed changes

vbudhram force-pushed the fxa-13473 branch from 89ee9ef to 481ed3c Compare April 27, 2026 17:35

vpomerleau approved these changes Apr 27, 2026

View reviewed changes

vbudhram force-pushed the fxa-13473 branch from 481ed3c to 8fee32d Compare April 27, 2026 18:17

vbudhram merged commit 8b554a6 into main Apr 27, 2026
20 checks passed

vbudhram deleted the fxa-13473 branch April 27, 2026 19:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(auth-server): drop per-test resetModules to fix Jest worker OOM#20465

test(auth-server): drop per-test resetModules to fix Jest worker OOM#20465
vbudhram merged 1 commit into
mainfrom
fxa-13473

vbudhram commented Apr 27, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 27, 2026

Uh oh!

vpomerleau Apr 27, 2026

Uh oh!

vbudhram Apr 27, 2026

Uh oh!

vpomerleau Apr 27, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	const Container = require('typedi').default;
	const { Container } = require('typedi');

Conversation

vbudhram commented Apr 27, 2026

Because

This pull request

Issue that this pull request solves

Checklist

Other information

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

vpomerleau Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

vbudhram Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

vpomerleau Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants