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

[cjihrig]

This is ready to land, but before it does, I'd like to hear from @JoshuaKGoldberg, who opened #51292. Do you still think changing the return types of these APIs is an improvement, with the assumption that people will need to lint their code across multiple versions of Node? As a linter maintainer, what will you tell users to do here?

Also note that we can make the 'automatically await subtests' change without changing the types, but we can't change the types without awaiting the subtests.

[JoshuaKGoldberg]

First of all, ❤️ you do fantastic work @cjihrig and I hope nothing around the linting / node:test conversation has made you unhappy.

tl;dr: this change looks great to me and I appreciate you working on it. Thank you!

Do you still think changing the return types of these APIs is an improvement, ...

I do. Both as a general user of node:test and as a linter user. As a general user I've had to deal with far too many bugs caused by parallel tests interacting with each other. And as a linter user I can't envision a programmatic way to configure linters to ignore just the fire-and-forget subtest calls, not other ones that should be awaited.

... with the assumption that people will need to lint their code across multiple versions of Node? As a linter maintainer, what will you tell users to do here?

IME: people don't generally lint across multiple versions of Node.js. Projects generally run linting in the user's editor and in a single task in CI.

I think the versioning concern really only applies to the single version of @types/node used in a project. Projects pinned to an older version of @types/node will have its existing->previous types saying the test functions return Promise<void>. Once this change goes in and a corresponding update to @types/node is released, the projects won't have that issue anymore.

So, I don't see this as being a problem cross-Node.js-versions for linters. Unless I misinterpreted something?

[cjihrig]

As a general user I've had to deal with far too many bugs caused by parallel tests interacting with each other. And as a linter user I can't envision a programmatic way to configure linters to ignore just the fire-and-forget subtest calls, not other ones that should be awaited.

So, I do think both of those problems could be solved with just the first commit here (automatically awaiting subtests). Obviously parallel tests can always interact, but you would not need to await any subtests or try to manage Promises. From the linter point of view, everything would become fire-and-forget.

IME: people don't generally lint across multiple versions of Node.js. Projects generally run linting in the user's editor and in a single task in CI.

This is the part that concerns me. My experience has been that a lot of projects linting is run in the CI for all versions. For example, in the Kubernetes client we recently migrated to typescript-eslint, and it is run on every version even though that might not be the most efficient thing. Another pattern is running the linter as part of npm test, which is what Fastify appears to do. They only lint one version in their CI, but it seems like anyone developing across versions would run into a lot of linting problems.

[JoshuaKGoldberg]

What benefits are there to running linting across versions? Are there any examples of past issues that have been caught by it?

In general, for linting, I just don't see any motivation to run it matrixed in CI across the Node version:

• Lint rules don't generally run project runtime source code
  • Custom rules might refer to shared libraries also built in the project, but I've yet to see that be Node-version-dependent in the real world
• Intricacies of the node_modules structure shouldn't impact linting - especially in projects with lockfiles
• Plugins generally don't change behavior across Node versions
  • I can vouch that in typescript-eslint, if a difference like this were to be reported, we'd almost certainly treat it as a bug

[jsumners]

I will add me to the "lint once" evidence. I have never considered linting across multiple versions of Node. It's either JavaScript that is good, or it isn't.

[cjihrig]

I agree that there is not much benefit to doing it, but people do it a lot. I did not setup the CI on that project, but my guess is that it's just easier to do it at the same time as running the tests rather than setting up something separate. For my own projects, I generally run the linter as part of npm test and hopefully never think about it again.

From a quick search a few larger projects that lint across versions that I think would have problems with this type of update:

• kubernetes client for node (already mentioned)
• hapi
• koa
• nodemailer
• whatwg-url
• apollo-upload-client

[Renegade334]

My experience has been that a lot of projects linting is run in the CI for all versions. For example, in the Kubernetes client we recently migrated to typescript-eslint, and it is run on every version even though that might not be the most efficient thing.

To follow on from JoshuaKGoldberg's point, since each version's lint run will be linting against the same type definitions for the test runner API (from whichever version of @types/node is installed in the project), I wouldn't imagine this would be an issue? Either all the lint runs would fail, or none would?

[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