Nit: s/conveniency/convenience/?

Thank you 😅

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

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

👌

Since other assigned APIs do not return promises here:

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

This behavior looks somewhat strange to me..

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.

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?

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

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.

Why not?

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?

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

Copy that

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.

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.

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

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

s/or let it bubble/or rethrow.

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

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.

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.

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.

👌

Super tiny nit:

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

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.

👌

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 ... '
    ...
  })
);

👌

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

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? :-)

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

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

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

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

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.

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.

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.

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

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

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 & message will now contain appropriate values

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

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.

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').

This exceeds 80 characters.

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

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

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.

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.

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.

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.

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

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.

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

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

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

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').

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