cul-2016 / quiz

11 stars 4 forks source link

free plan #632

Closed sohilpandya closed 6 years ago

sohilpandya commented 6 years ago

related to #621

codecov-io commented 6 years ago

Codecov Report

Merging #632 into staging will decrease coverage by 0.02%. The diff coverage is 89.47%.

Impacted file tree graph

@@             Coverage Diff             @@
##           staging     #632      +/-   ##
===========================================
- Coverage    94.19%   94.17%   -0.03%     
===========================================
  Files          124      124              
  Lines         2791     2796       +5     
  Branches       481      483       +2     
===========================================
+ Hits          2629     2633       +4     
- Misses         162      163       +1
Impacted Files Coverage Δ
server/lib/authentication/saveUser.js 100% <100%> (ø) :arrow_up:
server/plugins/authenticate-user.js 95.74% <100%> (+0.18%) :arrow_up:
server/plugins/strategy.js 88.88% <81.81%> (-4.87%) :arrow_down:

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 c32f445...b3325de. Read the comment docs.

sohilpandya commented 6 years ago

@Danwhy we need some help, for some reason it is not covering this line https://codecov.io/gh/cul-2016/quiz/compare/c32f445f36c37ecc2315c656cd25f8bb0ffff94d...b3325de3b03112479baa60c80c0c62565b872d2b/src/server/plugins/strategy.js#L10

We are definitely doing tests on it here https://github.com/cul-2016/quiz/pull/632/commits/f0f640b37493ea4e419890630ce7f7017c40eb42

Any ideas what could cause this line to not be covered. I've put a console log inside that block and it is definitely getting inside that block when running the test. :(

sohilpandya commented 6 years ago

Otherwise this PR is ready 😢

sohilpandya commented 6 years ago

@Danwhy we thought we'd fixed it in #633 but luck still, I'd rather you look at that version as we've worked on the test a little more.

Still, for some reason, it gets in the if statement but doesn't run the callback.

Danwhy commented 6 years ago

@sohilpandya As far as I can see, on this branch, the token is invalid, that's why it's returning a 401 and the test is passing.

In the changes you've made in #633, the user is not getting logged in at all because the trial has expired, so again that clause isn't being called.

It's quite a difficult condition to test, the only thing I can think to suggest is to log the user in with a valid trial time, then maybe mock the Date.now function to return a date in the future where the trial will have expired.

I'll merge this branch for now, we can look at fixing this in a future pr