assert: improve assert rejects #19885

BridgeAR · 2018-04-09T02:30:27Z

Commit 1 (assert: add direct promises support in rejects):

This adds direct promise support to `assert.rejects` and
`assert.doesNotReject`. It will now accept both, functions and ES2015
promises as input.

Besides this the documentation was updated to reflect the latest
changes.

It also refactors the tests to a non blocking way to improve the
execution performance and improves the coverage.

Commit 2 (tools: add eslintrc rule for assert.rejects):

This makes sure `assert.rejects` is always called with a second
argument. Besides that it is also changes a eslint error message
to suggest objects instead of a regular expression.

Commit 3 (doc: improve assert documentation):

This adds a example to `assert.throws` to document checking against
error instances. It also makes it clearer what some arguments can
receive as input.

Checklist

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

vsemozhetbyt

The doc LGTM with some nits)

vsemozhetbyt · 2018-04-09T10:29:52Z

.eslintrc.js

      {
        selector: `CallExpression[callee.object.name='assert'][callee.property.name='throws'][arguments.1.type='Literal']:not([arguments.1.regex])`,
-        message: 'use a regular expression for second argument of assert.throws()',
+        message: 'Use a object as second argument of assert.throws()',


Nit: a object -> an object.

vsemozhetbyt · 2018-04-09T10:45:09Z

doc/api/assert.md

-function, or an object where each property will be tested for.
+If specified, `error` can be a [`Class`][], [`RegExp`][], a validation function,
+an object where each property will be tested for or an instance of error where
+each property will be tested for including the non-enumerable message and name


message and name -> `message` and `name`?

vsemozhetbyt · 2018-04-09T10:47:48Z

doc/api/assert.md

  Error
 ).then(() => {
  // ...
 });
 ```

+Note that `error` can not be a string. If a string is provided as the second


can not -> cannot?

vsemozhetbyt · 2018-04-09T10:49:47Z

doc/api/assert.md

-function, or an object where each property will be tested for.
+If specified, `error` can be a [`Class`][], [`RegExp`][], a validation function,
+an object where each property will be tested for or an instance of error where
+each property will be tested for including the non-enumerable message and name


message and name -> `message` and `name`?

vsemozhetbyt · 2018-04-09T10:53:54Z

lib/.eslintrc.yaml

    - selector: "CallExpression[callee.object.name='assert'][callee.property.name='throws'][arguments.1.type='Literal']:not([arguments.1.regex])"
-      message: "use a regular expression for second argument of assert.throws()"
+      message: "Use a object as second argument of assert.throws()"


a object -> an object?

BridgeAR · 2018-04-09T20:47:29Z

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

BridgeAR · 2018-04-10T02:00:41Z

Rebased due to conflicts.

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

BridgeAR · 2018-04-10T02:01:26Z

@nodejs/testing @targos @not-an-aardvark @vsemozhetbyt @jasnell @joyeecheung @mcollina PTAL.

not-an-aardvark

I'm not really convinced that this is worthwhile -- it seems unnecessary to switch behavior based on the argument type when we could just use another method instead. (It also seems like the behavior isn't well-defined in edge cases where the value is both a thenable and a function simultaneously.) But I'm not strongly opposed if you think this is simpler.

not-an-aardvark · 2018-04-10T02:46:38Z

lib/assert.js

@@ -448,13 +448,24 @@ function getActual(block) {
  return NO_EXCEPTION_SENTINEL;
 }

+function checkIsPromise(obj) {
+  return isPromise(obj) ||


I would recommend just checking for the presence of a .then function here, without doing any of the other checks. That would make it compatible with Promise libraries and the Promise.resolve method and Promise libraries, as well as the mental model of "anything that can be await-ed to get a different value".

My intention was not to support Promises/A+ but ES6 promises. And if I read the spec correct, the check is fine as I implemented it. Only checking for obj !== null && typeof obj.then === 'function' would allow multiple false positives, so I would rather stick to the current check.

I can remove the isPromise check though as any real promise will also adhere to the later checks.

If you only want to support ES6 promises, why do you need the checks below?

Well, I do not only want to support native promises. Every implementation that is spec compliant should be supported out of my perspective. That is not the case for all thenables. I do not have a strong opinion on this though, so I can go for what ever is suggested. Only having the upper check is definitely not enough though. The very least that should also be checked for is typeof obj === 'object' || typeof obj === 'function and I would argue also `typeof obj.catch === 'function'. In that case we should be relatively safe.

The difference with that check to the current implementation would be to also accept thenables that use a function instead a object as Promise.

I'm not against the current check, but it would be nice to have a small comment explaining this in the code.

There are two categories of "promise-like" objects that have special behavior in the language: native promises and thenables. By doing extra checks (e.g. for a .catch method), we would effectively be adding a third category (the categories would be "native promises", "thenables", and "thenables which also have a .catch method") This seems like it would complicate the model by adding an arbitrary additional check, and it's not clear to me that there is a substantial benefit to adding a .catch check. So in my opinion, it would be slightly better to just detect thenables for API simplicity.

