tests(lib/emails): Add tests for lib/email renderer and sender #19775

nshirley · 2025-12-09T21:41:36Z

Because

Moving/re-architecting email code to libs/ meant we needed new tests

This pull request

Adds tests for libs/accounts/email-renderer and libs/accounts/email-sender

Issue that this pull request solves

Closes: FXA-12579

Checklist

Put an x in the boxes that apply

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).

Screenshots (Optional)

Please attach the screenshots of the changes made in case of change in user interface.

Other information (Optional)

Any other information that is important to this pull request.

packages/fxa-auth-server/test/local/routes/account.js

nshirley · 2025-12-09T21:43:14Z

libs/accounts/email-sender/src/bounces.ts

    const now = Date.now();

+    // sort to descending order to guarantee latest bounces are processed first
+    bounces.sort((a, b) => b.createdAt - a.createdAt);


The original implementation had a note that there was an assumption that records come in order by createdAt, but to ensure we always process bounces in that way I figured it makes sense to explicitly order them here.

nshirley · 2025-12-09T21:43:28Z

libs/accounts/email-sender/src/bounces.ts

-const BOUNCE_TYPE_HARD = 1;
-const BOUNCE_TYPE_SOFT = 2;
-const BOUNCE_TYPE_COMPLAINT = 3;
+export const BOUNCE_TYPE_HARD = 1;


I wanted to use these in the test so it made sense to export them

nshirley · 2025-12-09T21:45:12Z

libs/accounts/email-sender/src/bounces.spec.ts

+      };
+      const bounces = new Bounces(config, db);
+
+      // internally, applyRules returns tallies, but it's not returned by check(),


I feel this comment has some more to it than I initially said - applyRules is also a private function so we're having to mock the db to also make this testable. Basically, core business logic is locked behind the private accessor and is difficult to test as a result. It would be nice to make applyRules public and/or a pure function as it would make testing a lot more straight forward.

I think that makes sense. Can we do that here?

Makes sense to me. Agreed, that testing it directly is better.

nshirley · 2025-12-09T21:47:58Z

libs/accounts/email-sender/src/bounces.spec.ts

+
+  beforeEach(() => {
+    // Mock Date.now() to return a consistent value for time-based tests
+    jest.spyOn(Date, 'now').mockReturnValue(mockNow);


I don't exactly like having to monkey patch this - we could get around/better test if the Bounces class had a systemClock: { now: () => number} parameter which was defaulted internally to Date. Kind of a nit-picky thing but would help make it clear that the module has a time component internally

I actually think this spy has caused problems before. I like the suggestion. Want to do that in a follow up?

nshirley · 2025-12-09T21:48:41Z

libs/accounts/email-renderer/project.json

-      "outputs": [
-        "{projectRoot}/public/en/auth.ftl"
-      ]
+      "inputs": ["{projectRoot}/gruntfile.js", "{projectRoot}/src/**/en.ftl"],


Formatting because I added the dependsOn and 'format on save' did it's thing. Let me know if I should revert this!

That's okay! I feel I've been seeing these formatting changes more and more recently... We should really get to the bottom of this as I team. I don't understand why this is happening, since we should all be using the same prettier configs and format on commit and format on save should essentially the same.

libs/accounts/email-renderer/src/renderer/email-renderer.ts

nshirley · 2025-12-09T23:10:26Z

libs/accounts/email-renderer/src/renderer/fxa-email-renderer.ts

    });
  }

