assert: ensure .rejects() disallows sync throws #19650

not-an-aardvark · 2018-03-28T05:36:19Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

This PR has two commits; the first commit fixes an issue with the assert tests, and the second commit fixes #19646.

First commit:

test: ensure failed assertions cause build to fail

This updates the test in `test/parallel/test-assert-async.js` to add an
assertion that the Promises used in the test end up fulfilled.
Previously, if an assertion failure occurred, the Promises would have
rejected and a warning would have been logged, but the test would still
have exit code 0.

Second commit:

assert: ensure .rejects() disallows sync throws

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: https://github.com/nodejs/node/issues/19646

Note that the second commit would be semver-major, but it modifies an API (introduced in #18023) that has not been backported to a release branch yet. As a result, the second commit could be backported as long as it's applied at the same time as #18023.

BridgeAR

LGTM

BridgeAR · 2018-03-28T08:06:27Z

test/parallel/test-assert-async.js

+    common.mustNotCall(),
+    common.mustCall((err) => {
+      assert.strictEqual(err, THROWN_ERROR);
+    })


Suggestion: use catch(common.mustCall(err) => { assert.strictEqual(err, THROWN_ERROR); })

BridgeAR · 2018-03-28T08:07:43Z

test/parallel/test-assert-async.js

  common.expectsError({
    code: 'ERR_ASSERTION',
    type: assert.AssertionError,
    message: 'Failed'
  })
-);
+).then(common.mustCall());


Nit: this test should now be redundant if I am not mistaken.

I'm assuming you mean that this test is redundant with the other test here. However, this test is a bit different from the other one because it throws asynchronously, and so the promise returned by assert.rejects fulfills (whereas in the other test, the function throws synchronously and the promise returned by assert.rejects rejects and needs to be caught separately).

Actually, I just realized what you meant: this test is now redundant with this other test. I agree, I'll remove the other one.

edit: Removed.

BridgeAR · 2018-03-28T08:08:10Z

test/parallel/test-assert-async.js

@@ -29,7 +29,7 @@ assert.doesNotReject(() => {});
    type: assert.AssertionError,
    message: 'Failed'
  }));
-  assert.doesNotReject(() => promise);
+  assert.doesNotReject(() => promise).then(common.mustCall());


Nit: use common.crashOnUnhandledRejection() at the top of the file instead of adding then(common.mustCall()) everywhere.

targos · 2018-03-28T09:12:45Z

@BridgeAR about .then(common.mustCall()):

It is necessary to make sure the promise eventually fulfills (does not stay forever in a pending state).

I think the best would be to combine common.crashOnUnhandledRejection() at the top and an async IIFE:

common.crashOnUnhandledRejection();

(async () => {
  {
    // test 1
    await assert.rejects(something);
  }

  {
    // test 2
    await assert.rejects(somethingElse);
  }
})().then(common.mustCall());

We should maybe add something like common.runAsync(async () => {}) that does both?

BridgeAR · 2018-03-28T09:50:19Z

@targos fair point. But should the test not time out in case the promise does not fulfill?

TimothyGu · 2018-03-28T16:03:08Z

@BridgeAR Here's a case where the .then(common.mustCall()) is absolutely necessary:

// Never fulfills.
const prom = new Promise(() => {});

(async () => {
  await prom;
  console.log('Never printed');
})();

The process does not stay alive since promise states are not part of the event loop.

not-an-aardvark · 2018-03-28T18:08:13Z

Updated to follow the suggestions in #19650 (comment) and #19650 (comment).

This updates the test in `test/parallel/test-assert-async.js` to add an assertion that the Promises used in the test end up fulfilled. Previously, if an assertion failure occurred, the Promises would have rejected and a warning would have been logged, but the test would still have exit code 0.

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

not-an-aardvark · 2018-03-28T21:21:57Z

CI: https://ci.nodejs.org/job/node-test-pull-request/13906/

BridgeAR · 2018-03-29T11:30:19Z

@TimothyGu thanks for the heads up!

