-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
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
// TODO should these fail in validation? | ||
// -Infinity, | ||
// Infinity, | ||
// NaN, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for this is the same approach you already took in nbf, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(sorry I forgot to publish this comment ^ )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
expiresIn
andexp
into 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
exp
andexpiresIn
from the current tests. The remaining tests are not related to these tests.I extracted
base64UrlEncode
into 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
expiresInSeconds
options 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!