mcollina

Can you only include the changes to assert.rejects?

Using the big red cross because I'm -1 to make assert.throws aware of any async behavior.

BridgeAR · 2018-04-10T10:52:08Z

@mcollina oh, I had a wrong description, it was never about assert.throws. I force pushed and updated the PR description. PTAL

mcollina

LGTM with a nit

mcollina · 2018-04-10T10:55:18Z

doc/api/assert.md

-When `assert.rejects()` is called, it will immediately call the `block`
-function, and awaits for completion.
+If `block` is a function and it throws synchronously, that error will be
+rejected directly without checking the `error` handler.


what do you mean with this?

Synchronously thrown errors (due to the block() call) will directly cause a rejected promise to be returned (with that error). That is different in the way that a rejection caused by calling block() will trigger the potential error handler (second argument). If the rejected error does not adhere to the error handler, then it will cause a rejection. Otherwise not.

I do see that this is not well worded yet and it would be great to get a suggestion for improvement. Looping in @nodejs/documentation

Ping @vsemozhetbyt @Trott

I cannot think of anything more clear(

Maybe something like this?

If block is a function and it throws an error synchronously, assert.rejects will return a rejected Promise with that error without checking the error handler.

BridgeAR · 2018-04-10T12:10:07Z

@not-an-aardvark my intention was not to support thenables but only ES6 Promises. I personally think it is best this way but I do not feel that strongly about it. So if others think we should support thenables as well, I am fine to accept those as well.

BridgeAR · 2018-04-12T11:18:17Z

@targos you reviewed #19886 and were involved in a former discussion about adding support for passing in a promise directly. So it would be nice if you could have a look :-)

@TimothyGu I saw in a comment that you expected assert.rejects to accept promises directly, so it would also be nice if you could have a look :-).

This adds direct promise support to `assert.rejects` and `assert.doesNotReject`. It will now accept both, functions and ES2015 promises as input. Besides this the documentation was updated to reflect the latest changes. It also refactors the tests to a non blocking way to improve the execution performance and improves the coverage.

This makes sure `assert.rejects` is always called with a second argument. Besides that it is also changes a eslint error message to suggest objects instead of a regular expression.

This adds a example to `assert.throws` to document checking against error instances. It also makes it clearer what some arguments can receive as input.

BridgeAR · 2018-04-12T12:11:58Z

Rebased due to conflicts.

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

targos · 2018-04-12T13:18:02Z

lib/assert.js