@not-an-aardvark to have feature parity, it might make sense to check in assert.doesNotReject that the passed in function will actually return a promise. From that point on the function has some legitimacy. Right now I do not see any legit use case. What do you think?

targos · 2018-03-29T11:37:06Z

Just thinking about it now: wouldn't it be better if assert.rejects expected a Promise instead of a function?

BridgeAR · 2018-03-29T11:40:17Z

@targos I agree. That solves that dilemma. Since we did not yet publish the functionality, we can still change it. [update] Thinking about it again: It might still be better to only handle functions:

We use functions in assert.throws and assert.doesNotThrow
It has the benefit to test a specific functionality to really return a promise

I am going to give it another thought when I have some more time later on.

targos · 2018-03-29T11:58:44Z

In favor of changing it: existing assertion libraries that I know (Should.js, Chai as Promised, jest) work directly with promises.

not-an-aardvark · 2018-03-29T18:20:02Z

Arguably, this is different from assert.throws because assert.throws makes an assertion about how a function behaves, whereas assert.rejects only makes an assertion about the function's return value. So it would make sense for it to just accept a Promise rather than a function.

On the other hand, one potential argument for the current assert.rejects behavior is that it could be simpler for users who don't know how Promises work. They might have a mental model of "always use await when calling an async function to avoid weird behavior". With that mental model, assert.rejects(someAsyncFunction) would make sense, but assert.rejects(someAsyncFunction()) would not make sense. They might try to use assert.rejects(await someAsyncFunction()) instead, but this would cause other problems. The current behavior of assert.rejects would allow for a mental model of "this is just like assert.throws, but for async functions."

not-an-aardvark · 2018-04-03T00:55:10Z

@BridgeAR @targos What would you recommend doing with this PR? If the behavior of assert.rejects is going to be changed to accept Promises rather than functions, then I'm fine with closing this. However, if we don't do anything, then it seems likely that the current behavior will end up landing in a release, after which it will be much harder to change. (I haven't landed this PR yet because I'm not sure if you wanted to change the API signature instead.)

BridgeAR · 2018-04-03T08:45:37Z

I am actually considering supporting to accept both: passing in a function that returns a promise or directly passing in a promise. That would ease the user experience. I do not really see a downside accepting both.

not-an-aardvark · 2018-04-04T01:43:39Z

I'm going to land this since we seem to be in agreement that it's an improvement over the current behavior, but I'd be fine with the behavior being changed in a future PR.

not-an-aardvark · 2018-04-04T02:48:09Z

Landed in 8995125 and fdb35d8

This updates the test in `test/parallel/test-assert-async.js` to add an assertion that the Promises used in the test end up fulfilled. Previously, if an assertion failure occurred, the Promises would have rejected and a warning would have been logged, but the test would still have exit code 0. PR-URL: #19650 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

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 PR-URL: #19650 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

This updates the test in `test/parallel/test-assert-async.js` to add an assertion that the Promises used in the test end up fulfilled. Previously, if an assertion failure occurred, the Promises would have rejected and a warning would have been logged, but the test would still have exit code 0. PR-URL: nodejs#19650 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

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>

This updates the test in `test/parallel/test-assert-async.js` to add an assertion that the Promises used in the test end up fulfilled. Previously, if an assertion failure occurred, the Promises would have rejected and a warning would have been logged, but the test would still have exit code 0. PR-URL: nodejs#19650 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

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>

This updates the test in `test/parallel/test-assert-async.js` to add an assertion that the Promises used in the test end up fulfilled. Previously, if an assertion failure occurred, the Promises would have rejected and a warning would have been logged, but the test would still have exit code 0. Backport-PR-URL: #24019 PR-URL: #19650 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

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>

nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label Mar 28, 2018

BridgeAR approved these changes Mar 28, 2018

View reviewed changes

jasnell approved these changes Mar 28, 2018

View reviewed changes

not-an-aardvark force-pushed the assert-rejects-sync-error branch from 3749412 to edeb64c Compare March 28, 2018 18:06

