Dropped exceptions in assert event handlers
Closed this issue · 17 comments
This is curious. When the handler for an 'assert' event throws an exception, tap-parser does not immediately throw an exception. Instead, it emits this exception in the next 'assert' event, naming the assertion as the toString() of the exception. tap-parser only itself throws an exception if this next call to the 'assert' handler again throws an exception. There is also at least one scenario where tap-parser neither propagates the exception nor emits 'complete'.
Case 1: The test contains a single passing assertion for which the 'assert' handler throws an exception. Result: tap-parser swallows the exception and quietly terminates without issuing a 'complete'.
Case 2: The test contains two passing assertions and the 'assert' handler throws an exception only on its first call. Result: tap-parser emits 'assert' again, setting assert.name to error.toString(). The 2nd assertion is never tested and tap-parser emits a 'complete'.
Case 3: The test contains two passing assertions and the 'assert' handler throws an exception on its 1st and 2nd call. Result: The 2nd 'assert' event has assert.name set to error.toString(), and tap-parser terminates with an exception, without emitting a 'complete'.
Case 4: The test contains a single failing assertion for which the 'assert' handler throws an exception. Result: tap-parser emits 'assert' again, setting assert.name to error.toString(). If there are any subsequent assertions, they are not run. tap-parser emits a 'complete'.
Case 5: The test contains a failing assertion and at least one following assertion. The first two calls to the 'assert' handler throw an exception. Result: The 2nd 'assert' event has assert.name set to error.toString(), and tap-parser terminates with an exception, without emitting a 'complete'. (Same result as case 3.)
It seems to me that if an event handler ever throws an exception, tap-parser should terminate with an exception. If the 'assert' event handler is expected to construct and throw an exception from the provided exception-generated assert object, we still have at least one scenario where the tap-parser quietly swallows the exception (case 1).
If the 'complete' event handler throws an exception, tap-parser also terminates throwing that exception. For consistency, it seems that the same should happen for 'assert'. Besides, if the 'assert' handler fails on its first call, it shouldn't be trusted to succeed on its next call.
Can you please clarify what input you're feeding into the parser, what output you're seeing out of it, and what you think it should do instead?
It's not clear to me what you mean with your comments about "throwing an exception". Do you mean that the event handler literally throws an exception? I really would love to get a failing test or something out of this.
Okay, adding tests now. But I'm laughing. I just noticed that you're using tape to test the fixtures, instead of tap. I originally built my tool on top of tape but found it too fragile a dependency. You're fine because you're using glob synchronously. tape does not properly wait for all tests to run before deciding that the run is done, making it unreliable with bin/tape.
ah, yeah, that's a relic. Updated in dc70415 to use tap everywhere instead, since it's functionally equivalent and just saves a dep.
When I say "add a test", I don't mean necessarily "add a test to this repo" if that's a challenge for you. Even just a gist that you can point to and say "this does X and I expect it to to Y" would be fine.
I've been working on a test I could PR, but it's looking like the only way to watch for the process throwing an exception is to catch the stderr output. It cannot be done with process.once('uncaughtException', cb). It was possible in earlier versions, but the node folks decided that there was no use case that merited suppressing exceptions.
What I'm saying is pretty simple. The following rightly causes the process to exit with an exception:
parser.on('comment', function (comment) {
throw new Error()
})The following does not cause the process to exit with an exception. Instead, what happens varies by circumstance, as I outlined above:
parser.on('assert', function (assert) {
throw new Error()
})I've created a set of tests for this problem. Let me know if/when you'd like a PR.
Belay that. I was expecting the 'comment' test to pass. Checking...
Eeep! All that work, I just proved that tap-parser is functioning properly. Not sure the tests are valuable, but they are passing. I wonder my problem is in tap. I'm piping tap to tap-parser.
I'm going to revise these test for tap piping to see what happens. Maybe they belong in tap instead.
Would an error thrown by tap-parser cause tap to output another not ok while piping? And only when the exception occurs during an 'assert' event? Investigating...
It has to be some sort of tap-tap-parser piping interaction problem. When I change my glob to synchronous, the second 'assert' event is no longer named for the prior exception and is instead labeled test unfinished.
I created a test suite for piping from tap to tap-parser, but the problem is not recreating in isolation from my tool. I'm closing this until I better understand what's going on.
That makes much more sense.
Because tap is a test runner (and not just a producer of TAP output), it's designed to work in some very hazardous situations and try to give you as much data as possible.
That means that if you throw an error in the context of a tap test, yes, it will catch it and fail the test. (This is required, for example, if you wish to use any of a variety of fancy assertion libs, and is how Mocha and many others work.)
If you explain what you're trying to do, I might be able to help guide you in the right direction.
Ah, I failed to consider that tap might have to try to report exceptions with additional assertions. I was hoping that it might be possible to separate pipe-related exceptions from exceptions thrown within the test code.
I decided to handle this by wrapping each of my tap-parser event handlers in a try-catch block. If an error occurs, I write the stack to stderr and process.exit(1).
This does eventually terminate the test, but it also results in numerous additional calls to the 'assert' handler. tap must listen for the beforeExit process event and emit additional failed assertions for tests that have not yet finished.
I wonder if it would be useful to design an error handling mechanism specifically for this situation. This would probably go in tap though. One way to do it would be to provide a file path found in all tap-parser-thrown exceptions and have tap not try to report these.
So here's how I'm dealing with it. All my tap-parser event handlers looks like this:
parser.on('some-event', function (data) {
if (this._terminated)
return;
try {
/* handle the event */
}
catch (err) {
this._exitWithError(err);
}
})MyClass.prototype._exitWithError = function (err) {
this._terminated = true;
process.stderr.write("\n"+ err.stack +"\n");
process.exit(1);
};You know, this behavior could be built into tap-parser and optionally turned on.