Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added tests for email-sender.js #198

Merged
merged 14 commits into from Nov 15, 2019
Merged

Conversation

@neilong31
Copy link
Collaborator

neilong31 commented Nov 15, 2019

Fixes #82

Currently adds two testers for email-sender.js, which uses nodemailer to send out emails.

I still have to add the code for the test recipients to come in as a .env variable.

What has been tested:

  1. Testing that sendMessage is being called with the right parameters
  2. That the resolve that sendMessage returns gives us the recipient that we passed to it, since it should resolve the recipients that have been accepted
@neilong31 neilong31 requested review from RyanWils and varshannagarajan Nov 15, 2019
@neilong31 neilong31 force-pushed the neilong31:testEmail branch 3 times, most recently from 7c8baef to d6fac16 Nov 15, 2019
@neilong31 neilong31 force-pushed the neilong31:testEmail branch from 833d69c to 64b67af Nov 15, 2019
env.example Outdated Show resolved Hide resolved
src/backend/web/app.js Outdated Show resolved Hide resolved
@neilong31 neilong31 force-pushed the neilong31:testEmail branch from 6c8776b to 6b4c95e Nov 15, 2019
@RyanWils

This comment has been minimized.

Copy link
Contributor

RyanWils commented Nov 15, 2019

I think it's a good base to start with. Once thing I noticed us that you should add comments to your code so people can use it after you finish creating it. I think you merged improperly as well. The proper commands if you want to pull to the latest version is git pull upstream master when your on the master branch then go to your branch and git rebase master

@neilong31

This comment has been minimized.

Copy link
Collaborator Author

neilong31 commented Nov 15, 2019

Yeah I know, I messed up. After I pulled git pulled upstream master, I git rebased master, but I forgot to git checkout into my branch. Also, you bring up a fair point of adding comments. Thanks, I'll get on that!

@neilong31 neilong31 requested a review from manekenpix Nov 15, 2019
@jrdnlx jrdnlx closed this Nov 15, 2019
@jrdnlx jrdnlx reopened this Nov 15, 2019
@RyanWils

This comment has been minimized.

Copy link
Contributor

RyanWils commented Nov 15, 2019

Everyone looks good, maybe its better to group the .env variables together with the other nodemailer variables. You might want to put some instructions for creating test email (ethereal email). Is it not possible to fix the merges from the master? Its going to say you created/ touched alot of different files.

@RyanWils

This comment has been minimized.

Copy link
Contributor

RyanWils commented Nov 15, 2019

it seems like const transporter = nodemailer.createTransport('SMTP', { is an old format which is failing on the Travis Ci.
You can remove the 'SMTP to make it const transporter = nodemailer.createTransport({
This works the same way, was just trying to be more explicit
Not sure if we should leave it or change it once we get the SMTP credentials

@neilong31

This comment has been minimized.

Copy link
Collaborator Author

neilong31 commented Nov 15, 2019

@RyanWils Sounds good! I saw the PR you created. If the test is good as is for now do mind approving the PR? That way we can have the test when we test that new PR.

@RyanWils

This comment has been minimized.

Copy link
Contributor

RyanWils commented Nov 15, 2019

@neilong31 The bug fix was pushed, is it possible to grab the latest changes and see if this function has any errors?

@neilong31 neilong31 force-pushed the neilong31:testEmail branch from 335342a to a0c8e3a Nov 15, 2019
@RyanWils

This comment has been minimized.

Copy link
Contributor

RyanWils commented Nov 15, 2019

Okay thats fine, It cannot connect cause their no server yet. O wait you fixed the merged commits right?

Copy link
Contributor

RyanWils left a comment

Everything looks good

testHTML,
);
// Tests if the expected resolve is correct
expect(testReturnValue).resolves.toBe([testRecipient]);

This comment has been minimized.

Copy link
@varshannagarajan

varshannagarajan Nov 15, 2019

Contributor

This tests checks to see if it works given a good email, but we should also add a test with bad data to be more through. I will approve this PR but you should either add one later or create another issue for it.

Copy link
Contributor

varshannagarajan left a comment

See comment about adding other tests

@@ -5,7 +5,7 @@ const emailsender = require('../src/email-sender');
// and checks if it's resolve returns what is to be expected,
// which an array of the testRecipient since it returns
// an array of addresses it sent the email too
test('Tests if sendMessage resolves with expected info', async () => {
test.skip('Tests if sendMessage resolves with expected info', async () => {

This comment has been minimized.

Copy link
@KarlaChen

KarlaChen Nov 15, 2019

Contributor

why this test need to skip?

This comment has been minimized.

Copy link
@neilong31

neilong31 Nov 15, 2019

Author Collaborator

It will currently fail since we have no connection to the server, but the issue before has been fixed with #227

Copy link
Contributor

RyanWils left a comment

Looks good

@neilong31 neilong31 dismissed manekenpix’s stale review Nov 15, 2019

The changes has been made

@neilong31 neilong31 merged commit 5f052c5 into Seneca-CDOT:master Nov 15, 2019
2 checks passed
2 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@neilong31 neilong31 deleted the neilong31:testEmail branch Nov 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.