-
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 iat and maxAge #507
Conversation
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.
🤔 Should we add for each claim / verification option some tests to check that sync vs async call works fine and return an error in the expected way?
test/claim-iat.test.js
Outdated
const base64UrlEncode = testUtils.base64UrlEncode; | ||
const noneAlgorithmHeader = 'eyJhbGciOiJub25lIiwidHlwIjoiSldUIn0'; | ||
|
||
function signWithIssueAt(issueAt, options = {}) { |
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.
Node 4 and 5 do not support options = {}
.
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.
👍 Keep forgetting how new that is. That's an easy fix.
I was thinking about adding async tests when I did the different algorithms. But now that you mention it, there is value in ensuring that both the async and sync calls provide similar errors. |
I've been on vacation this past week, and haven't had time to take a look at this PR. Now that I'm back I should get a chance. I am going to see about adding an async test for each sync test. I will add it to this PR first, and then follow up with PRs for the other options. |
I've been thinking on adding async tests to the existing claim tests. I think I've come up with a solution that doesn't add a lot of complexity to each test. I will try to get this PR updated with the idea ASAP, and if it gets approved it should be easy to update the existing claim tests to follow the same format. |
dba684f
to
5e69a2f
Compare
This change extracts all tests related to the iat claim and the maxAge verify option into two test files. Several additional tests are added that were missing from the existing tests.
5e69a2f
to
86aefdb
Compare
I've squashed 5e69a2f into this branch. It makes every test use the synchronous call followed by and asynchronous call, using parameterized tests to avoid duplication in the test cases. If this pattern works for you, I will update the existing tests to follow the same pattern. |
@MitMaro I merged because I liked the approach :) , however, maybe we can think in some sort of utility function that wrap every
Or similar, anyway we don't need to do it now, let's see, because maybe it adds too much noise to the report without real benefit. |
For #492
This change extracts all tests related to the
iat
claim and themaxAge
verify option into two test files. Several additional tests are added that were missing from the existing tests.Most of the references to
iat
have been removed from the existing tests. The remaining references are not directly related to theiat
claim.Unlike the previous test pull requests, this pull request adds some validation tests for
maxAge
to a separate file, since the option isn't directly related to a claim.As always, suggestions or missing tests are welcome!
Coverage Master
Coverage Branch