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: adds rejects() and doesNotReject() #18023

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
9 participants
@feugy
Contributor

feugy commented Jan 7, 2018

Implement async equivalent of assert.throws() and assert.doesNotThrow().

This PR is a follow-up of #17843, but takes into account James' suggestion to bring this as experimental feature, without change the existing API.

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
Affected core subsystem(s)

assert

// Asynchronous equivalent of original assert functions.
// Only throws() and doNotThrows() are truely async (all other perform
// synchronous tasks and don't use functions as parameters), but for
// conveniency, the entire API is supported here as well

This comment has been minimized.

@joyeecheung

joyeecheung Jan 9, 2018

Member

Nit: s/conveniency/convenience/?

This comment has been minimized.

@feugy

feugy Jan 10, 2018

Contributor

Thank you 😅

@@ -306,10 +301,38 @@ assert.doesNotThrow = function doesNotThrow(block, error, message) {
expected: error,
operator: 'doesNotThrow',
message: `Got unwanted exception${details}\n${actual.message}`,
stackStartFn: doesNotThrow
stackStartFn: assert.doesNotThrow

This comment has been minimized.

@joyeecheung

joyeecheung Jan 9, 2018

Member

Maybe name assert.doesNotThrow as a top level function and reference it directly here to avoid being messed up by monkey-patched assert.doesNotThrow

@@ -281,16 +274,18 @@ assert.throws = function throws(block, error, message) {
expected: error,
operator: 'throws',
message: `Missing expected exception${details}`,
stackStartFn: throws
stackStartFn: assert.throws

This comment has been minimized.

@joyeecheung

joyeecheung Jan 9, 2018

Member

Maybe name assert.throws as a top level function and reference it directly here to avoid being messed up by monkey-patched assert.throws

This comment has been minimized.

@feugy

feugy Jan 10, 2018

Contributor

👌

assert.ok(...args);
}
Object.assign(promises, assert, {

This comment has been minimized.

@joyeecheung

joyeecheung Jan 9, 2018

Member

Since other assigned APIs do not return promises here:

require('assert').promises.fail().then(() => {}); // does not work

This behavior looks somewhat strange to me..

This comment has been minimized.

@BridgeAR

BridgeAR Jan 9, 2018

Member

To me as well. I would rather have a dedicated assert.rejects function. Reject is used for promises, so this is a good fit out of my perspective.

This comment has been minimized.

@feugy

feugy Jan 10, 2018

Contributor

That's an interesting point.

My intention was to keep the existing behaviour for all assert functions except throws() and doesNotThrow(), which are the only two able to run (potentially async) code.
I wouldn't like to force people writing:

const assert = require('assert').promises

await assert(something)

But we can definitely add new functions like reject() that would be async equivalent of existing one.
Are there any other function you would like to cover?

This comment has been minimized.

@joyeecheung

joyeecheung Jan 10, 2018

Member

If we are not talking about redesigning a new assert API here, providing a promisified version of the existing assert APIs is good enough to me. Something like

for (const key of Object.keys(assert)) {
  const syncFunc = assert[key];
  promises[key] = async function (...args) { return syncFunc(...args) };
}
// override throws later, or in the block

This comment has been minimized.

@feugy

feugy Jan 11, 2018

Contributor

hum, but it's actually what I'd like to avoid.

There's no point making assert.ok() or assert.fail() asynchronous. It will make the API more cumbersome to use with no benefit.

It make sense for assert.throws() and assert.doesNotThrow() to turn this code (that we used quite a lot in our projects):

it('should report database option errors', async () => {
  try {
    await transferData({}, '');
    throw new Error('it should have failed');
  } catch (err) {
    assert(err.message.includes('"host" is required'));
  }
});

into

it('should report database option errors', async () =>
  assert.throws(async () => transferData({}, ''), /^Error: "host" is required/)
);

I don't want to redesign the whole API, but rather wants to support the above.

@@ -750,7 +750,8 @@ common.expectsError(
assert.equal(assert.deepEqual, assert.deepStrictEqual);
assert.equal(assert.notEqual, assert.notStrictEqual);
assert.equal(assert.notDeepEqual, assert.notDeepStrictEqual);
assert.equal(Object.keys(assert).length, Object.keys(a).length);
// strict doesn't expose promises

This comment has been minimized.

@BridgeAR

BridgeAR Jan 9, 2018

Member

Why not?

This comment has been minimized.

@feugy

feugy Jan 10, 2018

Contributor

In this PR, you have currently (all synchronous):

  • require('assert').equal
  • require('assert').strict.equal
  • require('assert').promises.equal
  • require('assert').promises.strict.equal

I wasn't sure it worth providing require('assert').strict.promises. Do you think it worth it?

This comment has been minimized.

@BridgeAR

BridgeAR Jan 11, 2018

Member

In general strict and regular assert should have exactly the same API.

This comment has been minimized.

@feugy

feugy Jan 11, 2018

Contributor

Copy that

assert.ok(...args);
}
Object.assign(promises, assert, {

This comment has been minimized.

@BridgeAR

BridgeAR Jan 9, 2018

Member

To me as well. I would rather have a dedicated assert.rejects function. Reject is used for promises, so this is a good fit out of my perspective.

@jasnell jasnell added the semver-minor label Jan 9, 2018

@BridgeAR

This comment has been minimized.

Member

BridgeAR commented Jan 11, 2018

Because there seems to be a bit of confusion in the discussion:

I personally am strongly against adding a new promise API.

The only thing that should be added out of my perspective are two new functions rejects and doesNotReject. These two would be accessible in both the legacy and strict mode.

They behave similar to the current throws and doesNotThrow functions but they are async instead of sync. No other assert function should be async as there is no reason for it as far as I can tell.

@feugy

This comment has been minimized.

Contributor

feugy commented Jan 11, 2018

@BridgeAR, by bringing assert.promises, I was following the advise @jasnell gave me on my previous PR.

But what you suggested above makes more sense, so I'm 100% with you, and implement it if @joyeecheung is happy with it.

@joyeecheung

This comment has been minimized.

Member

joyeecheung commented Jan 11, 2018

@feugy If adding asynchronous versions of the synchronous APIs doesn't make sense, then maybe we don't even need to add them to assert.promises. It's fine to only have throws() and doesNotThrow() in assert.promises, and we can work out other APIs later. IMO assert.promises don't need to be feature-complete in this PR.

@apapirovski

This is a good start but I'm also in favour of using rejects and doesNotReject instead of a separate promises namespace.

(Worth noting that this would also be in line with the fact that promises emit unhandledRejection rather than uncaughtException.)

@@ -243,24 +243,17 @@ function expectedException(actual, expected, msg) {
return expected.call({}, actual) === true;
}
function getActual(block) {
if (typeof block !== 'function') {
function isFunction(block) {

This comment has been minimized.

@apapirovski

apapirovski Jan 11, 2018

Member

Can this be moved down closer to trySyncBlock which is its first usage? Or otherwise, move up those two functions here.

I would actually even suggest just inlining this function since it's only used twice. The savings are minimal and now the error thrown will have an extra function in the trace. The latter can be worked around (captureStackTrace) but I don't think it's worth it.

// awaits for async block and catches error
expectThrows(async () => {
await wait(common.platformTimeout(10));

This comment has been minimized.

@apapirovski

apapirovski Jan 11, 2018

Member

We don't need the common.platformTimeout here. Just matters that it's async.

@@ -333,3 +356,31 @@ assert.strict = Object.assign(strict, assert, {
notDeepEqual: assert.notDeepStrictEqual
});
assert.strict.strict = assert.strict;
// Asynchronous equivalent of original assert functions.
// Only throws() and doNotThrows() are truely async (all other perform

This comment has been minimized.

@apapirovski

apapirovski Jan 11, 2018

Member

nits:
s/doNotThrows()/doesNotThrow()
s/truely/truly
s/other/others

const actual = getActual(block);
// Shared code for sync and async doesNotThrow(): no-op if actual is undefined.
// Otherwise, produces an AssertionError when actual matches the expected
// message/error class, or let it bubble.

This comment has been minimized.

@apapirovski

apapirovski Jan 11, 2018

Member

s/or let it bubble/or rethrow.

isFunction(block);
try {
block();
} catch (actual) {

This comment has been minimized.

@apapirovski

apapirovski Jan 11, 2018

Member

catch (err) or catch (e) is more in line with other usage in the code base.

@feugy feugy changed the title from assert: asynchronous throws() and doesNotThrow() to assert: adds rejects() and doesNotReject() Jan 11, 2018

@feugy

This comment has been minimized.

Contributor

feugy commented Jan 18, 2018

Hello gents!

What do you think about the latest update?

@jasnell

LGTM if CI is green

@jasnell

This comment has been minimized.

Member

jasnell commented Jan 18, 2018

Hello gents!

quick fyi... we're not all "gents" :-)

@feugy

This comment has been minimized.

Contributor

feugy commented Jan 18, 2018

Sorry, it's bad habit 😓
I'll start documenting the new functions then.

@BridgeAR

Overall it is already looking very promising to me! Just a few small comments :-)

if (typeof error === 'string') {
if (arguments.length === 3)
if (message !== undefined)

This comment has been minimized.

@BridgeAR

BridgeAR Jan 19, 2018

Member

This is actually a slightly different behavior as before.

Before assert.throws(() => {}, 'foo', undefined) would also result in an error. That is not the case anymore with this change. As this is completely independent from the actual change here, I would rather not have this included, especially as I think this should throw.

This comment has been minimized.

@feugy

feugy Jan 21, 2018

Contributor

You're absolutely right!

To keep this code in an internal function, and not break the existing behaviour, I've used rest params and splat so arguments.length would still make sense.

assert.doesNotThrow = function doesNotThrow(block, error, message) {
const actual = getActual(block);
// Shared code for doesNotThrow() and doesNotReject(): no-op if actual is undefined.

This comment has been minimized.

@BridgeAR

BridgeAR Jan 19, 2018

Member

The comment is actually not correct anymore as it is only a no-op in case actual is the sentinel.

I personally feel like it is not necessary to have these comments here because the code is easy to understand but others might disagree.

This comment has been minimized.

@feugy

feugy Jan 21, 2018

Contributor

👌

'use strict';
const common = require('../common');
const assert = require('assert');
const promisify = require('util').promisify;

This comment has been minimized.

@BridgeAR

BridgeAR Jan 19, 2018

Member

Super tiny nit:

Destructuring would be nice as in const { promisify } = require('util')

operator: undefined,
actual: undefined,
expected: undefined
}));

This comment has been minimized.

@BridgeAR

BridgeAR Jan 19, 2018

Member

I am not totally happy with this test as it does not test that the code does not fail sync.

So I would write something like:

assert.doesNotThrow(() => {
  assert.rejects(
    () => assert.fail(),
    common.expectsError({
      code: 'ERR_ASSERTION',
      type: assert.AssertionError,
      message: 'Failed',
      operator: undefined,
      actual: undefined,
      expected: undefined
    })
  )
});

The same applies to the test below.

This comment has been minimized.

@feugy

feugy Jan 21, 2018

Contributor

👌

// awaits for async block
expectsNoRejection(async () => {
await wait(10);
});

This comment has been minimized.

@BridgeAR

BridgeAR Jan 19, 2018

Member

If I am not mistaken, we do not know if these really work because the function itself might theoretically execute sync.

So a better test out of my perspective would be:

let promise;
let threw = false;
try {
  promise = assert.doesNotReject(async () => {
    await wait(10);
    assert.fail();
  });
} catch (err) {
  threw = true;
}
assert.strictEqual(threw, false);
assert.rejects(() => promise,
  common.expectsError({
    code: 'ERR_ASSERTION',
    type: assert.AssertionError,
    message: 'Got unwanted exception ... '
    ...
  })
);

This comment has been minimized.

@feugy

feugy Jan 21, 2018

Contributor

👌

const promisify = require('util').promisify;
const wait = promisify(setTimeout);
// Ensure async support for assert.rejects() and assert.doesNotReject()

This comment has been minimized.

@BridgeAR

BridgeAR Jan 19, 2018

Member

Hm, I would say it tests the functionality of assert.rejects() and assert.doesNotReject() but not that it "ensures" async support for those as they should always be async.

I would describe it as e.g. "Tests assert.rejects() and assert.doesNotReject() by checking their expected output and by verifying that they do not work sync."

}
assert.throws = function throws(block, error, message) {
expectsError(getActual(block), error, message, this);

This comment has been minimized.

@BridgeAR

BridgeAR Jan 19, 2018

Member

I am a bit surprised that this for the stackStartFn shall work. this should normally refer to either assert.ok or assert.strict.
Can you please write a test for this as well if none exist? :-)

This comment has been minimized.

@feugy

feugy Jan 21, 2018

Contributor

I added a test, and you were absolutely right.
Providing explicit values keeps the existing stack-traces:

https://github.com/feugy/node/blob/6cdbbe6f520862edda962dbf8507c9e7917bdd1e/test/parallel/test-assert-async.js#L60-L98

@BridgeAR

The code change itself looks very good now! Just the tests might need a bit more polishing :-)

};
assert.doesNotReject = async function doesNotReject(block, ...args) {
expectsNoError(assert.doesNotReject, await waitForActual(block), ...args);

This comment has been minimized.

@BridgeAR

BridgeAR Jan 23, 2018

Member

Super super tiny nit: you can actually access the own function without assert. It would just save some characters.

This comment has been minimized.

@joyeecheung

joyeecheung Jan 23, 2018

Member

It also keep you away from monkey-patching. I would prefer referencing it directly here.

} catch (err) {
const lines = err.stack.split('\n')
assert.equal(lines.length, 6)
assert.ok(lines[0].includes('Missing expected exception'))

This comment has been minimized.

@BridgeAR

BridgeAR Jan 23, 2018

Member

Nit: It would be really nice if the error message is changed to Missing expected rejection.

assert.ok(lines[0].includes('Got unwanted exception'))
assert.ok(lines[2].includes('Object.<anonymous>'))
assert.ok(lines[2].includes('test-assert-async.js'))
}

This comment has been minimized.

@BridgeAR

BridgeAR Jan 23, 2018

Member

The two tests above are already tested for in test-assert.js. This adds the check for the error stack, so it indeed tests something new, but in that case I would rather replace the ones in test-assert.js with these tests instead of "duplicating" them.

}
try {
await assert.doesNotReject(() => Promise.reject())

This comment has been minimized.

@BridgeAR

BridgeAR Jan 23, 2018

Member

This tests the same as https://github.com/nodejs/node/pull/18023/files#diff-cdf49dc677a8ea6f00de9f95639355e8R44.
What was not yet tested for is if the functions throws sync.

assert.ok(lines[0].includes('Got unwanted exception'))
assert.ok(lines[2].includes('<anonymous>'))
}
})()

This comment has been minimized.

@BridgeAR

BridgeAR Jan 23, 2018

Member

Using a async function as wrapper will actually hide if the function is executed sync or async. So I would just use the pattern as above without the async wrapper.

@BridgeAR

This comment has been minimized.

Member

BridgeAR commented Jan 23, 2018

@feugy thanks a lot for being so patient!

@Leko

Leko approved these changes Jan 23, 2018

LGTM

};
assert.doesNotReject = async function doesNotReject(block, ...args) {
expectsNoError(assert.doesNotReject, await waitForActual(block), ...args);

This comment has been minimized.

@joyeecheung

joyeecheung Jan 23, 2018

Member

It also keep you away from monkey-patching. I would prefer referencing it directly here.

@joyeecheung

This comment has been minimized.

Member

joyeecheung commented Jan 23, 2018

LGTM with @BridgeAR 's comments addressed

SyntaxError
);
```

This comment has been minimized.

@feugy

feugy Jan 27, 2018

Contributor

Maybe the next lines (copied from assert.doesNotThrow() doc) may be skipped in favour of a reference?

This comment has been minimized.

@BridgeAR

BridgeAR Feb 2, 2018

Member

I agree. I would actually even go so far and remove everything from here on. A text like:

Besides the async nature to await the completion it behaves identical to `assert.doesNotThrow()`.

Keep an example with async await and also maybe add a example using promises.

The same can be applied to the reject function.

operator: stackStartFn.name,
message: `Missing expected ${stackStartFn === rejects
? 'rejection'
: 'exception'}${details}`,

This comment has been minimized.

@feugy

feugy Jan 27, 2018

Contributor

operator & message will now contain appropriate values

This comment has been minimized.

@BridgeAR

BridgeAR Feb 2, 2018

Member

There is the unwritten rule to not use multi line template strings in Node.js. This does look somewhat elegant but it is probably best to just have something like: Missing expected ${fnType}${details}.

e => {
assert.strictEqual(e.code, 'ERR_ASSERTION');
assert(/^Missing expected exception\.$/.test(e.message));
assert.strictEqual(e.operator, 'throws');

This comment has been minimized.

@feugy

feugy Jan 27, 2018

Contributor

I wasn't confident to replace common.expectsError() with such custom assertions, but couldn't find a better way to reuse the existing test, as suggested.

This comment has been minimized.

@BridgeAR

BridgeAR Feb 2, 2018

Member

I would actually just keep this as is and just add a single line to one of the existing tests that checks for assert.ok(!err.stack.includes('at throws').

> Stability: 1 - Experimental
Awaits for the promise returned by function `block` to complete and not be rejected. See [`assert.rejects()`][] for more details.

This comment has been minimized.

@BridgeAR

BridgeAR Feb 2, 2018

Member

This exceeds 80 characters.

expectsError(throws, getActual(block), ...args);
}
assert.throws = throws;

This comment has been minimized.

@BridgeAR

BridgeAR Feb 2, 2018

Member

Nit (non-blocking): I personally prefer the following to save lines:

assert.throws = function throws(block, ...args) {
  expectsError(throws, getActual(block), ...args);
};

This comment has been minimized.

@feugy

feugy Feb 3, 2018

Contributor

I also prefer function expressions.
But even named, function expression do not benefit from hoisting.

This would imply to use assert.doesNotReject() here, and here.

Same goes for other functions.

This comment has been minimized.

@BridgeAR

BridgeAR Feb 6, 2018

Member

Good point. We could check for the function name instead but that could theoretically be manipulated.

So just keep it as is. Thanks for the heads up.

This comment has been minimized.

@BridgeAR

BridgeAR Feb 6, 2018

Member

Hm, while looking at the code again: we actually already use the function name and do not strictly test if it it a "valid" function. So we could indeed just switch to name checking only.

SyntaxError
);
```

This comment has been minimized.

@BridgeAR

BridgeAR Feb 2, 2018

Member

I agree. I would actually even go so far and remove everything from here on. A text like:

Besides the async nature to await the completion it behaves identical to `assert.doesNotThrow()`.

Keep an example with async await and also maybe add a example using promises.

The same can be applied to the reject function.

operator: stackStartFn.name,
message: `Missing expected ${stackStartFn === rejects
? 'rejection'
: 'exception'}${details}`,

This comment has been minimized.

@BridgeAR

BridgeAR Feb 2, 2018

Member

There is the unwritten rule to not use multi line template strings in Node.js. This does look somewhat elegant but it is probably best to just have something like: Missing expected ${fnType}${details}.

}
assert.strictEqual(threw, false);
assert.rejects(() => promise,
err => {

This comment has been minimized.

@BridgeAR

BridgeAR Feb 2, 2018

Member

I am relatively certain that this is going to trigger a lint error. The err should probably be in parentheses. The same applies to the test below.

} catch (err) {
threw = true;
}
assert.strictEqual(threw, false);

This comment has been minimized.

@BridgeAR

BridgeAR Feb 2, 2018

Member

After thinking about this again, the try catch and the threw is not necessary here at all. If the promise throws the test would fail right away. threw is only necessary for tests that check for threw === true. So the tests can be much smaller (I know I originally suggested this pattern).

message: 'Failed'
})
)
});

