Add unit tests for Nodemailer #184

SerpentBytes · 2023-02-12T04:48:14Z

Description

Resolves #147 by adding initial Unit Tests to test the functionality of Nodemailer. I had to adjust the sendNotification function for unit testing. Also, I have modified the tests script to require dotenv/config to read environment variables defined in .env.example.

Steps to Test

Setup the project locally (make sure Docker is running)
Run npm test (all tests should pass)
Navigate to the MailHog interface at http://localhost:8025/ to see the test notification:

.github/workflows/ci.yaml

app/lib/notifications.server.ts

Eakam1007

Looks good!

app/lib/notifications.server.ts

test/unit/notifications.server.test.ts

app/lib/notifications.server.ts

test/unit/notifications.server.test.ts

package.json

shawnyu5 · 2023-02-15T18:41:58Z

Please stop force pushing....

Eakam1007 · 2023-02-15T18:44:45Z

Please stop force pushing....

I think we don't need to squash and merge locally in general. It makes it hard to re-review changes.
When we merge PRs, we can make sure to Squash and Merge instead and keep the history on the PR

shawnyu5 · 2023-02-15T18:45:38Z

Exactly

test/unit/notifications.server.test.ts

Eakam1007

Looks good!

test/unit/notifications.server.test.ts

* Add unit tests for nodemailer * Make adjustments to Nodemailer module to work wiht Unit Tests * Add Nodemailer env vars to CI * Add nodemailer env vars to docker compose * Add env vars in right place in CI * Remove nodemailer env vars * Add dotenv to test script * Remove dotenv import * return a promise with resolve/reject * Use objectContaining/arrayContaining to read response * Add mailhog under services * Remove nested Promise * Use objectContaining/arrayContaining in all notification tests * Remove test log message * Remove await and extra catch * Add await and remove catch * Eliminate test dependency * Remove types where they are already inferred * Test against the whole item array * Assign value to messageId inside if block to prevent typecheck fail * remove exclamation mark after messageId * remove if block * use invariant to throw if sentMailResponse.messageId is undefined * Add blank lines for better readibility * Pass responseToJSON.items directly to expect * Add cross-env to test script * Remove invariant

test/unit/notifications.server.test.ts

Genne23v · 2023-02-15T17:06:45Z

test/unit/notifications.server.test.ts

+ const text = 'Sample text';
+
+ test('sending and receiving email notification using sendNotification()', async () => {
+ const sentMailResponse = await sendNotification(recipient, subject, text);


I have one question. Is this response always same when provided email does not exist?

Yes, even if the recipient is [email protected] the email would still get delivered using MailHog. We don't need real email addresses for testing.

I see. I was thinking we might need a fail case testing.

In production, we would expect it to fail. Here, we are just testing if are able to send/receive notifications.

Uhhh there is a distinction between production testing and dev testing?

I see. I was thinking we might need a fail case testing.

I could add that in a follow-up.

* Add unit tests for nodemailer * Make adjustments to Nodemailer module to work wiht Unit Tests * Add Nodemailer env vars to CI * Add nodemailer env vars to docker compose * Add env vars in right place in CI * Remove nodemailer env vars * Add dotenv to test script * Remove dotenv import * return a promise with resolve/reject * Use objectContaining/arrayContaining to read response * Add mailhog under services * Remove nested Promise * Use objectContaining/arrayContaining in all notification tests * Remove test log message * Remove await and extra catch * Add await and remove catch * Eliminate test dependency * Remove types where they are already inferred * Test against the whole item array * Assign value to messageId inside if block to prevent typecheck fail * remove exclamation mark after messageId * remove if block * use invariant to throw if sentMailResponse.messageId is undefined * Add blank lines for better readibility * Pass responseToJSON.items directly to expect * Add cross-env to test script * Remove invariant

SerpentBytes force-pushed the issue-126 branch 2 times, most recently from 56dcce2 to 1f2582d Compare February 12, 2023 04:57

SerpentBytes requested review from humphd, shawnyu5 and Eakam1007 February 12, 2023 05:00

SerpentBytes marked this pull request as ready for review February 12, 2023 05:00

SerpentBytes self-assigned this Feb 12, 2023

SerpentBytes added category: testing Unit tests, end to end tests category: notifications A service to notify users about their certificates/domains category: back end Back end part of our web service labels Feb 12, 2023

SerpentBytes added this to the Milestone 0.2 milestone Feb 12, 2023

Eakam1007 reviewed Feb 12, 2023

View reviewed changes

.github/workflows/ci.yaml Outdated Show resolved Hide resolved

Eakam1007 self-requested a review February 12, 2023 06:54

Eakam1007 reviewed Feb 12, 2023

View reviewed changes

app/lib/notifications.server.ts Outdated Show resolved Hide resolved

Eakam1007 self-requested a review February 12, 2023 08:04

Eakam1007 previously approved these changes Feb 12, 2023

View reviewed changes

humphd requested changes Feb 12, 2023

View reviewed changes

app/lib/notifications.server.ts Outdated Show resolved Hide resolved

app/lib/notifications.server.ts Outdated Show resolved Hide resolved

test/unit/notifications.server.test.ts Outdated Show resolved Hide resolved

SerpentBytes modified the milestones: Milestone 0.2, Milestone 0.3 Feb 12, 2023

SerpentBytes marked this pull request as draft February 12, 2023 17:10

SerpentBytes dismissed Eakam1007’s stale review via 38d2e22 February 12, 2023 22:44

SerpentBytes force-pushed the issue-126 branch 2 times, most recently from 38d2e22 to 7481522 Compare February 12, 2023 23:11

SerpentBytes requested a review from humphd February 12, 2023 23:14

SerpentBytes marked this pull request as ready for review February 12, 2023 23:15

humphd requested changes Feb 13, 2023

View reviewed changes

app/lib/notifications.server.ts Outdated Show resolved Hide resolved

SerpentBytes force-pushed the issue-126 branch from 7481522 to fbb6a57 Compare February 13, 2023 19:14

SerpentBytes requested a review from humphd February 13, 2023 19:15

Eakam1007 reviewed Feb 13, 2023

View reviewed changes

app/lib/notifications.server.ts Outdated Show resolved Hide resolved

SerpentBytes force-pushed the issue-126 branch from 7818a61 to 9b202bf Compare February 13, 2023 19:19

SerpentBytes marked this pull request as ready for review February 15, 2023 17:17

SerpentBytes force-pushed the issue-126 branch from 5abb3a6 to 8a5d8df Compare February 15, 2023 17:24

Eakam1007 requested changes Feb 15, 2023

View reviewed changes

test/unit/notifications.server.test.ts Outdated Show resolved Hide resolved

SerpentBytes force-pushed the issue-126 branch from 8a5d8df to 73689fd Compare February 15, 2023 17:53

SerpentBytes requested a review from Eakam1007 February 15, 2023 17:54

Eakam1007 reviewed Feb 15, 2023

View reviewed changes

test/unit/notifications.server.test.ts Outdated Show resolved Hide resolved

Eakam1007 reviewed Feb 15, 2023

View reviewed changes

package.json Outdated Show resolved Hide resolved

SerpentBytes force-pushed the issue-126 branch from 73689fd to 116d4f2 Compare February 15, 2023 18:41

SerpentBytes requested a review from Eakam1007 February 15, 2023 18:48

Eakam1007 requested changes Feb 15, 2023

View reviewed changes

test/unit/notifications.server.test.ts Outdated Show resolved Hide resolved

Eakam1007 self-requested a review February 15, 2023 19:01

shawnyu5 reviewed Feb 15, 2023

View reviewed changes

test/unit/notifications.server.test.ts Show resolved Hide resolved

Eakam1007 previously approved these changes Feb 15, 2023

View reviewed changes

humphd previously approved these changes Feb 15, 2023

View reviewed changes

test/unit/notifications.server.test.ts Show resolved Hide resolved

SerpentBytes dismissed stale reviews from humphd and Eakam1007 via 2539126 February 15, 2023 19:42

SerpentBytes force-pushed the issue-126 branch from 788a2f2 to 2539126 Compare February 15, 2023 19:42

Genne23v requested changes Feb 15, 2023

View reviewed changes

Genne23v approved these changes Feb 15, 2023

View reviewed changes

SerpentBytes requested review from humphd and Eakam1007 February 15, 2023 19:56

Eakam1007 approved these changes Feb 15, 2023

View reviewed changes

SerpentBytes merged commit dde1ede into DevelopingSpace:main Feb 15, 2023

SerpentBytes deleted the issue-126 branch February 15, 2023 20:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unit tests for Nodemailer #184

Add unit tests for Nodemailer #184

SerpentBytes commented Feb 12, 2023 •

edited

Loading

Eakam1007 left a comment

shawnyu5 commented Feb 15, 2023

Eakam1007 commented Feb 15, 2023

shawnyu5 commented Feb 15, 2023

Eakam1007 left a comment

Genne23v Feb 15, 2023

SerpentBytes Feb 15, 2023

Genne23v Feb 15, 2023

SerpentBytes Feb 15, 2023

shawnyu5 Feb 15, 2023

SerpentBytes Feb 15, 2023

Add unit tests for Nodemailer #184

Add unit tests for Nodemailer #184

Conversation

SerpentBytes commented Feb 12, 2023 • edited Loading

Description

Steps to Test

Eakam1007 left a comment

Choose a reason for hiding this comment

shawnyu5 commented Feb 15, 2023

Eakam1007 commented Feb 15, 2023

shawnyu5 commented Feb 15, 2023

Eakam1007 left a comment

Choose a reason for hiding this comment

Genne23v Feb 15, 2023

Choose a reason for hiding this comment

SerpentBytes Feb 15, 2023

Choose a reason for hiding this comment

Genne23v Feb 15, 2023

Choose a reason for hiding this comment

SerpentBytes Feb 15, 2023

Choose a reason for hiding this comment

shawnyu5 Feb 15, 2023

Choose a reason for hiding this comment

SerpentBytes Feb 15, 2023

Choose a reason for hiding this comment

SerpentBytes commented Feb 12, 2023 •

edited

Loading