feat(auth-server): migrate 15 Mocha test files to co-located Jest specs PART 7 by vbudhram · Pull Request #20161 · mozilla/fxa

vbudhram · 2026-03-10T15:47:14Z

Because

Mocha tests in test/local/ are legacy and being migrated to co-located Jest specs in lib/
Co-located specs improve discoverability, enable IDE test running, and align with the project's Jest-first testing direction
These 15 files cover core auth-server routes and utilities (382 total test cases)

This pull request

Adds 15 new .spec.ts files co-located next to their source modules in lib/
Converts all Mocha assert.* assertions to Jest expect() matchers while retaining sinon for mocking
Achieves 382/382 test case parity with the original Mocha files — no tests dropped
Converts lifecycle hooks: before/after → beforeAll/afterAll, beforeEach/afterEach unchanged
Updates jest.config.js to include jest.setup-proxyquire.js setup file

Test Parity

Test count per file (382/382 — 100% parity)

#	Module	Mocha file	Jest file	Mocha	Jest
1	customs	`test/local/customs.js`	`lib/customs.spec.ts`	18	18
2	account	`test/local/routes/account.js`	`lib/routes/account.spec.ts`	121	121
3	attached-clients	`test/local/routes/attached-clients.js`	`lib/routes/attached-clients.spec.ts`	16	16
4	cms	`test/local/routes/cms.js`	`lib/routes/cms.spec.ts`	28	28
5	devices-and-sessions	`test/local/routes/devices-and-sessions.js`	`lib/routes/devices-and-sessions.spec.ts`	44	44
6	emails	`test/local/routes/emails.js`	`lib/routes/emails.spec.ts`	33	33
7	geo-location	`test/local/routes/geo-location.js`	`lib/routes/geo-location.spec.ts`	6	6
8	linked-accounts	`test/local/routes/linked-accounts.js`	`lib/routes/linked-accounts.spec.ts`	34	34
9	newsletters	`test/local/routes/newsletters.js`	`lib/routes/newsletters.spec.ts`	11	11
10	oauth	`test/local/routes/oauth.js`	`lib/routes/oauth/index.spec.ts`	8	8
11	password	`test/local/routes/password.js`	`lib/routes/password.spec.ts`	15	15
12	security-events	`test/local/routes/security-events.js`	`lib/routes/security-events.spec.ts`	1	1
13	session	`test/local/routes/session.js`	`lib/routes/session.spec.ts`	36	36
14	support	`test/local/routes/support.js`	`lib/routes/subscriptions/support.spec.ts`	8	8
15	unblock-codes	`test/local/routes/unblock-codes.js`	`lib/routes/unblock-codes.spec.ts`	3	3
	Total			382	382

Assertion count comparison

#	Module	Mocha (`assert.` + `sinon.assert.`)	Jest (`expect()` + `sinon.assert.*`)	Delta	Notes
1	customs	105 (104 actual)	99	-5
2	account	670 (668 actual)	633 (632 actual)	-36	Largest file; some multi-assert patterns collapsed
3	attached-clients	68 (67 actual)	63	-4
4	cms	103 (102 actual)	97	-5
5	devices-and-sessions	196 (195 actual)	201 (199 actual)	+4	Jest added more granular checks
6	emails	180 (179 actual)	179 (178 actual)	-1
7	geo-location	15	15	0
8	linked-accounts	176 (175 actual)	173 (170 actual)	-5
9	newsletters	20 (19 actual)	19	0
10	oauth	16 (15 actual)	13	-2
11	password	150	148	-2
12	security-events	10	10	0
13	session	234 (231 actual)	228 (225 actual)	-6
14	support	24 (23 actual)	22	-1
15	unblock-codes	23	23	0
	Total	1990 (1976 actual)	1923 (1913 actual)	-63

"Actual" counts exclude false positives from grep (variable declarations like const assert = ..., comments, etc.).

The -63 delta (3.2%) is a syntactic artifact, not a coverage gap. Common causes:

assert.ok(x); assert.equal(x.length, 1) → expect(x).toHaveLength(1) (2 assertions → 1)

assert.equal(x, true, 'message string') → expect(x).toBe(true) (message param dropped; Jest uses test names for context)

Some Mocha tests had redundant guard assertions (assert.ok(result) before assert.equal(result.foo, ...)) that were consolidated

devices-and-sessions is the only file with a positive delta (+4), where the Jest version added more granular checks

All 382 test cases and their behavioral coverage are equivalent.

Issue

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

Checklist

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

How to test

cd packages/fxa-auth-server

# Run just the migrated specs (~19s)
npx jest lib/**/*.spec.ts lib/routes/**/*.spec.ts --verbose

# Run full unit test suite (~26s, 574 tests — excludes integration)
nx test-unit fxa-auth-server

# Run everything including verification-reminders (618 tests)
npx jest --verbose

Notes

Sinon is retained for mocking (not converted to jest.fn()) to minimize behavioral risk during migration
The original Mocha files in test/local/ are not removed in this PR — deletion is a separate step after CI validation
verification-reminders.spec.ts (44 tests) is excluded from nx test-unit since it requires Redis (treated as integration)

dschom

Leaving partial review comments. Currently I have looked at

packages/fxa-auth-server/lib/routes/oauth/index.spec.ts
packages/fxa-auth-server/lib/routes/subscriptions/support.spec.ts
packages/fxa-auth-server/lib/routes/account.spec.ts

I think there's a few things we should address in this PR, so as not make more work for ourselves later. Part of switching over to jest is using jest mocks and assertions, which is noted in the ticket, so:

Let's juse jest mock and stop using sinon. Worth noting seems like we've been doing a good job removing proxyquire.
Let's use .rejects instead of try/catches
Let's use .toBeCalledWith instead of .args[][]
Let's be consistent with use of afterEach & afterAll for mock resets & restores
We can take care of types in later reviews, so any are okay for now, but let's clean up low hanging things like getting rid of unused args and what not.

dschom · 2026-03-11T00:41:30Z

+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+export {};


What's up with these export empty objects...?

dschom · 2026-03-11T00:42:08Z

+
+import sinon from 'sinon';
+
+const mocks = require('../../../test/mocks');


Why are we requiring in a .ts file?

After side barring, I see what's going here... I am leaving this comment for historical record, so people know this is intended to be temporary and intentional so as to limit the number of changes that could affect parity. I'm not going to call out any stuff like this going forward.

dschom · 2026-03-11T00:45:35Z

+
+      sinon.assert.calledOnce(mockVerify);
+      expect(resp).toEqual(MOCK_ID_TOKEN_CLAIMS);
+      mockVerify.restore();


This should be done in an afterEach

dschom · 2026-03-11T00:48:09Z

+export {};
+
+import sinon from 'sinon';
+import nock from 'nock';


We should use jest for 'nocking'.

dschom · 2026-03-11T18:51:19Z

+
+      sinon.assert.calledOnce(mockVerify);
+      expect(resp).toEqual(MOCK_ID_TOKEN_CLAIMS);
+      mockVerify.restore();


This isn't needed. Already in afterEach... Clean up all occurrences.

dschom · 2026-03-13T01:25:51Z

+          Promise.reject(new Error('Database connection failed'))
+        );
+
+        return runTest(route, mockRequest, (response: any) => {


Remove unused response arg

dschom · 2026-03-13T01:28:22Z

+          },
+        ]);
+      });
+      return runTest(route, mockRequest, (response: any) => {


Remove unused response arg

dschom · 2026-03-13T01:28:36Z

+        expect(securityQuery.uid).toBe(uid);
+        expect(securityQuery.ipAddr).toBe(clientAddress);
+
+        expect(!!record).toBe(true);


Let's be consistent. Other places we've been doing it like this: expect(record).toBeTruty()

dschom · 2026-03-13T01:29:23Z

+        expect(record).toBeUndefined();
+      });
+    });
+  });


Move the test 'it records security event' here. Some how it got hoisted up to the top of the file.

dschom · 2026-03-13T01:33:21Z