This comment has been minimized.

@BridgeAR

BridgeAR Feb 2, 2018

Member

The wrapper is not needed as the test would fail right away if assert.rejects would fail.

)
});
assert.doesNotThrow(() => assert.doesNotReject(() => {}))

This comment has been minimized.

@BridgeAR

BridgeAR Feb 2, 2018

Member

The wrapper is not needed due to the reason mentioned above.

e => {
assert.strictEqual(e.code, 'ERR_ASSERTION');
assert(/^Missing expected exception\.$/.test(e.message));
assert.strictEqual(e.operator, 'throws');

This comment has been minimized.

@BridgeAR

BridgeAR Feb 2, 2018

Member

I would actually just keep this as is and just add a single line to one of the existing tests that checks for assert.ok(!err.stack.includes('at throws').

const lines = e.stack.split('\n')
assert.equal(lines.length, 11)
assert.ok(lines[0].includes(e.message))
assert.ok(lines[1].includes('at assert.throws'))

This comment has been minimized.

@BridgeAR

BridgeAR Feb 2, 2018

Member

Hm, I am not certain if that is actually true? I thought that frame would actually be excluded? Do the tests pass?

@feugy

@BridgeAR comments addressed!

},
SyntaxError
);
})();

This comment has been minimized.

@feugy

