Conversation
There was a problem hiding this comment.
Pull request overview
This PR advances the FxA auth-server’s migration from Mocha to Jest by moving metrics/logging/monitoring tests into co-located lib/**/*.spec.ts suites (and adding new coverage for previously-untested monitoring initialization behavior).
Changes:
- Migrates Mocha
test/local/**suites to Jestlib/**/*.spec.ts, replacing sinon/proxyquire/chai patterns with Jest-native mocking. - Adds a new Jest suite for
lib/monitoring.jsto cover its module-loadinitMonitoring()behavior and Sentry event filtering. - Introduces Jest coverage for supporting modules like Sentry config, metrics cache, and metrics event emitters.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/fxa-auth-server/lib/sentry.spec.ts | Jest migration of Sentry-related tests and helper coverage. |
| packages/fxa-auth-server/lib/monitoring.spec.ts | New Jest coverage for lib/monitoring.js module-load monitoring init and filter behavior. |
| packages/fxa-auth-server/lib/metricsCache.spec.ts | Jest migration for MetricsRedis cache behavior with Redis/TypeDI isolated via mocks. |
| packages/fxa-auth-server/lib/metrics/glean/index.spec.ts | Jest migration for Glean server-side event metrics and error logging behavior. |
| packages/fxa-auth-server/lib/metrics/events.spec.ts | Jest migration for metrics event emission behavior (activity/flow/route flow events). |
| packages/fxa-auth-server/lib/metrics/context.spec.ts | Jest migration for metrics context stash/get/gather/propagate/validate behaviors. |
| packages/fxa-auth-server/lib/metrics/amplitude.spec.ts | Jest migration for Amplitude event mapping/validation tests (with TypeDI involvement). |
| packages/fxa-auth-server/lib/log.spec.ts | Jest migration for logger initialization, amplitude validation handling, and notifications. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it('can be set up when sentry is not enabled', async () => { | ||
| let throws = false; | ||
| try { | ||
| await configureSentry(server, config); | ||
| } catch (err) { | ||
| throws = true; | ||
| } | ||
| expect(throws).toBe(false); | ||
| }); |
There was a problem hiding this comment.
The test "can be set up when sentry is not enabled" doesn’t actually disable Sentry: beforeEach always sets config.sentry.dsn to a non-empty value, so this test exercises the same enabled branch as the prior test. Set config.sentry.dsn to a falsy value for this test (or avoid setting it unconditionally in beforeEach) so the no-DSN path is covered.
| Container.set(StatsD, { increment: jest.fn() }); | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Helpers | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| const mocks = require('../../test/mocks'); | ||
| const { version } = require('../../package.json'); | ||
|
|
||
| const amplitudeModule = require('./amplitude'); | ||
|
|
||
| const mockAmplitudeConfig = { | ||
| schemaValidation: true, | ||
| rawEvents: false, | ||
| }; | ||
|
|
||
| const DAY = 1000 * 60 * 60 * 24; | ||
| const WEEK = DAY * 7; | ||
| const MONTH = DAY * 28; | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Tests | ||
| // | ||
| // NOTE: mocks.mockLog() returns sinon spies, so we access call data via the | ||
| // sinon API (.callCount, .args) rather than the jest mock API (.mock.calls). | ||
| // StatsD instances created with jest.fn() use the jest API. | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| describe('metrics/amplitude', () => { | ||
| afterEach(() => { | ||
| jest.restoreAllMocks(); | ||
| }); | ||
|
|
||
| afterAll(() => { | ||
| Container.reset(); | ||
| }); |
There was a problem hiding this comment.
This spec mutates the global TypeDI Container at module load (Container.set(StatsD, ...)) and only calls Container.reset() in afterAll. That can leak DI state into other Jest suites running in the same worker and make results order-dependent. Please move the Container.set into a beforeEach and call Container.reset() in afterEach (this is the pattern used in other specs, e.g. lib/account-events.spec.ts:56-58, lib/account-delete.spec.ts:156-159).
|
@nshirley I've also addressed feedback from copilot and reviewed this code (verified parity on assertions, tests and coverage). The biggest change was the added coverage to lib/monitoring.js, not sure but original ticket called out adding it but there was no tests for it. It was straightforward to add so I did. Welcome to give this a pass over as well. |
nshirley
left a comment
There was a problem hiding this comment.
Thanks for adding the new tests for monitoring!
Because
test/local/used sinon/proxyquire/chai patterns that are being replaced with Jest-native equivalentslib/monitoring.jshad zero test coverageThis pull request
.spec.tsfiles inlib/lib/monitoring.js(previously untested)jest.fn(),jest.spyOn(),jest.mock()) instead of sinonproxyquirewithjest.resetModules()+jest.doMock()for modules with load-time side effectsTest migration summary
test/local/metricsCache.jslib/metricsCache.spec.tstest/local/sentry.jslib/sentry.spec.tslib/monitoring.spec.tstest/local/log.jslib/log.spec.tstest/local/metrics/context.jslib/metrics/context.spec.tstest/local/metrics/amplitude.jslib/metrics/amplitude.spec.tstest/local/metrics/events.jslib/metrics/events.spec.tstest/local/metrics/glean.tslib/metrics/glean/index.spec.tsIssue
Closes: https://mozilla-hub.atlassian.net/browse/FXA-12562
Checklist
Other Information
Verification:
Key decisions:
jest.mock('@sentry/node')used forsentry.spec.tsbecause Sentry exports are frozen/non-configurable —jest.spyOncannot redefine themmonitoring.spec.tsextractsfilterSentryEventfrominitMonitoringcall args since it's not exportedevents.spec.tsuses sinon.callCount/.argsformetricsContextmethods sincemocks.mockMetricsContext()returns sinon spies.catch(() => {})on an intentionally-rejected promise to avoid Node 22 unhandled rejection crashes