feat(fxa-auth-server): migrate MFA/recovery phone Mocha tests to co-located by vbudhram · Pull Request #20185 · mozilla/fxa

vbudhram · 2026-03-13T18:19:29Z

Because

MFA and recovery route unit tests still use Mocha + chai in test/local/, outside the co-located Jest pattern established for lib/**/*.spec.ts
Part of the broader Mocha → Jest migration (FXA-12536) to unify the test runner and enable TypeScript-native test files

This pull request

Migrates 5 Mocha JS test files to co-located Jest TypeScript .spec.ts files:
- serverJWT.spec.ts — 5 tests, proxyquire → jest.doMock/jest.resetModules
- recovery-codes.spec.ts — 8 tests, merged assert pattern split
- totp.spec.ts — 16 tests, TOTP generation via otplib
- recovery-key.spec.ts — 52 tests, proxyquire → jest.mock with delegating function pattern
- recovery-phone.spec.ts — 34 tests, largest file (1075 lines), custom Glean mocks
Converts chai.assert.* → expect().*, assert.isRejected → .rejects.toThrow(), assert.fail guards → .rejects.*
Preserves sinon.assert.* calls alongside Jest (established codebase pattern)
Adds Container.reset() in afterAll for TypeDI cleanup
Does not delete old Mocha files (follow-up task)
115 tests, 100% parity with original Mocha suites

Parity Comparison

File	Mocha `it()`	Jest `it()`	Mocha Assertions	Jest Assertions	Notes
serverJWT	5	5	16	14	`assert.isTrue(spy.calledOnceWith)` split; `try/catch` → `.rejects.toThrow`
recovery-codes	8	8	27	26	Redundant `assert.isDefined` removed
totp	16	16	83	72	3 `try/catch` → `.rejects.toMatchObject`; `assert.fail` guards removed
recovery-key	52	52	135	133	`assert.fail` in `.then(fail, err)` patterns removed
recovery-phone	34	34	170	150	14 redundant `assert.isDefined` removed; `assert.isRejected` → `.rejects.toThrow`
Total	115	115	431	395	-36 = redundant guards only, no coverage dropped

Issue

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

Checklist

My commit is GPG signed
Tests pass locally (if applicable)
Documentation updated (if applicable)
RTL rendering verified (if UI changed)

Other Information

How to verify:

cd packages/fxa-auth-server
NODE_ENV=dev npx jest --no-coverage lib/serverJWT.spec.ts lib/routes/recovery-codes.spec.ts lib/routes/totp.spec.ts lib/routes/recovery-key.spec.ts lib/routes/recovery-phone.spec.ts

All 115 tests pass. The -36 assertion delta vs Mocha is entirely from removing redundant assert.isDefined guards before property access and collapsing try/catch + assert.fail into Jest-native .rejects patterns — no behavioral coverage was dropped.

nshirley · 2026-03-20T21:01:00Z

+        expect(db.totpToken.callCount).toBe(1);
+
+        // emits correct metrics
+        expect(request.emitMetricsEvent.callCount).toBe(1);


Obviously nothing to change, just noting that this is the opposite of the kind of tests I highlighted above 😅 . The test just says "should return false for invalid TOTP code" but then it has a bunch of other assertions

nshirley · 2026-03-20T21:02:07Z

non-blocking, just curious why you removed this?

Ah, it is still there just got moved to diff line.

nshirley

Nothing blocking, just a few observations why checking out the tests!

Copilot

Pull request overview

