Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

assert: inconsistency in assert.doesNotThrow #2385

Closed
jasnell opened this issue Aug 15, 2015 · 8 comments
Closed

assert: inconsistency in assert.doesNotThrow #2385

jasnell opened this issue Aug 15, 2015 · 8 comments
Labels
assert Issues and PRs related to the assert subsystem. doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors.

Comments

@jasnell
Copy link
Member

jasnell commented Aug 15, 2015

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.

@mscdex mscdex added the assert Issues and PRs related to the assert subsystem. label Aug 15, 2015
@jasnell
Copy link
Member Author

jasnell commented Aug 16, 2015

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
Copy link

Terribly sorry for the mess above!

@cjihrig
Copy link
Contributor

cjihrig commented Aug 17, 2015

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

@Fishrock123 Fishrock123 added doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors. labels Aug 25, 2015
@diversario
Copy link

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
Copy link
Contributor

#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
Copy link
Member

Trott commented Oct 19, 2015

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?

@jasnell jasnell closed this as completed Apr 3, 2016
diversario pushed a commit to diversario/node that referenced this issue Apr 15, 2016
Addresses nodejs#2385.
Special handling to detect when user has supplied a custom message.
Added a test for user message.
When testing if `actual` value is an error use
`util.isError` instead of `instanceof`.
jasnell pushed a commit that referenced this issue Apr 18, 2016
Special handling to detect when user has supplied a custom message.
Added a test for user message.
When testing if `actual` value is an error use
`util.isError` instead of `instanceof`.

Fixes: #2385
PR-URL: #2407
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Apr 19, 2016
Special handling to detect when user has supplied a custom message.
Added a test for user message.
When testing if `actual` value is an error use
`util.isError` instead of `instanceof`.

Fixes: #2385
PR-URL: #2407
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Apr 20, 2016
Special handling to detect when user has supplied a custom message.
Added a test for user message.
When testing if `actual` value is an error use
`util.isError` instead of `instanceof`.

Fixes: #2385
PR-URL: #2407
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Apr 20, 2016
Special handling to detect when user has supplied a custom message.
Added a test for user message.
When testing if `actual` value is an error use
`util.isError` instead of `instanceof`.

Fixes: #2385
PR-URL: #2407
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Apr 20, 2016
Special handling to detect when user has supplied a custom message.
Added a test for user message.
When testing if `actual` value is an error use
`util.isError` instead of `instanceof`.

Fixes: #2385
PR-URL: #2407
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Apr 21, 2016
Special handling to detect when user has supplied a custom message.
Added a test for user message.
When testing if `actual` value is an error use
`util.isError` instead of `instanceof`.

Fixes: #2385
PR-URL: #2407
Reviewed-By: James M Snell <jasnell@gmail.com>
joelostrowski pushed a commit to joelostrowski/node that referenced this issue Apr 25, 2016
Special handling to detect when user has supplied a custom message.
Added a test for user message.
When testing if `actual` value is an error use
`util.isError` instead of `instanceof`.

Fixes: nodejs#2385
PR-URL: nodejs#2407
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this issue Apr 26, 2016
Special handling to detect when user has supplied a custom message.
Added a test for user message.
When testing if `actual` value is an error use
`util.isError` instead of `instanceof`.

Fixes: #2385
PR-URL: #2407
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue May 3, 2016
Special handling to detect when user has supplied a custom message.
Added a test for user message.
When testing if `actual` value is an error use
`util.isError` instead of `instanceof`.

Fixes: #2385
PR-URL: #2407
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue May 6, 2016
Special handling to detect when user has supplied a custom message.
Added a test for user message.
When testing if `actual` value is an error use
`util.isError` instead of `instanceof`.

Fixes: #2385
PR-URL: #2407
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue May 18, 2016
Special handling to detect when user has supplied a custom message.
Added a test for user message.
When testing if `actual` value is an error use
`util.isError` instead of `instanceof`.

Fixes: #2385
PR-URL: #2407
Reviewed-By: James M Snell <jasnell@gmail.com>
@andrewcharnley
Copy link

andrewcharnley commented May 19, 2017

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 Trott reopened this May 19, 2017
@Trott
Copy link
Member

Trott commented Aug 13, 2017

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

@Trott Trott closed this as completed Aug 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem. doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors.
Projects
None yet
Development

No branches or pull requests

8 participants