Refactor tests related to expiresIn and exp#501
Conversation
|
LGTM. However, not sure why the coverage is now lower than in master: This PR: Master: I took a look to the tests and they seemed to be fine. Can you take a look to the reports? About:
I'd say to keep the current function instead of adding another dependency. |
test/exp.test.js
Outdated
There was a problem hiding this comment.
I think for this is the same approach you already took in nbf, right?
There was a problem hiding this comment.
(sorry I forgot to publish this comment ^ )
There was a problem hiding this comment.
Yup, I meant to remove these for now, like I did in the other tests. I will update this evening.
There was a problem hiding this comment.
Updated in MitMaro@874bca2 before squash.
|
I will verify when I get home, but I believe the coverage drop is due to the removal of the |
|
Confirmed that the drop in coverage is from the removal of the |
fb405eb to
5dbb8fe
Compare
|
Fixed the drop in coverage in MitMaro@fb405eb before squash. |
This change extracts all tests in the current test files related to expiresIn and exp into a single test file. It also adds several missing tests.
5dbb8fe to
54bed49
Compare
|
Excellent work, ship it 👍 |
ziluvatar
left a comment
There was a problem hiding this comment.
My guess is that it was the only test that was testing the case of an invalid option being passed. If that is the case, I can re-add the test until another test is added for an invalid option being passed.
Yes, 100% is that. Let's do that plan.
Next PR for #492.
This change extracts all tests in the current test files related to
expiresInandexpinto a single test file. It also adds several missing tests.These tests are almost identical to the tests added in #497. I believe I removed most references to
expandexpiresInfrom the current tests. The remaining tests are not related to these tests.I extracted
base64UrlEncodeinto a separate file so it could be shared with the not before tests. This utility could also be easily replaced withbase64url. If this is desired, I will gladly swap it out.There was one test for a
expiresInSecondsoptions that was deprecated several versions ago. I didn't see the value in this test at this point, as it would be treated like any other invalid option, so it has been removed. I can re-add this test is desired.Any suggestions of missing tests are more than welcome, and I will gladly add!