test_runner: remove Promise return values from test(), suite(), and t.test()

[cjihrig]

test_runner: automatically wait for subtests to finish

This commit updates the test runner to automatically wait for
subtests to finish. This makes the experience more consistent
with suites and removes the need to await anything.

test_runner: remove promises returned by test()

This commit updates the test() and suite() APIs to no longer
return a Promise.

Fixes: #51292

test_runner: remove promises returned by t.test()

This commit updates the TestContext.prototype.test() API to no
longer return a Promise.

Fixes: #51292

[nodejs-github-bot]

Review requested:

• @nodejs/test_runner

[codecov]

Codecov Report

Attention: Patch coverage is 97.05882% with 1 line in your changes missing coverage. Please review.

Project coverage is 89.21%. Comparing base (55cc372) to head (30b191c).
Report is 36 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/test_runner/harness.js	94.73%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #56664   +/-   ##
=======================================
  Coverage   89.21%   89.21%           
=======================================
  Files         663      663           
  Lines      192002   192028   +26     
  Branches    36928    36925    -3     
=======================================
+ Hits       171292   171319   +27     
+ Misses      13583    13582    -1     
  Partials     7127     7127           

Files with missing lines	Coverage Δ
lib/internal/test_runner/test.js	97.09% <100.00%> (+0.02%)	⬆️
lib/internal/test_runner/harness.js	92.91% <94.73%> (+0.23%)	⬆️

... and 28 files with indirect coverage changes

[mcollina]

lgtm

[pmarchini]

Aside from the nits: I think this is a great devEx enhancement! LGTM

[cjihrig]

There is no change here regarding how a test is recognized as finished. The test is either a synchronous function, a function that returns a Promise, or a function that takes a callback. The test is finished after awaiting the return value or the callback is invoked. That part is already documented.

For example, how would it understand this one completes?

Unrelated to this change - I believe this first snippet has a bug, assuming someCallback() is asynchronous? If it is asynchronous, there should be a Promise or a done callback involved. The second code snippet looks correct, but a Promise could have been used as well.

Does this PR make the done callback required for all subtests?

No.

[JakobJingleheimer]

Do we have a test for ensuring dynamically added test cases finish before the suite closes? This await behaviour was previously required in order to orchestrate that in userland, so I'm just concerned this could break that.

Ref nodejs/nodejs.org#7387

[cjihrig]

Yes, dynamically added tests still work. The breaking change (aside from the return value change) is that you can no longer await subtests for control flow purposes. For users of describe() there are no changes at all. For users of t.test() awaiting the result becomes await undefined.

[jsumners]

The breaking change (aside from the return value change) is that you can no longer await subtests for control flow purposes.

I'm confused. The docs still say that a test can return a promise:

node/doc/api/test.md

Lines 32 to 43 in 2b34ffe

	Tests created via the `test` module consist of a single function that is
	processed in one of three ways:
	1. A synchronous function that is considered failing if it throws an exception,
	and is considered passing otherwise.
	2. A function that returns a `Promise` that is considered failing if the
	`Promise` rejects, and is considered passing if the `Promise` fulfills.
	3. A function that receives a callback function. If the callback receives any
	truthy value as its first argument, the test is considered failing. If a
	falsy value is passed as the first argument to the callback, the test is
	considered passing. If the test function receives a callback function and
	also returns a `Promise`, the test will fail.

Are you saying that we can no longer await those returned promises in order to keep the parent test alive?

[cjihrig]

const r = test('foo', () => {
  return new Promise((resolve) => {
    resolve();
  });
});

This test returns a Promise (the Promise that is resolved()). When that Promise settles, the test is considered finished. That has not changed.

r is the value that node:test returns from test(). This is not, and never has been, the same Promise inside of the test.

Previously, if you were writing a test inside of another test, you needed to await r so that the parent test would not finish before the subtest. With this change, the test runner will automatically wait for you. That is already how describe() works.

The only scenario where this is breaking:

test('foo', async (t) => {
  await t.test('subtest 1');
  // It is no longer guaranteed that the subtest on the previous line has
  // finished by the time this line executes. But the test runner still ensures
  // the order that subtest 1 and subtest 2 are executed in.
  await t.test('subtest 2');
});

[jsumners]

The only scenario where this is breaking:

Thank you for the clarification. I was unaware of await test to guarantee subtests executing in a specific order. That sounds like someone relying on side effects in tests and not something I do. 👍

[targos]

So, is t.test() now identical to a nested test() call?

[cjihrig]

As far as I know, that was already the case. The difference was that describe() waited for its children, while test() and t.test() did not. Now, all of them wait for their children.

[targos]

Approving the breaking change.

[nodejs-github-bot]

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

[climba03003]

Can we keep the return Promise but allows to automatic wait for all sub-test be done first, then remove the Promise return on next major?

The main reason is that

1. Allows backport to 20 and 22. (I believe semver-major won't backport to old release.)
2. Better migration path for removing the await test when the package is supporting multiple node version

[JoshuaKGoldberg]

👍 I checked just now and none of those repositories swap what version of @types/node they use. They all either use 0 or 1 version.

So, I think the only way this change would likely+reasonably get in the way via linting is if they:

1. Bump their version of @types/node
2. Autofix away awaits due to reports from @typescript-eslint/await-thenable
3. Run tests matrixed in an older version of Node.js

To get around that, I'd advise using inline eslint-disable-next-line comments on any places where a subtest actually does need to be awaited:

// eslint-disable-next-line @typescript-eslint/await-thenable
await t.test(...);

If there are a ton they could always disable @typescript-eslint/await-thenable in their ESLint config for just e.g. *.test.* files:

{
  files: ['src/**/*.test.*'],
  rules: { '@typescript-eslint/await-thenable': 'off' }
}

[cjihrig]

Thanks @JoshuaKGoldberg. I've been thinking about this PR for the past few days. I still have a lot of apprehension about landing the return type change, but given the number of folks who appear to be on board with it, I think I'm going to rebase and get this landed.

[nodejs-github-bot]

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

[jasnell]

Landed in ba4587c...1a2eb15