+        allowedRegions: ['US'],
+      });
+      // Set after route creation — handler reads from Container at call time
+      Container.set(RecoveryPhoneService, mockService);


Why does it matter if it's done before or after?

vbudhram · 2026-03-13T14:21:51Z

Leaving partial review comments. Currently I have looked at

* `packages/fxa-auth-server/lib/routes/oauth/index.spec.ts`

* `packages/fxa-auth-server/lib/routes/subscriptions/support.spec.ts`

* `packages/fxa-auth-server/lib/routes/account.spec.ts`

I think there's a few things we should address in this PR, so as not make more work for ourselves later. Part of switching over to jest is using jest mocks and assertions, which is noted in the ticket, so:

* Let's juse jest mock and stop using sinon. Worth noting seems like we've been doing a good job removing proxyquire.

* Let's use `.rejects` instead of `try/catches`

* Let's use `.toBeCalledWith` instead of  `.args[][]`

* Let's be consistent with use of `afterEach` & `afterAll` for mock resets & restores

* We can take care of types in later reviews, so `any` are okay for now, but let's clean up low hanging things like getting rid of unused args and what not.

The sinion removal was noted in this PR description. Keeping sinion assertions allow us to have near direct parity to verify no gaps in testing coverage and logic, whereas jest we have to do more thinking to verify they match since it can simpfly assertions. There is a ticket filed to track this update https://mozilla-hub.atlassian.net/browse/FXA-13263.

I can update the PR to include your other comments though.

dschom · 2026-03-14T01:28:36Z

+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+export {};


dschom · 2026-03-14T01:30:16Z

+      expect(err.errno).toBe(error.ERRNO.INVALID_PARAMETER);
+    }
+
+    expect(devices.destroy.calledOnceWith(request, deviceId)).toBe(true);


Should be expect(devices.destroy).toHaveBeenCalledWith(request, deviceId)

dschom · 2026-03-14T01:31:02Z

+    }
+
+    expect(devices.destroy.calledOnceWith(request, deviceId)).toBe(true);
+    expect(db.deleteSessionToken.notCalled).toBe(true);


Ditto, pattern exists throughout file.

dschom · 2026-03-14T01:32:55Z

+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+export {};


dschom · 2026-03-14T01:33:53Z

