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

Refactor sign validation tests #493

Closed
wants to merge 1 commit into from

Conversation

MitMaro
Copy link
Contributor

@MitMaro MitMaro commented Jun 15, 2018

This gives an idea of what I was thinking of for my proposal in #492. This would be the first of several PRs that I would like to create that would cleanup and complete the tests.

Description

The validation tests for the sign method were spread across multiple files, this change brings them together into a single file. Additional tests and improvements are also made to ensure that all the validation options are fully tested.

The validation tests for the sign method were spread across multiple
files, this change brings them together into a single file. Additional
tests and improvements are also made to ensure that all the validation
options are fully tested.
@MitMaro MitMaro force-pushed the sign-validation-test-refactor branch from 741e11f to 1848bd5 Compare June 15, 2018 14:23
@ziluvatar
Copy link
Contributor

One doubt that I have is: Imagine I want to add a new feature, that has a new option that has some validation and some other code to perform the business actions, with this approach I would need to write the validation test here (in the new file) and the business testing in the feature test file. Isn't it better to have tests by functionality? and inside of it all the needed tests: validation for its options, errors, outputs, etc?

Like expiration.tests.js, and it contains tests for signing + verifying + decode (or 3 files).
Like RS256.tests.js, and it contains tests for signing + verifying.

This way tests are self-contained by functionality.

Thoughts?

@MitMaro
Copy link
Contributor Author

MitMaro commented Jun 16, 2018

Understandable concerns, and I agree with the point. Let me try something that tackles it from the perspective of a use case. It might mean some minor overlap in the tests, but I don't think that is a bad thing in this case.

The tests in this PR should still be needed, it's just a question of the structure.

@MitMaro
Copy link
Contributor Author

MitMaro commented Jun 16, 2018

I'll reject this PR, and leave a comment on the issue for this proposal.

@MitMaro MitMaro closed this Jun 16, 2018
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.

2 participants