test(auth-server): migrate script tests from Mocha to co-located Jest test#20294
test(auth-server): migrate script tests from Mocha to co-located Jest test#20294
Conversation
packages/fxa-auth-server/test/scripts/bulk-mailer.in.spec.ts
Dismissed
Show dismissed
Hide dismissed
packages/fxa-auth-server/test/scripts/bulk-mailer.in.spec.ts
Dismissed
Show dismissed
Hide dismissed
packages/fxa-auth-server/test/scripts/bulk-mailer.in.spec.ts
Dismissed
Show dismissed
Hide dismissed
packages/fxa-auth-server/test/scripts/bulk-mailer.in.spec.ts
Dismissed
Show dismissed
Hide dismissed
There was a problem hiding this comment.
Pull request overview
This PR migrates remaining fxa-auth-server script tests from Mocha to Jest, unblocking broader Jest adoption and aiming to reduce integration-test flakiness caused by shared DB usage.
Changes:
- Added co-located Jest unit specs under
scripts/**for script modules. - Added Jest integration specs under
test/scripts/**that exercise scripts via child-process execution. - Updated Jest config and npm scripts to pick up the new test locations/patterns.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/fxa-auth-server/test/scripts/verification-reminders.in.spec.ts | Adds Jest integration smoke test for scripts/verification-reminders. |
| packages/fxa-auth-server/test/scripts/update-subscriptions-to-new-plan.in.spec.ts | Adds Jest integration smoke test for scripts/update-subscriptions-to-new-plan. |
| packages/fxa-auth-server/test/scripts/stripe-products-and-plans-converter.in.spec.ts | Adds Jest integration coverage for Stripe→Firestore converter behavior and error paths. |
| packages/fxa-auth-server/test/scripts/recorded-future/check-and-reset.in.spec.ts | Adds Jest integration test for recorded-future check/reset script CLI defaults/args. |
| packages/fxa-auth-server/test/scripts/prune-tokens.in.spec.ts | Ports prune-tokens integration tests to Jest (DB/Redis assertions). |
| packages/fxa-auth-server/test/scripts/prune-oauth-authorization-codes.in.spec.ts | Adds Jest integration smoke tests for oauth auth-code pruning script. |
| packages/fxa-auth-server/test/scripts/must-reset.in.spec.ts | Ports must-reset integration tests to Jest (file inputs, argument validation). |
| packages/fxa-auth-server/test/scripts/move-customers-to-new-plan.in.spec.ts | Adds Jest integration smoke test for move-customers-to-new-plan script. |
| packages/fxa-auth-server/test/scripts/move-customers-to-new-plan-v2.in.spec.ts | Adds Jest integration smoke test for move-customers-to-new-plan-v2 script. |
| packages/fxa-auth-server/test/scripts/dump-users.in.spec.ts | Ports dump-users integration tests to Jest (input parsing & output JSON). |
| packages/fxa-auth-server/test/scripts/delete-unverified-accounts.in.spec.ts | Adds Jest integration tests for delete-unverified-accounts CLI validation and defaults. |
| packages/fxa-auth-server/test/scripts/delete-inactive-accounts/enqueue-inactive-account-deletions.in.spec.ts | Adds Jest integration tests for inactive-account deletion enqueuer CLI behavior. |
| packages/fxa-auth-server/test/scripts/delete-account.in.spec.ts | Adds Jest integration smoke test for delete-account script. |
| packages/fxa-auth-server/test/scripts/convert-customers-to-stripe-automatic-tax.in.spec.ts | Adds Jest integration smoke test for convert-customers-to-stripe-automatic-tax script. |
| packages/fxa-auth-server/test/scripts/check-users.in.spec.ts | Ports check-users integration tests to Jest (server + CSV IO). |
| packages/fxa-auth-server/test/scripts/bulk-mailer.in.spec.ts | Ports bulk-mailer integration tests to Jest (file output + stdout modes). |
| packages/fxa-auth-server/scripts/update-subscriptions-to-new-plan/update-subscriptions-to-new-plan.spec.ts | Adds Jest unit tests for SubscriptionUpdater behaviors. |
| packages/fxa-auth-server/scripts/stripe-products-and-plans-to-firestore-documents/plan-language-tags-guesser.spec.ts | Adds Jest unit tests for language-tag inference logic. |
| packages/fxa-auth-server/scripts/stripe-products-and-plans-to-firestore-documents/converter.spec.ts | Adds Jest unit tests for converter metadata transformations and file output. |
| packages/fxa-auth-server/scripts/recorded-future/lib.spec.ts | Adds Jest unit tests for recorded-future helper library functions. |
| packages/fxa-auth-server/scripts/move-customers-to-new-plan/move-customers-to-new-plan.spec.ts | Adds Jest unit tests for v1 customer plan mover logic. |
| packages/fxa-auth-server/scripts/move-customers-to-new-plan-v2/move-customers-to-new-plan-v2.spec.ts | Adds Jest unit tests for v2 plan mover + refund flows. |
| packages/fxa-auth-server/scripts/delete-inactive-accounts/lib.spec.ts | Adds Jest unit tests for delete-inactive-accounts helper library. |
| packages/fxa-auth-server/scripts/convert-customers-to-stripe-automatic-tax/helpers.spec.ts | Adds Jest unit tests for automatic-tax converter helpers. |
| packages/fxa-auth-server/scripts/convert-customers-to-stripe-automatic-tax/convert-customers-to-stripe-automatic-tax.spec.ts | Adds Jest unit tests for automatic-tax converter main workflow. |
| packages/fxa-auth-server/scripts/cleanup-old-carts/cleanup-old-carts.spec.ts | Adds Jest unit tests for cart cleanup script logic. |
| packages/fxa-auth-server/scripts/check-firestore-stripe-sync/check-firestore-stripe-sync.spec.ts | Adds Jest unit tests for Firestore/Stripe sync checker behavior. |
| packages/fxa-auth-server/scripts/cancel-subscriptions-to-plan/cancel-subscriptions-to-plan.spec.ts | Adds Jest unit tests for plan cancellation script flows. |
| packages/fxa-auth-server/package.json | Narrows Jest integration script selection via --testPathPattern. |
| packages/fxa-auth-server/jest.config.js | Expands testMatch to include scripts/**/*.spec.ts and adjusts transform ignore patterns. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| (await SessionToken.knexQuery().where({ uid }).count())[0]['count(*)']; | ||
|
|
||
| const deviceCount = async () => | ||
| (await Device.knexQuery().where({ uid }).count())[0]['count(*)']; |
There was a problem hiding this comment.
sessionCount/deviceCount return values from Knex count() are typically strings (e.g. '20'). These are later compared with toBe(size), which uses strict equality and will fail if the count is a string. Coerce the result to a number (e.g. Number(...)) before comparing, or use toEqual(size) after coercion.
| (await SessionToken.knexQuery().where({ uid }).count())[0]['count(*)']; | |
| const deviceCount = async () => | |
| (await Device.knexQuery().where({ uid }).count())[0]['count(*)']; | |
| Number( | |
| (await SessionToken.knexQuery().where({ uid }).count())[0]['count(*)'] | |
| ); | |
| const deviceCount = async () => | |
| Number( | |
| (await Device.knexQuery().where({ uid }).count())[0]['count(*)'] | |
| ); |
|
|
||
| const cwd = path.resolve(__dirname, '../..'); | ||
|
|
||
| // Usea really big number for max age. |
There was a problem hiding this comment.
Typo in comment: "Usea" → "Use a".
| // Usea really big number for max age. | |
| // Use a really big number for max age. |
| it('fails if --method missing', async () => { | ||
| try { | ||
| await execAsync( | ||
| 'node -r esbuild-register scripts/bulk-mailer --input ${USER_DUMP_PATH}', | ||
| execOptions | ||
| ); | ||
| throw new Error('script should have failed'); | ||
| } catch (err: any) { | ||
| expect(err.message).toContain('Command failed'); | ||
| } | ||
| }); | ||
|
|
||
| it('fails if --method is invalid', async () => { | ||
| try { | ||
| await execAsync( | ||
| 'node -r esbuild-register scripts/bulk-mailer --input ${USER_DUMP_PATH} --method doesNotExist', | ||
| execOptions | ||
| ); |
There was a problem hiding this comment.
These two tests are using single-quoted strings with ${USER_DUMP_PATH} inside, so the path is not interpolated. As written, the command will always use the literal ${USER_DUMP_PATH} and fail due to a missing file, which means the tests are not actually validating "--method missing" / "invalid method" behavior. Use a template string or string concatenation so the real USER_DUMP_PATH is passed.
There was a problem hiding this comment.
This is intentional since ${USER_DUMP_PATH} is invalid
| it('Throws an error when validation fails', async () => { | ||
| paymentConfigManager.validateProductConfig = sandbox | ||
| .stub() | ||
| .rejects(); | ||
| const spyWriteFile = sandbox | ||
| .stub(fs.promises, 'writeFile') | ||
| .resolves(); | ||
| try { | ||
| await converter.writeToFileProductConfig(); | ||
| sinon.assert.fail('An exception is expected to be thrown'); | ||
| } catch (err) { | ||
| sinon.assert.calledOnce( | ||
| paymentConfigManager.validateProductConfig | ||
| ); | ||
| sinon.assert.notCalled(spyWriteFile); | ||
| } |
There was a problem hiding this comment.
The test calls writeToFileProductConfig() with no arguments, but the implementation requires (productConfig, existingProductConfigId, productConfigPath, ...) and will likely throw before reaching the validation stub. Pass valid args and make validateProductConfig reject, or update the assertions to match the actual thrown error for missing args.
| it('Throws an error when validation fails', async () => { | ||
| paymentConfigManager.validatePlanConfig = sandbox.stub().rejects(); | ||
| const spyWriteFile = sandbox | ||
| .stub(fs.promises, 'writeFile') | ||
| .resolves(); | ||
|
|
||
| try { | ||
| await converter.writeToFilePlanConfig(); | ||
| sinon.assert.fail('An exception is expected to be thrown'); | ||
| } catch (err) { | ||
| sinon.assert.calledOnce(paymentConfigManager.validatePlanConfig); | ||
| sinon.assert.notCalled(spyWriteFile); | ||
| } |
There was a problem hiding this comment.
Same issue as above: writeToFilePlanConfig() is invoked with no arguments, but the implementation requires (planConfig, productConfigId, existingPlanConfigId, planConfigPath). This will throw before validatePlanConfig is called, so the test won't be exercising the intended failure path.
| expect(diff).toBeLessThanOrEqual(1000); | ||
|
|
||
| const startDateString = getOutputValue(outputLines, 'Start date'); | ||
| expect(startDateString!.startsWith('2012-03-12')).toBe(true); |
There was a problem hiding this comment.
Non-null assertion (startDateString!) violates the repo's @typescript-eslint/no-non-null-assertion rule. Prefer asserting the value is defined first (e.g. expect(startDateString).toBeDefined()) before using it, or handle the undefined case explicitly.
| expect(startDateString!.startsWith('2012-03-12')).toBe(true); | |
| expect(startDateString).toBeDefined(); | |
| expect(startDateString?.startsWith('2012-03-12')).toBe(true); |
| // Write the test accounts to a file that will be used to verify the script | ||
| let csvData = `${validClient.email}:${PASSWORD_VALID}\n`; | ||
| csvData = csvData + `${invalidClient.email}:wrong_password\n`; | ||
| csvData = csvData + `invalid@email.com:wrong_password\n`; | ||
| filename = `./test/scripts/fixtures/${Math.random()}_two_email_passwords.txt`; | ||
| fs.writeFileSync(filename, csvData); | ||
| }); | ||
|
|
||
| afterAll(async () => { | ||
| await server.stop(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Temporary fixture/output files created in beforeAll/the test case are never deleted (filename, outfile). This can pollute test/scripts/fixtures across runs and make local debugging harder. Add cleanup in afterAll/afterEach (and consider using path.join(__dirname, ...) for paths so the tests don't depend on the process CWD).
| }, | ||
| } as unknown as ConfigType; | ||
|
|
||
| describe('CustomerPlanMover', () => { |
There was a problem hiding this comment.
The top-level describe block name is "CustomerPlanMover", but this file is testing SubscriptionUpdater. Renaming the describe label to match the class under test will make failures and reports easier to interpret.
| describe('CustomerPlanMover', () => { | |
| describe('SubscriptionUpdater', () => { |
| }); | ||
|
|
||
| it('returns undefined when the plan language is en and detail is indentical to the product', async () => { | ||
| const actual = await getLanguageTagFromPlanMetadata( | ||
| plan, | ||
| supportedLanguages | ||
| ); | ||
| expect(actual).toBeUndefined(); | ||
| }); |
There was a problem hiding this comment.
Typo in test name: "indentical" → "identical".
| "test": "VERIFIER_VERSION=0 scripts/test-local.sh", | ||
| "test-jest": "jest --no-coverage --forceExit", | ||
| "test-jest-unit": "jest --no-coverage --forceExit", | ||
| "test-jest-integration": "jest --no-coverage --forceExit --config jest.integration.config.js", | ||
| "test-jest-integration": "jest --no-coverage --forceExit --config jest.integration.config.js --testPathPattern='\\.in\\.spec\\.ts$'", | ||
| "test-jest-ci": "JEST_JUNIT_OUTPUT_DIR='../../artifacts/tests/fxa-auth-server' JEST_JUNIT_OUTPUT_NAME='jest-results.xml' jest --coverage --forceExit --ci --reporters=default --reporters=jest-junit", |
There was a problem hiding this comment.
This script adds a Jest-only --testPathPattern filter, but the repo's default yarn test flow (scripts/test-local.sh) still runs Mocha test/scripts tests. Given the PR goal of migrating script tests off Mocha, consider updating the test runner scripts (or removing the old Mocha script tests) so the Jest migration is complete and script tests aren't duplicated/confusing.
There was a problem hiding this comment.
This is fine for now, gonna remove mocha soon
d3b7676 to
35c155e
Compare
35c155e to
43bd42d
Compare
43bd42d to
0c2af32
Compare
| }, | ||
| }; | ||
|
|
||
| jest.setTimeout(30000); |
There was a problem hiding this comment.
These timeouts were added from mocha, but I'm going to remove them and see how it goes.
Because
This pull request
.ts) to co-located Jest.spec.tsfiles underscripts/.js) to Jest.in.spec.tsfiles undertest/scripts/jest.config.jsto includescripts/intestMatchandpackage.jsonto add--testPathPatternfor integration testsCloses https://mozilla-hub.atlassian.net/browse/FXA-13337
Checklist
Integration tests
Unit tests
assert.failguards →throwassert.fail/assert.okguards removedassert.failguards →throwassert.failguards removedOther information
How to test: