assert.throws is broken #2989

Activity

1. Trott commented on Sep 21, 2015

   

   Trott

   on Sep 21, 2015

   Member

   More actions

   shouldThrow will be false in that line of code if the user invokes assert.doesNotThrow() rather than assert.throws(). The condition is true for one test in test/parallel/node-assert.js, the code in the block executes, and things happen as expected.

   I don't think this is a bug.
2. brendanashworth

   added

   assertIssues and PRs related to the assert subsystem.Issues and PRs related to the assert subsystem.

   on Sep 21, 2015
3. cjihrig commented on Sep 22, 2015

   

   cjihrig

   on Sep 22, 2015

   Contributor

   More actions

   @silkentrance can you provide a failing test case?
4. Trott commented on Sep 22, 2015

   

   Trott

   on Sep 22, 2015

   Member

   More actions

   The condition being checked in that statement is: "If we are expecting the code not to throw, and the type of exception we are not expecting was specified, and we get that type of exception that we are expecting not to be thrown, then fail with AssertionError." That all seems correct to me.

   Observations:

   • If the type of exception that is expected to not be thrown isn't specified by the user, then the error is re-thrown. This would seem to be the typical case when using doesNotThrow() because...
   • ...the fact that you can specify the exception type with doesNotThrow() is not documented at the current time. See https://github.com/nodejs/node/blob/master/doc/api/assert.markdown#assertdoesnotthrowblock-message
   • This probably means that in the typical usage, doesNotThrow() is basically doing nothing. If the code throws, it just re-throws the error in most cases. It seems that only if you use the (undocumented) error function argument will you get an AssertionError. Fortunately, TypeError etc. will blow up a test just as well as AssertionError, so that all works out. But still, that's kind of peculiar. (EDIT: And upon further reflection, it is far more useful to get the actual error type and stack trace for the error you need to fix rather than an AssertionError that masks the real error type and a stack trace for that AssertionError. In other words, this "basically doing nothing" is a feature, not a bug.)
   • The condition we're talking about here is tested by this block in test/parallel/node-assert.js.

   There is a documentation bug, as the method signature should be assert.doesNotThrow(block[, error][, message]) but is currently documented as assert.doesNotThrow(block[, message]). I'll get a PR in to fix that.
5. silkentrance commented on Sep 22, 2015

   

   silkentrance

   on Sep 22, 2015

   Author

   More actions

   @Trott it is about assert.throws and not assert.doesNotThrow. The latter works fine, it is assert.throws that fails to run its instanceof test, at least on my machine.

   @cjihrig I will prepare a test case for that. The test cases I have all use custom checkers to see whether the correct exception was thrown. Without them, assert.throws will always succeed, reporting false positives as it will not apply the instanceof test.
6. Trott commented on Sep 22, 2015

   

   Trott

   on Sep 22, 2015

   Member

   More actions

   @silkentrance Do the comments below not accurately describe the behavior you are seeing?

   var assert = require('assert');
   
   // this assertion passes so nothing happens, hooray!
   assert.throws(function () {throw new TypeError();}, TypeError);
   
   // this assertion fails because we throw `TypeError` but expect `RangeError`.
   // The assertion library does not throw `AssertionError` here because it
   // would mask the real problem. Instead, it rethrows the `TypeError` so you
   // get a useful stack trace telling you where things went wrong.
   assert.throws(function () {throw new TypeError();}, RangeError);
   

   The relevant check for assert.throws() matching the instance of the actual error against the expected error happens a few lines after the code you're talking about.
7. silkentrance commented on Sep 22, 2015

   

   silkentrance

   on Sep 22, 2015

   Author

   More actions

   @Trott will come back to you once I am back at my dev machine.
8. silkentrance commented on Sep 23, 2015

   

   silkentrance

   on Sep 23, 2015

   Author

   More actions

   @Trott @cjihrig Well, it seems that I messed things up with my mocha test cases and test subjects. And, having read through the existing code and failing to understand it properly, leaves me with the notion that the assert.throws/doesNotThrow is still broken, but in a different way than originally described. Will open a new issue as soon as I get my head around that "utterly complex" code.
9. silkentrance

   closed this as completedon Sep 23, 2015