Migrates MFA and recovery-related route/unit tests in fxa-auth-server from legacy Mocha/Chai test/local/** to co-located Jest TypeScript specs under lib/**, aligning with the repo’s Jest co-location pattern and supporting the broader Mocha → Jest migration.

Changes:

Added co-located Jest .spec.ts suites for serverJWT, totp, recovery-codes, recovery-key, and recovery-phone.
Replaced Chai assertions with Jest expect patterns and updated module mocking (proxyquire → jest.mock/jest.doMock).
Added TypeDI cleanup via Container.reset() in relevant suites.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
packages/fxa-auth-server/lib/serverJWT.spec.ts	New Jest/TS unit tests for JWT signing/verifying with `jest.doMock` of `jsonwebtoken`.
packages/fxa-auth-server/lib/routes/totp.spec.ts	New Jest/TS tests for TOTP routes and setup/verify flows, including Redis and Glean mocks.
packages/fxa-auth-server/lib/routes/recovery-codes.spec.ts	New Jest/TS tests for recovery codes routes and related events/metrics.
packages/fxa-auth-server/lib/routes/recovery-key.spec.ts	New Jest/TS tests for recovery key routes, including mocking authorized clients.
packages/fxa-auth-server/lib/routes/recovery-phone.spec.ts	New Jest/TS tests for recovery phone routes, custom mocks, and webhook verification paths.

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

Copilot · 2026-03-23T14:46:38Z

+      );
+      sinon.assert.calledOnceWithExactly(
+        mockStatsd.increment,
+        'account.recoveryPhone.signinSendCode.success'


RecoveryPhoneHandler calls statsd.increment(metric, getClientServiceTags(request)), which always provides a second argument (an object, often {} in these unit tests). This assertion uses calledOnceWithExactly but only checks the metric name, so it will fail if increment is invoked with the expected tags argument. Update the assertion to include the tags argument (e.g., {}/sinon.match({})) or relax it to calledOnce + argument checks.

Suggested change

'account.recoveryPhone.signinSendCode.success'

'account.recoveryPhone.signinSendCode.success',

sinon.match({})

Copilot · 2026-03-23T14:46:39Z

+          },
+        }
+      );
+      sinon.assert.calledOnceWithExactly(


statsd.increment is invoked by the route with a second argument for client/service tags (typically {} in these unit tests). This calledOnceWithExactly assertion only expects the metric string, so it will not match the actual call signature. Include the tags argument in the expectation or relax the assertion.

Suggested change

sinon.assert.calledOnceWithExactly(

sinon.assert.calledOnce(mockStatsd.increment);

sinon.assert.calledWith(

Copilot · 2026-03-23T14:46:39Z

+      expect(mockCustoms.checkAuthenticated.getCall(0).args[3]).toBe(
+        'recoveryPhoneSendSetupCode'
+      );
+      sinon.assert.calledOnceWithExactly(


The route calls statsd.increment(metric, getClientServiceTags(request)), so increment receives a second (tags) argument. This assertion uses calledOnceWithExactly but only supplies the metric name, which will fail against the actual call. Assert on both arguments (metric + tags) or switch to a less strict assertion.

Suggested change

sinon.assert.calledOnceWithExactly(

sinon.assert.calledWithMatch(

Copilot · 2026-03-23T14:46:40Z

+        }
+      );
+
+      sinon.assert.calledOnceWithExactly(


statsd.increment is called with a tags object via getClientServiceTags(request) in the handler. This calledOnceWithExactly expectation only includes the metric string, so it won't match the real invocation (which includes a second argument). Update the assertion to expect the tags argument as well (often {} in these tests) or relax it.

Suggested change

sinon.assert.calledOnceWithExactly(

sinon.assert.calledOnce(mockStatsd.increment);

sinon.assert.calledWith(

Copilot · 2026-03-23T14:46:40Z

+      sinon.assert.calledOnceWithExactly(
+        mockStatsd.increment,


This handler increments StatsD with client/service tags (statsd.increment(metric, getClientServiceTags(request))). The calledOnceWithExactly assertion only expects the metric name, so it will fail when the tags object is provided as the second argument. Include the tags argument in the assertion or relax it.

Suggested change

sinon.assert.calledOnceWithExactly(

mockStatsd.increment,

sinon.assert.calledOnce(mockStatsd.increment);

expect(mockStatsd.increment.getCall(0).args[0]).toBe(

Copilot · 2026-03-23T14:46:40Z

+          },
+        }
+      );
+      sinon.assert.calledOnceWithExactly(


statsd.increment receives a second argument for tags (getClientServiceTags(request)). This calledOnceWithExactly assertion only passes the metric string, so it will not match the real call signature. Update the assertion to include the tags argument (likely {} here) or make the assertion less strict.

Suggested change

sinon.assert.calledOnceWithExactly(

sinon.assert.calledOnce(mockStatsd.increment);

sinon.assert.calledWith(

Copilot · 2026-03-23T14:46:41Z

+      );
+      sinon.assert.calledOnceWithExactly(
+        mockStatsd.increment,
+        'account.recoveryPhone.phoneRemoved.success'


The recovery-phone handlers call statsd.increment(metric, getClientServiceTags(request)), which passes a second tags argument. This calledOnceWithExactly assertion only expects the metric name, so it will fail if increment is invoked with {} as the second parameter. Include the tags argument (or use a looser assertion) to match the real call.

Suggested change

'account.recoveryPhone.phoneRemoved.success'

'account.recoveryPhone.phoneRemoved.success',

sinon.match.any

… Jest specs (FXA-12612)

vbudhram · 2026-03-23T17:36:43Z

Copilot picked up that the tests were failing because of stale assertion, fixed the tests and rebasing

vbudhram requested a review from a team as a code owner March 13, 2026 18:19

vbudhram self-assigned this Mar 13, 2026

vbudhram force-pushed the fxa-12612 branch 2 times, most recently from f86524b to 1c4c787 Compare March 14, 2026 04:06

vbudhram requested review from nshirley March 18, 2026 17:14

nshirley reviewed Mar 20, 2026

View reviewed changes

nshirley approved these changes Mar 20, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings March 23, 2026 14:38

vbudhram force-pushed the fxa-12612 branch from 1c4c787 to d3a6016 Compare March 23, 2026 14:38

Copilot AI reviewed Mar 23, 2026

View reviewed changes

test(fxa-auth-server): migrate MFA/recovery Mocha tests to co-located…

00bea49

… Jest specs (FXA-12612)

vbudhram force-pushed the fxa-12612 branch from d3a6016 to 00bea49 Compare March 23, 2026 17:34

vbudhram merged commit 68edb2b into main Mar 23, 2026
21 checks passed

vbudhram deleted the fxa-12612 branch March 23, 2026 18:37

	'account.recoveryPhone.signinSendCode.success'
	'account.recoveryPhone.signinSendCode.success',
	sinon.match({})

	sinon.assert.calledOnceWithExactly(
	sinon.assert.calledOnce(mockStatsd.increment);
	sinon.assert.calledWith(

	sinon.assert.calledOnceWithExactly(
	sinon.assert.calledWithMatch(

	'account.recoveryPhone.phoneRemoved.success'
	'account.recoveryPhone.phoneRemoved.success',
	sinon.match.any

Conversation

vbudhram commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Because

This pull request

Parity Comparison

Issue

Checklist

Other Information

Uh oh!

nshirley Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

nshirley Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

vbudhram Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

nshirley left a comment

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!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

vbudhram commented Mar 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vbudhram commented Mar 13, 2026 •

edited

Loading