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.rejects passes if the given function throws synchronously #19646

Closed
not-an-aardvark opened this issue Mar 28, 2018 · 0 comments
Closed
Assignees
Labels
assert Issues and PRs related to the assert subsystem. confirmed-bug Issues with confirmed bugs.

Comments

@not-an-aardvark
Copy link
Contributor

  • Version: Built from source off of latest master, df62e69. (This API has not appeared in a release yet.)
  • Platform: macOS
  • Subsystem: assert

The following code prints assertion passed:

const assert = require('assert');

function functionThatThrows() {
  throw new Error();
}

assert.rejects(functionThatThrows).then(
  () => console.log('assertion passed'),
  () => console.log('assertion failed')
);

This is unexpected because I would expect assert.rejects to check that the given function returns a Promise which eventually rejects. In this case, no Promise is returned at all, so there is nothing that "rejects".

From an ergonomics perspective, if I were writing a function which could return a rejected Promise, I would want to always return a Promise to indicate errors, rather than sometimes throwing synchronously. If my function did throw synchronously rather than rejecting, this would be incorrect behavior and I would expect assert.rejects to report the error.

@not-an-aardvark not-an-aardvark added confirmed-bug Issues with confirmed bugs. assert Issues and PRs related to the assert subsystem. labels Mar 28, 2018
@not-an-aardvark not-an-aardvark self-assigned this Mar 28, 2018
not-an-aardvark added a commit to not-an-aardvark/node that referenced this issue Mar 28, 2018
This updates `assert.rejects()` to disallow any errors that are thrown
synchronously from the given function. Previously, throwing an error
would cause the same behavior as returning a rejected Promise.

Fixes: nodejs#19646
BridgeAR pushed a commit to BridgeAR/node that referenced this issue May 1, 2018
This updates `assert.rejects()` to disallow any errors that are thrown
synchronously from the given function. Previously, throwing an error
would cause the same behavior as returning a rejected Promise.

Fixes: nodejs#19646
PR-URL: nodejs#19650
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit to MylesBorins/node that referenced this issue Nov 2, 2018
This updates `assert.rejects()` to disallow any errors that are thrown
synchronously from the given function. Previously, throwing an error
would cause the same behavior as returning a rejected Promise.

Fixes: nodejs#19646
PR-URL: nodejs#19650
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this issue Nov 4, 2018
This updates `assert.rejects()` to disallow any errors that are thrown
synchronously from the given function. Previously, throwing an error
would cause the same behavior as returning a rejected Promise.

Fixes: #19646
Backport-PR-URL: #24019
PR-URL: #19650
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
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. confirmed-bug Issues with confirmed bugs.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant