Goodparts refactor -- resolves #66 #69

newswim · 2017-02-07T07:33:59Z

Just one hanging point of review for me. On line 57, i wasn't totally sure about the {returns} value for JSDoc. Next time before tackling something like this, I should read over the source code (and readme) fully. If someone doesn't mind double checking me 🏁 --- that would make you the best 🏆

…odparts-javascript-code-style-

newswim · 2017-02-07T07:39:22Z

Current issue with Travis comes from that same error, TEMPLATE_DIRECTORY not being set. I resolved that locally, which leads to an error in one of the tests. Pursuing fix...

nelsonic · 2017-02-07T07:55:03Z

@Shouston3 would you mind peer-reviewing this PR? (please/thanks!)
(if you can help @newswim make the tests pass it would be ace!)

samhstn · 2017-02-07T08:56:48Z

@newswim You are now throwing an Error object here instead of a string.

So you should look to handle an object rather than a string in the tests here and in the other try catch statements

Instead of looking at e.indexOf, look at e.message

newswim · 2017-02-07T09:15:53Z

Did you want tests resolved here or in a different pr? This is only meant to address #66

…

On Feb 7, 2017 2:56 AM, "Sam Houston" ***@***.***> wrote: @newswim <https://github.com/newswim> You are now throwing an Error object here <https://github.com/dwyl/sendemail/pull/69/files#diff-6d186b954a58d5bb740f73d84fe39073R19> instead of a string. So you should look to handle an object rather than a string in the tests here <https://github.com/dwyl/sendemail/blob/master/test/index.test.js#L22> and in all of the other try catch statments — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#69 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGW7WFExTl5lLPPDs6QJ8vMKB0irqY5jks5raDHRgaJpZM4L5J6g> .

samhstn · 2017-02-07T09:19:45Z

The only way to address the failing tests and throw an error object is to change how we're handling the error in the tests

So I'd say change the tests just in regards to reading e.message instead of e.indexOf and leave the rest of the test file.

newswim · 2017-02-07T09:19:59Z

Ah, reread your message, Sam. I'll resolve this tomorrow. Thank you for looking!

newswim · 2017-02-07T17:57:02Z

Almost there, I'm not sure about these last two tests. I'll go ahead and push what I've done. Is it normal with tape to get a error Command failed with exit code 1. after all the tests run?

samhstn · 2017-02-07T18:00:30Z

No it should be exit code 0

newswim · 2017-02-07T18:01:05Z

I just realized I'm not testing the type param. This seemed to be causing the tests to fail, whereas hardcoding them passed:


=============================== Coverage summary ===============================
Statements   : 96.67% ( 29/30 )
Branches     : 90% ( 9/10 )
Functions    : 100% ( 4/4 )
Lines        : 96.67% ( 29/30 )
================================================================================

samhstn · 2017-02-07T18:03:03Z

Note the compile_template function needs two arguments.
You are currently passing one and that could be causing the breakage

samhstn · 2017-02-07T18:08:31Z

Also, I think the email function should have the template name as the first argument and I think you've passed in a filename

newswim · 2017-02-07T18:08:54Z

hey @Shouston3, thanks for looking at this.

I had changed that because I was getting this error:

Error: ENOENT: no such file or directory, open './examples/templates/hello'

This is probably going to take some reworking.

newswim · 2017-02-07T18:17:21Z

Got it, filepath was getting declared wrong.

newswim · 2017-02-07T18:27:08Z

🆘

I'm not sure how to test this, and I have a dentist appt in 15 minutes. I'm not sure how to handle this returned callback, please halp 😬

…rage)

newswim · 2017-02-07T18:42:25Z

Looks like an env variable is still not getting set. Sorry to have to leave it like this, be back in a few hours.

newswim · 2017-02-07T21:06:08Z

Well.. a little further. There was an error, and I guess I need to get aws keys now.

ses.sendEmail :: err: { InvalidClientTokenId: The security token included in the request is invalid.

newswim · 2017-02-07T21:42:45Z

I believe this is now a matter of the env variables within the Travis machine not getting set.

see: https://travis-ci.org/dwyl/sendemail/builds/198932105#L126
compared with: https://travis-ci.org/dwyl/sendemail/builds/199391820

codecov-io · 2017-02-07T22:15:24Z

Codecov Report

Merging #69 into master will not impact coverage by -3.34%.

@@            Coverage Diff             @@
##           master      #69      +/-   ##
==========================================
- Coverage     100%   96.66%   -3.34%     
==========================================
  Files           1        1              
  Lines          29       30       +1     
==========================================
  Hits           29       29              
- Misses          0        1       +1

Impacted Files	Coverage Δ
lib/index.js	`96.66% <100%> (-3.34%)`	❌

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 62f7ad4...9924704. Read the comment docs.

newswim · 2017-02-07T22:17:17Z

Hmm.. my guess is that that last test isn't passing because of the other environment variables.

I just realized it's late in the day for you guys, so I'll leave this and wait until it has time for review tomorrow. I would also really like to know how environment variables are typically set with Travis (it's my first time using it). I wonder if this has something to do with:

Encrypted environment variables are not available to pull requests from forks due to the security risk of exposing such information to unknown code.

https://docs.travis-ci.com/user/environment-variables/#Defining-public-variables-in-.travis.yml

samhstn · 2017-02-07T23:59:24Z

@newswim Nice find, I think 'encrypted environment variables not available to forks' seems reasonable, since my tests were passing locally.

I think what you added to the travis.yml file makes sense

nelsonic · 2017-02-08T00:04:29Z

@newswim you should have access to push your branch to this repo
and then open a new pull request to test that theory... 👍

samhstn · 2017-02-08T00:07:11Z

test/index.test.js

  })