feugy Feb 3, 2018

Contributor

This async wrapper is really ugly.
But If I omit it, the linter cannot even parse this code (top-level await is not for tomorrow...).

The other option would be to remove await, but it doesn't highlight the async nature of doesNotReject().
What do you think?

This comment has been minimized.

@BridgeAR

BridgeAR Feb 6, 2018

Member

I think it is fine to keep it as is.

expectsError(throws, getActual(block), ...args);
}
assert.throws = throws;

This comment has been minimized.

@feugy

feugy Feb 3, 2018

Contributor

I also prefer function expressions.
But even named, function expression do not benefit from hoisting.

This would imply to use assert.doesNotReject() here, and here.

Same goes for other functions.

assert.strictEqual(err.code, 'ERR_ASSERTION');
assert(/^Got unwanted rejection\.\n$/.test(err.message));
assert.strictEqual(err.operator, 'doesNotReject');
assert.ok(!err.stack.includes('at Function.doesNotReject'));

This comment has been minimized.

@feugy

feugy Feb 3, 2018

Contributor

Without stack trace cleaning:

AssertionError [ERR_ASSERTION]: Got unwanted rejection.

    at new AssertionError (internal/errors.js:320:11)
    at innerFail (assert.js:74:9)
    at expectsNoError (assert.js:464:5)
    at Function.doesNotReject (assert.js:494:3)
    at <anonymous>