+  renderNewDeviceLoginStrapi(


I think I was testing how strapi works with the new flow here - I can remove this if it doesn't need to be here yet

I think it's good to test this. It's a valid 'state' that can be rendered.

nshirley · 2025-12-10T00:14:16Z

libs/accounts/email-renderer/src/renderer/fxa-email-renderer.spec.ts

I left a comment in this, but also want to explicitly call this out - snapshots are going to be good long-term as it allows us to quickly identify nuanced, unintentional changes to email templates and partials.

However, this pattern of snapshots is not going to be fun long-term. We cannot specify a directory to save a snapshot (it's always relative to test file) and so, all of these get put into one massive 18k+ line snapshot file. In the future, it would be BEST to be able to upgrade Storybook (maybe we can just upgrade this package?) since it would allow us to leverage snapshots directly on the storybooks. Snapshots would be more focused at that point, and per template so they're easier to read.

Can you add this info to the upgrade storybook ticket? https://mozilla-hub.atlassian.net/browse/FXA-8765

libs/accounts/email-sender/src/bounces.spec.ts

nshirley · 2025-12-10T17:47:56Z

libs/accounts/email-renderer/src/renderer/__snapshots__/fxa-email-renderer.spec.ts.snap

The more I've thought about this, the less I like it. The only other "solution" I can think for some what manageable tests until we can upgrade storybook is to explicitly test for email subject, template, version, etc. But the biggest part we care about is the html and that would be difficult to test without explicit "for this template, we need to make sure this element exists with this body."

Which I suppose I could do, at least getting the groundwork down, then future updates and tests won't be so tedious... I'm open to suggestions!

dschom · 2025-12-12T00:13:35Z

libs/accounts/email-renderer/src/layouts/fxa/index.ts

  privacyUrl: string;

+  /** The current support url. */
+  supportUrl: string;


Are these referenced in the layout template? I'm not seeing them.

dschom · 2025-12-12T00:13:47Z

libs/accounts/email-renderer/src/renderer/email-renderer.ts


    return {
-      langauge: selectedLocale,
+      language: selectedLocale,


dschom · 2025-12-12T00:18:08Z

libs/accounts/email-renderer/src/templates/passwordResetAccountRecovery/index.txt

 <%- include('/partials/userInfo/index.txt') %>

-passwordResetAccountRecovery-regen-required-txt-1 = "We logged you out of all your synced devices. We created a new account recovery key to replace the one you used. You can change it in your account settings:"
+passwordResetAccountRecovery-information-txt = "We logged you out of all your synced devices. We created a new account recovery key to replace the one you used. You can change it in your account settings:"


Just curious, did this mistake get picked up by the tests?

dschom · 2025-12-12T00:22:43Z

libs/accounts/email-renderer/src/templates/postChangeTwoStepAuthentication/index.txt

@@ -1,11 +1,11 @@
-postChangeTwoStepAuthentication-title-2 = "Two-step authentication has been updated"


I find this suffixes a little suspicous... Do when know when/why they were added?

dschom · 2025-12-12T00:24:34Z

libs/accounts/email-renderer/project.json

      }
    },
    "test-unit": {
+      "dependsOn": ["l10n-merge"],


Thanks for spotting this.

dschom

These changes look great. I'm a big fan of the snapshot approach over the what we had before.

dschom · 2025-12-12T00:25:47Z

libs/accounts/email-sender/src/bounces.spec.ts

+
+  beforeEach(() => {
+    // Mock Date.now() to return a consistent value for time-based tests
+    jest.spyOn(Date, 'now').mockReturnValue(mockNow);


I actually think this spy has caused problems before. I like the suggestion. Want to do that in a follow up?

dschom · 2025-12-12T00:27:24Z

libs/accounts/email-sender/src/email-sender.spec.ts

+  let mockBounces: jest.Mocked<Bounces>;
+  let mockStatsd: jest.Mocked<StatsD>;
+  let mockLogger: jest.Mocked<ILogger>;
+  let mockTransport: any;


Can we give it a type of some sort?

dschom · 2025-12-12T00:41:34Z

libs/accounts/email-sender/src/bounces.spec.ts

+      };
+      const bounces = new Bounces(config, db);
+
+      // internally, applyRules returns tallies, but it's not returned by check(),


I think that makes sense. Can we do that here?

dschom · 2025-12-12T00:43:25Z

libs/accounts/email-sender/src/bounces.spec.ts

+      };
+      const bounces = new Bounces(config, db);
+
+      // internally, applyRules returns tallies, but it's not returned by check(),


Makes sense to me. Agreed, that testing it directly is better.

nshirley commented Dec 9, 2025

View reviewed changes

packages/fxa-auth-server/test/local/routes/account.js Show resolved Hide resolved

nshirley commented Dec 9, 2025

View reviewed changes

nshirley force-pushed the FXA-12579 branch 2 times, most recently from 52b34fb to 2bec955 Compare December 9, 2025 21:50

dschom reviewed Dec 9, 2025

View reviewed changes

libs/accounts/email-renderer/src/renderer/email-renderer.ts Outdated Show resolved Hide resolved

nshirley commented Dec 9, 2025

View reviewed changes

nshirley force-pushed the FXA-12579 branch from 2bec955 to 27429cb Compare December 9, 2025 23:12

nshirley commented Dec 10, 2025

View reviewed changes

libs/accounts/email-sender/src/bounces.spec.ts Show resolved Hide resolved

nshirley force-pushed the FXA-12579 branch from 27429cb to a7d7f70 Compare December 10, 2025 00:30

tests(lib/emails): Add tests for lib/email renderer

6c21f6f

nshirley force-pushed the FXA-12579 branch from a7d7f70 to 6c21f6f Compare December 10, 2025 00:33

nshirley commented Dec 10, 2025

View reviewed changes

nshirley marked this pull request as ready for review December 10, 2025 20:10

nshirley requested a review from a team as a code owner December 10, 2025 20:10

dschom reviewed Dec 12, 2025

View reviewed changes

libs/accounts/email-renderer/src/renderer/email-renderer.ts

return {

langauge: selectedLocale,

language: selectedLocale,

Copy link

Contributor

dschom Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

dschom reviewed Dec 12, 2025

View reviewed changes

libs/accounts/email-renderer/project.json

}

},

"test-unit": {

"dependsOn": ["l10n-merge"],

Copy link

Contributor

dschom Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for spotting this.

dschom approved these changes Dec 12, 2025

View reviewed changes

		@@ -1,11 +1,11 @@
		postChangeTwoStepAuthentication-title-2 = "Two-step authentication has been updated"

tests(lib/emails): Add tests for lib/email renderer and sender #19775

Are you sure you want to change the base?

tests(lib/emails): Add tests for lib/email renderer and sender #19775

Conversation

nshirley commented Dec 9, 2025

Because

This pull request

Issue that this pull request solves

Checklist

Screenshots (Optional)

Other information (Optional)

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nshirley Dec 10, 2025 • 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!

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 Dec 12, 2025 • 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 Dec 12, 2025 • 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nshirley Dec 10, 2025 •

edited

Loading

dschom Dec 12, 2025 •

edited

Loading

dschom Dec 12, 2025 •

edited

Loading