+  let resultPromise;
+  if (typeof block === 'function') {
+    // Return a rejected promise if `block` throws synchronously.
+    resultPromise = block();


Should we check that block() effectively returns a Promise?

I have a follow up PR in #19886 that does exactly that :-)

BridgeAR · 2018-04-13T18:09:22Z

I updated the comments according to the suggestion and added a comment next to the promise check.

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

trivikr · 2018-04-14T07:30:29Z

doc/api/assert.md

 * `error` {RegExp|Function}
 * `message` {any}

-Awaits for the promise returned by function `block` to complete and not be
-rejected.
+Awaits the `block` promise or, if `block` is a function, immediately call the


nit: immediately calls the function and awaits the returned promise
Ditto on line 927

trivikr · 2018-04-14T07:34:03Z

doc/api/assert.md

-If specified, `error` can be a constructor, [`RegExp`][], a validation
-function, or an object where each property will be tested for.
+If specified, `error` can be a [`Class`][], [`RegExp`][], a validation function,
+an object where each property will be tested for or an instance of error where


nit: an object where each property will be tested for, or an instance
Ditto on line 1027

This adds direct promise support to `assert.rejects` and `assert.doesNotReject`. It will now accept both, functions and ES2015 promises as input. Besides this the documentation was updated to reflect the latest changes. It also refactors the tests to a non blocking way to improve the execution performance and improves the coverage. PR-URL: nodejs#19885 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

This makes sure `assert.rejects` is always called with a second argument. Besides that it is also changes a eslint error message to suggest objects instead of a regular expression. PR-URL: nodejs#19885 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

This adds a example to `assert.throws` to document checking against error instances. It also makes it clearer what some arguments can receive as input. PR-URL: nodejs#19885 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

BridgeAR · 2018-04-16T01:22:20Z

Landed in 11819c7...1d6cdfc

Nits addressed while landing.

This adds direct promise support to `assert.rejects` and `assert.doesNotReject`. It will now accept both, functions and ES2015 promises as input. Besides this the documentation was updated to reflect the latest changes. It also refactors the tests to a non blocking way to improve the execution performance and improves the coverage. PR-URL: #19885 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

This makes sure `assert.rejects` is always called with a second argument. Besides that it is also changes a eslint error message to suggest objects instead of a regular expression. PR-URL: #19885 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

This adds a example to `assert.throws` to document checking against error instances. It also makes it clearer what some arguments can receive as input. PR-URL: #19885 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

This adds direct promise support to `assert.rejects` and `assert.doesNotReject`. It will now accept both, functions and ES2015 promises as input. Besides this the documentation was updated to reflect the latest changes. It also refactors the tests to a non blocking way to improve the execution performance and improves the coverage. PR-URL: nodejs#19885 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

This makes sure `assert.rejects` is always called with a second argument. Besides that it is also changes a eslint error message to suggest objects instead of a regular expression. PR-URL: nodejs#19885 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

This adds a example to `assert.throws` to document checking against error instances. It also makes it clearer what some arguments can receive as input. PR-URL: nodejs#19885 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

BridgeAR requested review from targos, vsemozhetbyt and not-an-aardvark April 9, 2018 02:30

nodejs-github-bot added assert Issues and PRs related to the assert subsystem. tools Issues and PRs related to the tools directory. labels Apr 9, 2018

BridgeAR added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 9, 2018

BridgeAR mentioned this pull request Apr 9, 2018

assert: validate the block return type #19886

Closed

4 tasks

vsemozhetbyt reviewed Apr 9, 2018

View reviewed changes

BridgeAR force-pushed the improve-assert-rejects branch from 7789f2d to 85ef34a Compare April 10, 2018 02:00

not-an-aardvark reviewed Apr 10, 2018

View reviewed changes

mcollina requested changes Apr 10, 2018

View reviewed changes

BridgeAR force-pushed the improve-assert-rejects branch from 85ef34a to c7ef1b9 Compare April 10, 2018 10:50

mcollina approved these changes Apr 10, 2018

View reviewed changes

BridgeAR requested a review from TimothyGu April 12, 2018 11:18

BridgeAR added 3 commits April 12, 2018 14:10

tools: add eslintrc rule for assert.rejects

7ba5d61

This makes sure `assert.rejects` is always called with a second argument. Besides that it is also changes a eslint error message to suggest objects instead of a regular expression.

doc: improve assert documentation

fa6ac5f

This adds a example to `assert.throws` to document checking against error instances. It also makes it clearer what some arguments can receive as input.

BridgeAR force-pushed the improve-assert-rejects branch from c7ef1b9 to fa6ac5f Compare April 12, 2018 12:10

targos reviewed Apr 12, 2018

View reviewed changes

targos approved these changes Apr 12, 2018

View reviewed changes

jasnell approved these changes Apr 12, 2018

View reviewed changes

fixup: address comments

0e6e876

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 13, 2018

trivikr approved these changes Apr 14, 2018

View reviewed changes

BridgeAR closed this Apr 16, 2018

BridgeAR deleted the improve-assert-rejects branch April 1, 2019 23:40

BridgeAR restored the improve-assert-rejects branch April 1, 2019 23:40

BridgeAR deleted the improve-assert-rejects branch April 1, 2019 23:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

assert: improve assert rejects #19885

assert: improve assert rejects #19885

BridgeAR commented Apr 9, 2018 •

edited

Loading

vsemozhetbyt left a comment

vsemozhetbyt Apr 9, 2018

vsemozhetbyt Apr 9, 2018

vsemozhetbyt Apr 9, 2018

vsemozhetbyt Apr 9, 2018

vsemozhetbyt Apr 9, 2018

BridgeAR commented Apr 9, 2018

BridgeAR commented Apr 10, 2018

BridgeAR commented Apr 10, 2018

not-an-aardvark left a comment

not-an-aardvark Apr 10, 2018

BridgeAR Apr 10, 2018

targos Apr 12, 2018

BridgeAR Apr 12, 2018

targos Apr 12, 2018

not-an-aardvark Apr 14, 2018 •

edited

Loading

mcollina left a comment

BridgeAR commented Apr 10, 2018

mcollina left a comment

mcollina Apr 10, 2018

BridgeAR Apr 10, 2018

BridgeAR Apr 12, 2018

vsemozhetbyt Apr 12, 2018

not-an-aardvark Apr 12, 2018

BridgeAR commented Apr 10, 2018

BridgeAR commented Apr 12, 2018 •

edited

Loading

BridgeAR commented Apr 12, 2018

targos Apr 12, 2018

BridgeAR Apr 12, 2018

BridgeAR commented Apr 13, 2018

trivikr Apr 14, 2018

trivikr Apr 14, 2018

BridgeAR commented Apr 16, 2018

assert: improve assert rejects #19885

assert: improve assert rejects #19885

Conversation

BridgeAR commented Apr 9, 2018 • edited Loading

Checklist

vsemozhetbyt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Apr 9, 2018

BridgeAR commented Apr 10, 2018

BridgeAR commented Apr 10, 2018

not-an-aardvark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

not-an-aardvark Apr 14, 2018 • edited Loading

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

BridgeAR commented Apr 10, 2018

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Apr 10, 2018

BridgeAR commented Apr 12, 2018 • edited Loading

BridgeAR commented Apr 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Apr 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Apr 16, 2018

BridgeAR commented Apr 9, 2018 •

edited

Loading

not-an-aardvark Apr 14, 2018 •

edited

Loading

BridgeAR commented Apr 12, 2018 •

edited

Loading