With cleaning:

AssertionError [ERR_ASSERTION]: Got unwanted rejection.

    at <anonymous>
assert.strictEqual(err.code, 'ERR_ASSERTION');
assert(/^Missing expected rejection\.$/.test(err.message));
assert.strictEqual(err.operator, 'rejects');
assert.ok(!err.stack.includes('at Function.rejects'));

This comment has been minimized.

@feugy

feugy Feb 3, 2018

Contributor

Without stack trace cleaning:

AssertionError [ERR_ASSERTION]: Missing expected rejection.
    at new AssertionError (internal/errors.js:320:11)
    at innerFail (assert.js:74:9)
    at expectsError (assert.js:439:5)
    at Function.rejects (assert.js:482:3)
    at <anonymous>

With cleaning:

AssertionError [ERR_ASSERTION]: Missing expected rejection.
    at <anonymous>
@@ -447,6 +447,7 @@ assert.throws(makeBlock(thrower, TypeError));
} catch (e) {
threw = true;
assert.ok(e instanceof a.AssertionError);
assert.ok(!e.stack.includes('at Function.doesNotThrow'));

This comment has been minimized.

@feugy

feugy Feb 3, 2018

Contributor

As requested, I removed the unnecessary test, and enriched an existing one.

Without stack trace cleaning:

AssertionError [ERR_ASSERTION]: Got unwanted exception.
[object Object]
    at new AssertionError (internal/errors.js:320:11)
    at innerFail (assert.js:74:9)
    at expectsNoError (assert.js:464:5)
    at Function.doesNotThrow (assert.js:488:3)
    at Object.<anonymous> (D:\Workspaces\perso\node\test\parallel\test-assert.js:450:7)

With cleaning:

AssertionError [ERR_ASSERTION]: Got unwanted exception.
[object Object]
    at Object.<anonymous> (D:\Workspaces\perso\node\test\parallel\test-assert.js:446:7)
} catch (e) {
threw = true;
assert.ok(e instanceof a.AssertionError);
assert.ok(!e.stack.includes('at Function.throws'));

This comment has been minimized.

@feugy

feugy Feb 3, 2018

Contributor

As requested, I've restored the previous test, but I cannot extend an existing test because:

  • Only 4 tests a triggering the "Missing expected exception", which I'm looking for
  • those 4 tests are using common.expectsError(), and a a assert.throws(() => { a.throws( pattern which makes the stack trace more complicated than it should.

Without stack trace cleaning:

AssertionError [ERR_ASSERTION]: Missing expected exception.
    at new AssertionError (internal/errors.js:320:11)
    at innerFail (assert.js:74:9)
    at expectsError (assert.js:439:5)
    at Function.throws (assert.js:476:3)
    at Object.<anonymous> (D:\Workspaces\perso\node\test\parallel\test-assert.js:566:12)

With cleaning:

AssertionError [ERR_ASSERTION]: Missing expected exception.
    at Object.<anonymous> (D:\Workspaces\perso\node\test\parallel\test-assert.js:566:12)
} catch (e) {
threw = true;
assert.ok(e instanceof a.AssertionError);
assert.ok(!e.stack.includes('at Function.throws'));

This comment has been minimized.

@feugy

feugy Feb 3, 2018

Contributor

As requested, I reverted the existing test.
But I had to add this new one, because:

  • only 4 tests are triggering the 'Missing expected exception' which I'm looking for
  • those tests are using common.expectsError( and the same assert.throws(() => { a.throws( pattern which overcomplicates the stack trace.

Without stack trace cleaning:

AssertionError [ERR_ASSERTION]: Missing expected exception.
    at new AssertionError (internal/errors.js:320:11)
    at innerFail (assert.js:74:9)
    at expectsError (assert.js:439:5)
    at Function.throws (assert.js:476:3)
    at Object.<anonymous> (D:\Workspaces\perso\node\test\parallel\test-assert.js:566:12)

With cleaning:

AssertionError [ERR_ASSERTION]: Missing expected exception.
    at Object.<anonymous> (D:\Workspaces\perso\node\test\parallel\test-assert.js:566:12)
@BridgeAR

LGTM

Some small suggestions that would be nice to get in but I do not want to block this any further for those nits.

@@ -395,8 +395,7 @@ function expectedException(actual, expected, msg) {
function getActual(block) {
if (typeof block !== 'function') {
throw new TypeError('ERR_INVALID_ARG_TYPE', 'block', 'Function',
block);
throw new TypeError('ERR_INVALID_ARG_TYPE', 'block', 'Function', block);

This comment has been minimized.

@BridgeAR
expectsError(throws, getActual(block), ...args);
}
assert.throws = throws;

This comment has been minimized.

@BridgeAR

BridgeAR Feb 6, 2018

Member

Hm, while looking at the code again: we actually already use the function name and do not strictly test if it it a "valid" function. So we could indeed just switch to name checking only.

{
const promise = assert.rejects(async () => {
await wait(10);

This comment has been minimized.

@BridgeAR

BridgeAR Feb 6, 2018

Member

Just if you feel like it: our tests currently take to much time, so the smallest async operation would already be enough.

function, and awaits for completion.
Besides the async nature to await the completion it behaves identical to
`assert.doesNotThrow()`.

This comment has been minimized.

@BridgeAR

BridgeAR Feb 6, 2018

Member

It would be nice to reference that function. And maybe also to add smth. like: For further usage information, read that part.

(The same applies to assert.throws).

@BridgeAR

This comment has been minimized.

Member

BridgeAR commented Feb 6, 2018

@BridgeAR BridgeAR added notable-change and removed author ready labels Feb 7, 2018

@BridgeAR

This comment has been minimized.

Member

BridgeAR commented Feb 10, 2018

I removed the ready and would like this to sit for a bit longer. I would like to deprecate assert.doesNotThrow and in that case it is probably better not to add assert.doesNotReject.

@feugy

This comment has been minimized.

Contributor

feugy commented Feb 10, 2018

@BridgeAR, to be sure I understand, you simply want to remove doesNotReject?
I can do it as part of the documentation changes you requested.

Out of curiosity, what is the reason to deprecate doesNotThrow?

@BridgeAR

This comment has been minimized.

Member

BridgeAR commented Feb 10, 2018

@feugy see #18699 and #18669.

I would just wait with removing it until it is decided if we deprecate it or not because I would like to have feature parity.

@apapirovski

This comment has been minimized.

Member

apapirovski commented Mar 2, 2018

@BridgeAR It seems to me like rejections have slightly different semantics than throw. Since it's currently possible to just entirely ignore unhandledRejection in our code, I can see a use for doesNotReject. Just IMO of course...

@BridgeAR

This comment has been minimized.

Member

BridgeAR commented Mar 2, 2018

@apapirovski I agree that it is not exactly the same but the reason for that is that we have another issue that has to be solved as well. See #15126.

@apapirovski

This comment has been minimized.

Member

apapirovski commented Mar 2, 2018

@BridgeAR Yeah, I'm aware and have dug around in that PR, actually, but that code will take a while to land given the performance implications. I'm not really certain we have a great path forward that doesn't impact performance. Maybe I'm wrong...

(I have some thoughts on how to do it without all of the C++ boundary crossing but only theoretical atm.)

@apapirovski

This comment has been minimized.

Member

apapirovski commented Mar 5, 2018

@BridgeAR given that this is only semver-minor whereas any changes to uncaught Promise handling would be semver-major, do you think it would be ok to land as is? Especially in the light of the recent rejection of the doesNotThrow deprecation?

@apapirovski

This comment has been minimized.

@BridgeAR

This comment has been minimized.

Member

BridgeAR commented Mar 5, 2018

@apapirovski yes, I am now ok with landing it as is. I will open another PR to add a comment to it similar to doesNotThrow afterwards.

@feugy thanks a lot for your work and for being patient :)

@feugy

This comment has been minimized.

Contributor

feugy commented Mar 6, 2018

It's all fine!
I'll resolve the conflict soon.

assert: add rejects() and doesNotReject()
Implement asynchronous equivalent for assert.throws() and
assert.doesNotThrow().
@feugy

This comment has been minimized.

Contributor

feugy commented Mar 10, 2018

And done! What's next?

BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 11, 2018

assert: add rejects() and doesNotReject()
Implement asynchronous equivalent for assert.throws() and
assert.doesNotThrow().

PR-URL: nodejs#18023
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@BridgeAR

This comment has been minimized.

Member

BridgeAR commented Mar 11, 2018

Landed in 599337f 🎉

I removed the experimental as I do not really see that and fixed a typo in the docs while landing.

@MylesBorins

This comment has been minimized.

Member

MylesBorins commented Mar 20, 2018

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

@not-an-aardvark not-an-aardvark referenced this pull request Mar 28, 2018

Closed

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

3 of 3 tasks complete

@targos targos referenced this pull request Apr 2, 2018

Closed

assert: add warning about `assert.doesNotReject` #19462

4 of 4 tasks complete
@not-an-aardvark

This comment has been minimized.

Member

not-an-aardvark commented Apr 4, 2018

If this gets backported, it should be backported at the same time as #19650 (possibly in the same commit).

BridgeAR added a commit to BridgeAR/node that referenced this pull request May 1, 2018

assert: add rejects() and doesNotReject()
Implement asynchronous equivalent for assert.throws() and
assert.doesNotThrow().

PR-URL: nodejs#18023
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

MayaLekova added a commit to MayaLekova/node that referenced this pull request May 8, 2018

assert: add rejects() and doesNotReject()
Implement asynchronous equivalent for assert.throws() and
assert.doesNotThrow().

PR-URL: nodejs#18023
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

MylesBorins added a commit to MylesBorins/node that referenced this pull request Nov 1, 2018

assert: add rejects() and doesNotReject()
Implement asynchronous equivalent for assert.throws() and
assert.doesNotThrow().

PR-URL: nodejs#18023
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

MylesBorins added a commit to MylesBorins/node that referenced this pull request Nov 1, 2018

assert: add rejects() and doesNotReject()
Implement asynchronous equivalent for assert.throws() and
assert.doesNotThrow().

PR-URL: nodejs#18023
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

MylesBorins added a commit that referenced this pull request Nov 4, 2018

assert: add rejects() and doesNotReject()
Implement asynchronous equivalent for assert.throws() and
assert.doesNotThrow().

Backport-PR-URL: #24019
PR-URL: #18023
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

@BethGriggs BethGriggs referenced this pull request Nov 12, 2018

Open

v8.13.0 proposal #23974

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment