assert.throws is broken

[silkentrance]

See https://github.com/nodejs/node/blob/master/lib/assert.js#L305

Here, you test for !shouldThrow and expectedException(...).

In case of assert.throws() shouldThrow is always true and the above test will always fail, causing the instanceof test to never fail.

I think that the correct statement would be

if (shouldThrow && !expectedException(actual,expected))

[Trott]

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.

[cjihrig]

@silkentrance can you provide a failing test case?

[Trott]

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.

[silkentrance]

@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.

[Trott]

@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.

[silkentrance]

@Trott will come back to you once I am back at my dev machine.

[silkentrance]

@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.