Refactor tests related to notBefore and nbf (#492) #497
Conversation
package.json
Outdated
| @@ -4,7 +4,8 @@ | |||
| "description": "JSON Web Token implementation (symmetric and asymmetric)", | |||
| "main": "index.js", | |||
| "scripts": { | |||
| "test": "nyc --reporter=html --reporter=text mocha && nsp check && cost-of-modules" | |||
| "test:unit": "nyc --reporter=html --reporter=text mocha", | |||
There was a problem hiding this comment.
🤔 they are not unit tests. I guess you wanted a fast way to develop the tests getting coverage feedback without the delay of nsp + cost-of-modules, right? since this is test + coverage, let's name it: coverage or test:coverage or similar.
There was a problem hiding this comment.
Sure thing! cost-of-modules temporarily renames the node_modules/ directory, and it was causing my editor to continually re-index. This was a way to run the tests without those checks.
I'm okay with a coverage task.
There was a problem hiding this comment.
Fixed in MitMaro@f564d20 before I squashed commits.
test/nbf.test.js
Outdated
| Infinity, | ||
| NaN, | ||
| // TODO empty string currently fails | ||
| // '', |
There was a problem hiding this comment.
Let's create a test with the current behavior and a PR to fix this, since it should be invalid too (with same error message), and we can merge the fix on next major release.
There was a problem hiding this comment.
Just to make sure I understand correctly before I update:
- add a test for an empty string that checks for the
mserror - add another PR that fixes the behaviour
There was a problem hiding this comment.
Fixed in MitMaro@b037c26 before I squashed commits.
test/nbf.test.js
Outdated
| // TODO should these fail in validation? | ||
| // -Infinity, | ||
| // Infinity, | ||
| // NaN, |
There was a problem hiding this comment.
As you said, yes, I think they should fail, like I said before, let's add the current behavior and a PR (same one as before?) to fix in next major.
There was a problem hiding this comment.
Fixed in MitMaro@b037c26 before I squashed commits.
MitMaro
force-pushed
the
refactor-nbf-tests
branch
from
9179675 to
b55f2ab
Compare
This change extracts all tests in the current files related to notBefore and nbf into a single test file. It also adds several missing related tests.
MitMaro
force-pushed
the
refactor-nbf-tests
branch
from
b55f2ab to
e529ca3
Compare
|
I fixed some of the formatting in MitMaro@943dab9 after fixing the issues noted above. |
First PR for #492.
This change extracts all tests in the current files related to
notBeforeandnbfinto a single test file. It also adds several missing related tests.I believe I removed all references to
nbfandnotBeforefrom the current tests.There are two TODOs in the tests, because I found a few unhandled cases that I expected would cause an error but did not. They are:Passing an empty string tonotBefore. I'm not sure if this should default to0or should throw an error. Currently it throws aval is not a non-empty string or a valid number. val=""fromms.Passing-Infinity,InfinityandNaNtonbf. These allJSON.stringifytonull, so I would assume should be considered invalid.Any suggestions of missing tests are more than welcome, and I will gladly add!