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 tests related to expiresIn and exp #501

Merged
merged 1 commit into from
Jul 6, 2018

Conversation

MitMaro
Copy link
Contributor

@MitMaro MitMaro commented Jun 28, 2018

Next PR for #492.

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.

These tests are almost identical to the tests added in #497. I believe I removed most references to exp and expiresIn 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 with base64url. 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!

@ziluvatar
Copy link
Contributor

LGTM. However, not sure why the coverage is now lower than in master:

This PR:

===============================
Statements   : 97.02% ( 228/235 )
Branches     : 96.57% ( 197/204 )
Functions    : 100% ( 23/23 )
Lines        : 97.41% ( 226/232 )
===============================

Master:

===============================
Statements   : 97.45% ( 229/235 )
Branches     : 97.06% ( 198/204 )
Functions    : 100% ( 23/23 )
Lines        : 97.84% ( 227/232 )
===============================

I took a look to the tests and they seemed to be fine. Can you take a look to the reports?

About:

This utility could also be easily replaced with base64url. If this is desired, I will gladly swap it out.

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,
Copy link
Contributor

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?

Copy link
Contributor

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 ^ )

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

@MitMaro
Copy link
Contributor Author

MitMaro commented Jul 3, 2018

I will verify when I get home, but I believe the coverage drop is due to the removal of the expiresInSeconds test. 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.

@MitMaro
Copy link
Contributor Author

MitMaro commented Jul 3, 2018

Confirmed that the drop in coverage is from the removal of the expiresInSeconds test. I will re-add that test for now.

@MitMaro
Copy link
Contributor Author

MitMaro commented Jul 4, 2018

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.
@JacoKoster
Copy link
Contributor

Excellent work, ship it 👍

Copy link
Contributor

@ziluvatar ziluvatar left a 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.

@ziluvatar ziluvatar merged commit 72f0d9e into auth0:master Jul 6, 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.

3 participants