+  t.end();


Same thing as below here.
You should leave the t.end inside of the email function

samhstn · 2017-02-08T00:07:13Z

test/index.test.js

  })
+  t.end()


You shouldn't put the t.end callback outside of the email function.
It will cause it to run immediately and not wait to test what is returned from the email function.

samhstn

@newswim I'd say invoke the t.end inside the email callback from the comments above and remove the global environment variable from travis.yml, then I think it's good to merge 👍

newswim · 2017-02-08T17:09:47Z

Thanks Sam, I'll initiate a new PR from the this repo rather than my fork. Feel free to close 🔚

samhstn · 2017-02-08T17:24:51Z

Closing see #70 instead

newswim added 3 commits February 7, 2017 01:17

add goodparts -- fixes 66

d88fdfd

typo in package.json

b4654f6

adds goodparts repo badge see: https://github.com/dwyl/repo-badges#go…

a08c670

…odparts-javascript-code-style-

newswim changed the title ~~Goodparts refactor -- fixes #66~~ Goodparts refactor -- fixes 66 Feb 7, 2017

newswim changed the title ~~Goodparts refactor -- fixes 66~~ Goodparts refactor -- resolves #66 Feb 7, 2017

nelsonic requested a review from samhstn February 7, 2017 07:55

Fixing tests, add npm-debug.log to .gitignore

31b2fea

Fixes filepath name, all tests now pass (1 function missing code cove…

2e4f62b

…rage)

all tests pass

d377164

try setting environment variable in travis

9924704

samhstn added the enhancement label Feb 7, 2017

samhstn added the in-review label Feb 7, 2017

samhstn reviewed Feb 8, 2017

View reviewed changes

test/index.test.js Outdated

})

t.end();

Copy link

Member

samhstn Feb 8, 2017

Choose a reason for hiding this comment

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

Same thing as below here.
You should leave the t.end inside of the email function

samhstn reviewed Feb 8, 2017

View reviewed changes

samhstn requested changes Feb 8, 2017

View reviewed changes

move t.end calls back inside function body, remove travis env variables

6046d44

samhstn closed this Feb 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Goodparts refactor -- resolves #66 #69

Goodparts refactor -- resolves #66 #69

newswim commented Feb 7, 2017

newswim commented Feb 7, 2017

nelsonic commented Feb 7, 2017 •

edited

Loading

samhstn commented Feb 7, 2017 •

edited

Loading

newswim commented Feb 7, 2017 via email

samhstn commented Feb 7, 2017 •

edited

Loading

newswim commented Feb 7, 2017

newswim commented Feb 7, 2017

samhstn commented Feb 7, 2017

newswim commented Feb 7, 2017 •

edited

Loading

samhstn commented Feb 7, 2017 •

edited

Loading

samhstn commented Feb 7, 2017 •

edited

Loading

newswim commented Feb 7, 2017

newswim commented Feb 7, 2017

newswim commented Feb 7, 2017

newswim commented Feb 7, 2017

newswim commented Feb 7, 2017

newswim commented Feb 7, 2017

codecov-io commented Feb 7, 2017

newswim commented Feb 7, 2017

samhstn commented Feb 7, 2017 •

edited

Loading

nelsonic commented Feb 8, 2017

samhstn Feb 8, 2017

samhstn Feb 8, 2017

samhstn left a comment •

edited

Loading

newswim commented Feb 8, 2017

samhstn commented Feb 8, 2017

Goodparts refactor -- resolves #66 #69

Goodparts refactor -- resolves #66 #69

Conversation

newswim commented Feb 7, 2017

newswim commented Feb 7, 2017

nelsonic commented Feb 7, 2017 • edited Loading

samhstn commented Feb 7, 2017 • edited Loading

newswim commented Feb 7, 2017 via email

samhstn commented Feb 7, 2017 • edited Loading

newswim commented Feb 7, 2017

newswim commented Feb 7, 2017

samhstn commented Feb 7, 2017

newswim commented Feb 7, 2017 • edited Loading

samhstn commented Feb 7, 2017 • edited Loading

samhstn commented Feb 7, 2017 • edited Loading

newswim commented Feb 7, 2017

newswim commented Feb 7, 2017

newswim commented Feb 7, 2017

newswim commented Feb 7, 2017

newswim commented Feb 7, 2017

newswim commented Feb 7, 2017

codecov-io commented Feb 7, 2017

Codecov Report

newswim commented Feb 7, 2017

samhstn commented Feb 7, 2017 • edited Loading

nelsonic commented Feb 8, 2017

samhstn Feb 8, 2017

Choose a reason for hiding this comment

samhstn Feb 8, 2017

Choose a reason for hiding this comment

samhstn left a comment • edited Loading

Choose a reason for hiding this comment

newswim commented Feb 8, 2017

samhstn commented Feb 8, 2017

nelsonic commented Feb 7, 2017 •

edited

Loading

samhstn commented Feb 7, 2017 •

edited

Loading

samhstn commented Feb 7, 2017 •

edited

Loading

newswim commented Feb 7, 2017 •

edited

Loading

samhstn commented Feb 7, 2017 •

edited

Loading

samhstn commented Feb 7, 2017 •

edited

Loading

samhstn commented Feb 7, 2017 •

edited

Loading

samhstn left a comment •

edited

Loading