Create and implement async/sync test helpers #523
Conversation
MitMaro
force-pushed
the
async-tests
branch
from
8255627
to
f669d27
Compare
|
Hey @ziluvatar , just a friendly bump. I know pull requests can get lost in the weeds on GitHub. |
test/claim-iat.test.js
Outdated
| expect(err.message).to.equal('"iat" should be a number of seconds'); | ||
| done(); | ||
| signWithIssueAt(iat, {}, (err) => { | ||
| // the sign function swallows the error that is thrown by expect, so use done(err) |
There was a problem hiding this comment.
Interesting, I didn't know about this, I guess it happens because of this:
Line 176 in 5498bdc
We might tackle that at some point, for now, it's fine your handling here.
There was a problem hiding this comment.
Actually I just checked our current async tests, if they fail they fail with error:
ensure "done()" is called (because as you said sign function swallows the throw error.
There was a problem hiding this comment.
Yeah, I assumed the existing tests would have the same issue. This is definitely something I would like to fix eventually, assuming someone else doesn't get to it first.
There was a problem hiding this comment.
After looking into this in further detail, this would also be a problem is #511 was every addressed.
Long term introducing Promises would be a possible solution as it would properly handle errors thrown in an asynchronous context.
MitMaro
force-pushed
the
async-tests
branch
2 times, most recently
from
fc7f484
to
731a76d
Compare
It is difficult to write tests that ensure that both the asynchronous and synchronous calls to the sign and verify functions had the same result. These helpers ensure that the calls are the same and return the common result. As a proof of concept, the iat claim tests have been updated to use the new helpers.
| expect(err.message).to.equal('"iat" should be a number of seconds'); | ||
| done(); | ||
| signWithIssueAt(iat, {}, (err) => { | ||
| testUtils.asyncCheck(done, () => { |
There was a problem hiding this comment.
It's a bit weird to use this, however, it is better than the try-catch, and more stable than simply... adding the asserts. So, good, let's keep this solution for now until we find a better one.
There was a problem hiding this comment.
Long term, promises solves this problem and that appears to be direction that the JavaScript/Node.js community is moving. But that would mean converting the project to not use callbacks, which would be well outside the scope of fixing the tests.
| function asyncCheck(done, testFunction) { | ||
| try { | ||
| testFunction(); | ||
| done(); |
There was a problem hiding this comment.
I guess we want this call (done()) outside of the try to avoid double called in case of error inside of it, right? (because we are passing the done from mocha)
There was a problem hiding this comment.
^ Silly thing, I'm also fine if you leave it as is, just let me know and I'll merge.
There was a problem hiding this comment.
Both options have different problems.
The way the code is currently means that if the done() call throws an error then done(err) will be called. This would be a double call to done which is generally considered a bad practice. In the case of Mocha, it will warn about the double call to done. Also, in this case anyways, for done() to fail, it would mean that Mocha is broken in some way.
By moving the done() call outside the try block then an error thrown by done() could be swallowed and not reported by Mocha or logged to the console.
I decided on what is there now, because I would rather Mocha tell the developer that something went wrong, then ignoring the error, or reporting that done was not called.
I think it's fine to keep things this way and try to fix the library later to avoid this problem altogether.
For #492
In #507 there was a comment about creating a utility function to help with testing both the asynchronous and synchronous calls to
signandverify. This is the concept that I came up with based on that comment. If this is accepted I will update the existing refactored tests to use the helpers.Coverage Master
Coverage Branch