+    };
+
+    mockCmsManager = {
+      fetchCMSData: sinon.stub(),


Why don't we use the sinon sandbox? If we are worried about tests drifting in behavior, seems like keeping the original sandbox approach is better?

dschom · 2026-03-14T01:37:50Z

+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+export {};


I don't see why this is needed.

dschom · 2026-03-14T01:39:07Z

+        uid: uid,
+      },
+      payload: {
+        id: deviceId,


I think it's fine, but previously this was deviceId.toString('hex')

dschom · 2026-03-14T01:40:06Z

+      },
+    };
+    return runTest(route, mockRequest, () => {
+      expect(false).toBeTruthy();


In other tests we've just been throwing an error. Let's be consistent throw new Error('should have thrown').

dschom · 2026-03-14T01:41:31Z

+          userAgent: 'test user-agent',
+          sigsciRequestId: 'test-sigsci-id',
+          clientJa4: 'test-ja4',
+          uid: uid,


Same comment as above. For some reason old tests were calling uid.toString('hex'). I'm guessing it's not necessary though...

dschom · 2026-03-14T01:44:25Z

+    mockCustoms = mocks.mockCustoms();
+  });
+
+  it('retrieves messages using the pushbox service', () => {


Not there's a pending change on this one, https://github.com/mozilla/fxa/pull/20183/changes

There will be some drift, I think its ok if we can address that in follow ups. My goal (in theory) was to get the tests landed, then locked, then update/refactor more before deleting the mocha ones.

dschom · 2026-03-16T17:13:50Z

+          '/linked_account/login'
+        );
+
+        await expect(runTest(route, mockRequest)).rejects.toMatchObject({


In other places in this PR, we did not use rejects. I still feel like this the right thing to do, but can we be consistent with changes?

Sigh, this was addressed as part of your original feedback but I suppose there were some missing parts. I'll do another pass and see what it finds.

dschom · 2026-03-16T17:20:09Z

+      sinon.stub(ScopeSet, 'fromArray').returns({ contains: () => true });
+    });
+
+    afterEach(() => {


Calling this out for future cleanup, but this seems like a much more typical way to write at test. Either way, it'd be nice to be consistent between test suites in the future.

dschom · 2026-03-16T17:22:40Z

+        passwordRoutes,
+        '/password/forgot/send_otp',
+        mockRequest
+      ).then((response: any) => {


In other test conversions we moved away from promise changes and switched to async/await, which is better. Just calling this out for future improvement / consistency.

dschom · 2026-03-16T17:35:55Z

+      expect(err.output.headers['retry-after']).toBe('10001');
+    }
+
+    // flag() is now a noop


Looks like dropped an assertion... See 'Nothing is returned when flag() is called (now a noop)' in original file. Maybe it's fine not to check this... it's just not one to one with the previous test suite.

The assertion is below, verifies the same as mocha.

dschom · 2026-03-16T18:00:42Z

    let accessToken2: any;

    beforeEach(async () => {
-      await redis.redis.flushall();


There's a note about why we don't use flush all due to concerns with parallel test workers. Jest actually runs in parallel by default. Are we sure we ant to switch to flush all?

Also why are these tests being altered? Isn't this PR just for unit tests?

This change is technically unrelated but I thought necessary since its current form introduces flaky behavior in the remote tests (it failed in one of the PR runs while working on this). In mocha, our integration tests run serially so the flushAll is fine, however, with jest we are running multiple tests at same time and the flushAll could break unrelated tests.

dschom · 2026-03-16T18:01:02Z

    let oldMeta: any;

    beforeEach(async () => {
-      await redis.redis.flushall();


Same comment as before.

dschom

R+WC

…ix l10n test brittleness

vbudhram · 2026-03-17T19:17:43Z

@dschom Thanks for the reviews, I've made the non sinion updates (I think). We got a few more iterations so will also be able to address if anything is missing.

vbudhram requested a review from a team as a code owner March 10, 2026 15:47

vbudhram changed the title ~~feat(auth-server): migrate 15 Mocha test files to co-located Jest specs~~ feat(auth-server): migrate 15 Mocha test files to co-located Jest specs PART 7 Mar 10, 2026

vbudhram force-pushed the fxa-12611-v2 branch 3 times, most recently from a690a58 to e7dddc3 Compare March 11, 2026 17:30

vbudhram self-assigned this Mar 11, 2026

vbudhram force-pushed the fxa-12611-v2 branch from e7dddc3 to 94990ae Compare March 11, 2026 20:16

dschom requested changes Mar 13, 2026

View reviewed changes

dschom reviewed Mar 14, 2026

View reviewed changes

vbudhram force-pushed the fxa-12611-v2 branch from 94990ae to 6fe7e15 Compare March 14, 2026 04:24

dschom reviewed Mar 16, 2026

View reviewed changes

dschom approved these changes Mar 16, 2026

View reviewed changes

vbudhram force-pushed the fxa-12611-v2 branch from 6fe7e15 to c9e99d4 Compare March 17, 2026 14:11

feat(auth-server): migrate Mocha tests to co-located Jest specs and f…

87c95bb

…ix l10n test brittleness

vbudhram force-pushed the fxa-12611-v2 branch from c9e99d4 to 87c95bb Compare March 17, 2026 17:11

vbudhram merged commit a7638d6 into main Mar 17, 2026
22 checks passed

vbudhram deleted the fxa-12611-v2 branch March 17, 2026 19:16


		import sinon from 'sinon';

		const mocks = require('../../../test/mocks');

Conversation

vbudhram commented Mar 10, 2026

Because

This pull request

Test Parity

Test count per file (382/382 — 100% parity)

Assertion count comparison

Issue

Checklist

How to test

Notes

Uh oh!

dschom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vbudhram commented Mar 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

dschom Mar 14, 2026 •

edited

Loading

dschom Mar 14, 2026 •

edited

Loading

dschom Mar 16, 2026 •

edited

Loading