assert: async parameter for assert.throws() and assert.doesNotThrow()

[feugy]

Hello community!
It's my first PR, I hope I didn't missed important stuff.
Documentation isn't included yet as I'd like to get feedback on this new feature first.

The first parameter of assert.throws() and assert.doesNotThrow() can be an async function.

Benefit is to make tests way more easier to read.
Validating error behaviour with promise-based code used to look like this:

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

Now, thanks to async/await support, it looks like this:

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

With native async support, it can now be written like this:

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

However, it may be considered as a breaking change: assert.throws() used to return nothing, while now, due to its async nature, it returns a Promise.

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

[Trott]

No opinion (yet) one way or the other, but it does seem to break this test case which you probably want to fix?:

{
  let threw = false;
  try {
    a.throws(makeBlock(thrower, TypeError), a.AssertionError);
  } catch (e) {
    threw = true;
    assert.ok(e instanceof TypeError, 'type');
  }
  assert.strictEqual(true, threw,
                     'a.throws with an explicit error is eating extra errors');
}

(This test it in test-assert.js.)

[Trott]

Hi @feugy! Welcome and thanks for the pull request. This may be obvious, but do be patient given the time of year. Lots of folks might not be available for another week or maybe even longer.

Meanwhile, I'll @-mention @BridgeAR to get their attention (since they've been working on lib/assert.js more than anyone else lately)

[apapirovski]

Thanks for the pull request, @feugy!

My initial feeling is that making assert.throws & assert.doesNotThrow return a Promise is a no-go. It can break way too much existing code and it also means that they cannot be put inside a try-catch block.

I'm guessing this also presents a significant regression for the benchmark at benchmark/assert/throws.js as it means the two functions are now always creating a Promise, even when the code passed in is completely sync.

This is also further complicated by a scenario where the function passed in is supposed to return a Promise and the end-user have no expectation of assert.throws actually waiting for the evaluation of said Promise. This change would break the current behaviour and would provide no way of distinguishing between someone intentionally passing in an async/await function vs someone passing in a function that just returns a Promise that shouldn't be evaluated.

[BridgeAR]

This is a very hard breaking change because no error will be thrown sync anymore. As far as I see it, it is not possible to implement this as requested. The only possibility I see is adding a new function with this functionality. I would be fine with that (the name should indicate that it is async in this case, since all assert functions are sync).

Theoretically we could check if the input is a promise and special handle that but this would not only also be a hard breaking change, it would also release Zalgo.

[feugy]

Hi gents!

Thank you so much for the feedback. I wasn't expecting such quick answer.
I sincerely apologies for not having spot the regression. I wouldn't have submitted the PR otherwise.
(side note: python .\tools\test.py -p tap assert doesn't run test-assert.js)

It is definitely better to provide distinct functions. I'll do it if you guys think this feature could be handy.

[jasnell]

While moving to a promises-based API is generally the right thing, doing it this way is the wrong approach. Rather than modifying the existing methods (and breaking existing code), new async methods should be exposed. See #17739 for an example.

[jasnell]

Making the objection on this explicit for the time being.

[BridgeAR]

@feugy do not worry. Thanks a lot for the suggestion. I recommend to open a new PR that adds the new functions.

I am closing this due to the expressed issues.