not-an-aardvark added 2 commits March 28, 2018 14:15

assert: ensure .rejects() disallows sync throws

75d2d1e

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

not-an-aardvark force-pushed the assert-rejects-sync-error branch from edeb64c to 75d2d1e Compare March 28, 2018 18:16

not-an-aardvark closed this Apr 4, 2018

not-an-aardvark deleted the assert-rejects-sync-error branch April 4, 2018 02:48

targos added the backport-requested-v9.x label Apr 4, 2018

not-an-aardvark mentioned this pull request Apr 4, 2018

assert: adds rejects() and doesNotReject() #18023

Closed

4 tasks

MylesBorins added the dont-land-on-v8.x label Aug 7, 2018

not-an-aardvark mentioned this pull request Nov 1, 2018

[v8.x] backport assert: add rejects() and doesNotReject() #24019

Closed

4 tasks

MylesBorins added backported-to-v8.x and removed dont-land-on-v8.x labels Nov 2, 2018

BethGriggs mentioned this pull request Nov 12, 2018

v8.13.0 proposal #23974

Merged

beevelop mentioned this pull request Nov 20, 2018

2018-11-20 Version 8.13.0 'Carbon' (LTS) @MylesBorins prepared by @BethGriggs beevelop/docker-android-nodejs#250

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

assert: ensure .rejects() disallows sync throws #19650

assert: ensure .rejects() disallows sync throws #19650

not-an-aardvark commented Mar 28, 2018

BridgeAR left a comment

BridgeAR Mar 28, 2018

BridgeAR Mar 28, 2018

not-an-aardvark Mar 28, 2018

not-an-aardvark Mar 28, 2018 •

edited

Loading

BridgeAR Mar 28, 2018

targos commented Mar 28, 2018

BridgeAR commented Mar 28, 2018

TimothyGu commented Mar 28, 2018 •

edited

Loading

not-an-aardvark commented Mar 28, 2018

not-an-aardvark commented Mar 28, 2018

BridgeAR commented Mar 29, 2018

targos commented Mar 29, 2018

BridgeAR commented Mar 29, 2018 •

edited

Loading

targos commented Mar 29, 2018

not-an-aardvark commented Mar 29, 2018

not-an-aardvark commented Apr 3, 2018

BridgeAR commented Apr 3, 2018

not-an-aardvark commented Apr 4, 2018

not-an-aardvark commented Apr 4, 2018

assert: ensure .rejects() disallows sync throws #19650

assert: ensure .rejects() disallows sync throws #19650

Conversation

not-an-aardvark commented Mar 28, 2018

Checklist

BridgeAR left a comment

Choose a reason for hiding this comment

BridgeAR Mar 28, 2018

Choose a reason for hiding this comment

BridgeAR Mar 28, 2018

Choose a reason for hiding this comment

not-an-aardvark Mar 28, 2018

Choose a reason for hiding this comment

not-an-aardvark Mar 28, 2018 • edited Loading

Choose a reason for hiding this comment

BridgeAR Mar 28, 2018

Choose a reason for hiding this comment

targos commented Mar 28, 2018

BridgeAR commented Mar 28, 2018

TimothyGu commented Mar 28, 2018 • edited Loading

not-an-aardvark commented Mar 28, 2018

not-an-aardvark commented Mar 28, 2018

BridgeAR commented Mar 29, 2018

targos commented Mar 29, 2018

BridgeAR commented Mar 29, 2018 • edited Loading

targos commented Mar 29, 2018

not-an-aardvark commented Mar 29, 2018

not-an-aardvark commented Apr 3, 2018

BridgeAR commented Apr 3, 2018

not-an-aardvark commented Apr 4, 2018

not-an-aardvark commented Apr 4, 2018

not-an-aardvark Mar 28, 2018 •

edited

Loading

TimothyGu commented Mar 28, 2018 •

edited

Loading

BridgeAR commented Mar 29, 2018 •

edited

Loading