assert: inconsistency in assert.doesNotThrow

[jasnell]

nodejs/node-v0.x-archive#6470 is an old PR that never landed but points to a valid issue.

The test case:

var assert = require('assert');
assert.doesNotThrow(function() {
  throw new Error();
});

Prints the error stack but does not output an AssertionError.

However,

var assert = require('assert');
assert.doesNotThrow(function() {
  throw new Error();
}, Error);

Raises an Assertion Error (AssertionError: Got unwanted exception (Error)..

var assert = require('assert');
assert.doesNotThrow(function() {
  throw 'custom message';
});

And

var assert = require('assert');
assert.doesNotThrow(function() {
  throw 'custom message';
}, 'custom message');

Both just output custom message without raising the Assertion Error.

The solution submitted in nodejs/node-v0.x-archive#6470 should be investigated as a possible fix but possibly needs to be revisited.

[jasnell]

Related: nodejs/node-v0.x-archive#6580

The docs show different signatures for assert.doesNotThrow and assert.throws but they use the same code under the covers. Need to determine for certain if assert.doesNotThrow is supposed to have an identical signature. /cc @trevnorris @cjihrig

[diversario]

Terribly sorry for the mess above!

[cjihrig]

@jasnell doesNotThrow() probably should not have the same signature, as throws() since once of the arguments to throws() is the expected error. It would be nice if the docs clarified the expected behavior of each function in more detail. The docs for throws() don't mention what message really does, so you're forced to look in the code. The docs for doesNotThrow() are even worse.

[diversario]

I'm working on updating the documentation but first I want clarify the behavior of throws/doesNotThrow. The current code (with the PR) throws an AssertionError in some cases but in three out of five cases. The other two are in the last if statement:

// if not the right exception or if did not expect an exception
// then rethrow
  if ((shouldThrow && actual && expected &&
      !expectedException(actual, expected)) || (!shouldThrow && actual)) {
    throw actual;
  }

and this simply rethrows the error from the block. That seems inconsistent and counter-intuitive as I, at least, would've expected assert to only throw AssertionError. These two also ignore message.

[foliveira]

#2807 tries to clarify the current behavior of the doesNotThrow function.

This is based on the test cases already implemented here and here and the method implementation in the assert.

[Trott]

The Assert API is now Locked. Should this be closed? Or is this a bug fix? Does it matter that it's a bug that does not seem to adversely affect Node.js project tests?

[andrewcharnley]

There's still a problem with it, if the second argument is not callable then it will always throw "Got unwanted exception.." instead of the actual exception, which contradicts the documentation which clearly states if the second argument is falsy it will throw the original exception. I currently resolve the bug with;

assert.doesNotThrow(() => somethingThatThrows(), () => null)

Documentation:

If an error is thrown and it is the same type as that specified by the error parameter, then an AssertionError is thrown. If the error is of a different type, or if the error parameter is undefined, the error is propagated back to the caller.

https://nodejs.org/dist/latest-v7.x/docs/api/assert.html#assert_assert_doesnotthrow_block_error_message

// tested with 7.10.0

[Trott]

This is fixed in current master. Closing. (Comment or re-open or open a new issue if I'm wrong.)