Skip to content
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

Add tests for ES256 #360

Merged
merged 1 commit into from Jun 14, 2017
Merged

Add tests for ES256 #360

merged 1 commit into from Jun 14, 2017

Conversation

ziluvatar
Copy link
Contributor

@ziluvatar ziluvatar commented Jun 11, 2017

It reuses same tests we have for RS256, for ES256 in this case.
It only wraps the main describe to run it once per asymmetric algorithm and replace 'RS256' by algorithm variable. I didn't add/remove any test internally.

I uploaded a x509 certificate as well, it is not used, but it can be useful in the future.

Closes #97

});
});

it('should be invalid', function (done) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In the same describe when signing a token with expiration there is quite contradictory cases like should be valid and should be invalid. Also, the describe creates a token, which is used in some cases and overwritten in other cases. Either, make separate describe blocks for when expiration has gone out etc, or make the case-description clearer. And decide if the token variable is for the whole describe or make one per case :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TBH I didn't check the tests internally, I just wanted to run the same ones for ES256 as well.
If you feel I should refactor the tests before applying these changes let me know, I can prepare a PR to refactor them + renaming and a following one with the wrapping for ES256.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, with unaltered tests but applied to more areas than before, it's a step in a good direction. Let's put test refactoring to a future endeavor.

@fiddur fiddur merged commit 2f36063 into auth0:master Jun 14, 2017
@ziluvatar ziluvatar deleted the add-ecdsa-tests branch June 14, 2017 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants