Revert https://github.com/nodejs/node/pull/56664

[romainmenke]

Being able to await sub tests and orchestrate the order of sub tests and other async functions was no longer possible after #56664

I think this was an obvious mistake.

As far as I understand from https://github.com/nodejs/node/issues/51292 the original issue was that sometimes people get linter nags.

No one actually sufficiently argued that the previous behavior was either wrong or not a useful capability.

I suggest the changes are reverted to restore the previous capabilities. Being able to await sub tests was a really useful feature.

The linter concerns should be secondary to the capabilities of node:test

Fixes: https://github.com/nodejs/node/issues/58227

[nodejs-github-bot]

Review requested:

• [ ] @nodejs/test_runner

[pmarchini]

Hey @romainmenke , thanks for your contribution! As a first-time contributor, I suggest you take a look at https://github.com/nodejs/core-validate-commit.

Regarding the content of the PR: I think that, as the behaviour has already been implemented, it would be reasonable to try to make this behaviour configurable via a flag. What do you think?

[romainmenke]

Regarding the content of the PR: I think that, as the behaviour has already been implemented, it would be reasonable to try to make this behaviour configurable via a flag. What do you think?

I think it is also reasonable to consider a straight up revert :) The previous behavior has been around for a lot longer and many people depend on it. It also offers more capabilities.

As I said in the pull request message, I don't think the change was sufficiently argued for. Reverting to the previous state and re-opening the original issue might reveal a better way forwards.

---

As a first-time contributor, I suggest you take a look at https://github.com/nodejs/core-validate-commit.

Happy to look at this, but unsure how to proceed. Do you mean that I should not have commit messages in the default git revert style? And that I should alter each message and do a force push?

[pmarchini]

Happy to look at this, but unsure how to proceed. Do you mean that I should not have commit messages in the default git revert style?

Sorry, I only just noticed that the original PR landed without a squash commit! The PR should be fine as is 😁

I think it is also reasonable to consider a straight up revert :)

~~Personally I agree, IMHO we already have suite-test/describe-it that covers that use case.~~ On second thought, I still think that the introduced behaviour provides a more intuitive devEx, and that an optional configuration could be added to cover these use cases.

I'd like to collect some feedback from @MoLow, @cjihrig, @jakecastelli, and @atlowChemi too!

[aduh95]

Could you give an example of something that no longer works with the new behavior in #51292, with a link to this PR? That should give everyone who participated in that issue a notification so they can participate in this discussion

[romainmenke]

See: https://github.com/nodejs/node/issues/51292#issuecomment-2870050165

[codecov[bot]]

Codecov Report

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

Project coverage is 90.21%. Comparing base (770be2c) to head (afe2fea). Report is 314 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/test_runner/harness.js	85.71%	1 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #58282      +/-   ##
==========================================
+ Coverage   90.18%   90.21%   +0.03%     
==========================================
  Files         631      633       +2     
  Lines      186689   186793     +104     
  Branches    36663    36672       +9     
==========================================
+ Hits       168359   168519     +160     
+ Misses      11128    11045      -83     
- Partials     7202     7229      +27     

Files with missing lines	Coverage Δ
lib/internal/test_runner/test.js	97.11% <100.00%> (-0.03%)	:arrow_down:
lib/internal/test_runner/harness.js	93.13% <85.71%> (-0.22%)	:arrow_down:

... and 41 files with indirect coverage changes

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[pmarchini]

I think we should avoid reverting this feature and instead explore an idea that aligns with the current direction: the addition of a step method similar to what Deno provides. Correct me if I’m wrong, but from your perspective the issue doesn’t seem to be about the return type itself (or how promises are handled), but rather about a lost capability, specifically the ability to have asynchronous intermediate steps executed in a controlled way between tests.

Here’s an example, based on what you shared in the other issue, of what it could look like using steps:

import { test } from 'node:test';
import assert from 'node:assert';

test('outer', async (t1) => {
	let a = 1;
	let b = 1;

	// Run a first sub test
	t1.test('inner 1', async (t2) => {
		await new Promise((resolve) => setTimeout(resolve, 1000));
		assert.equal(a, b);

		t2.after(async () => {
			await new Promise((resolve) => {
				b = 2;
				resolve();
			});
		});
	});

	// Do some async workload in between sub tests
	t1.step('async workload', async () => new Promise((resolve) => {
		b = 2;
		resolve();
	}));

	// Run a second sub test
	t1.test('inner 2', async () => {
		// Run some extra asserts
		assert.equal(b, 2);
	});
});

Something like this could potentially address the issue while avoiding the reintroduction of the discrepancy between suite/test and t.test.

[romainmenke]

I think we should avoid reverting this feature and instead explore an idea that aligns with the current direction: the addition of a step method similar to what Deno provides.

Such new API doesn't really help when we still support a wide range of node versions. In the long term it might, but it doesn't resolve the short term disruption.

I also honestly do not understand how this change got through in the first place. I don't understand how anyone favored more callbacks over async/await. Being able to await async workloads is a better DX than callback hell, it is why we have async/await in the first place.

Before the change made in #56664 it was possible to intuitively and organically add sub tests in between existing code. Or to add some non-test code in between sub tests

After #56664 you have to be very careful that in a given test you either have only sub tests or only non-sub test code. You can no longer interleave these without race conditions.

This is clearly bad, right? Needing to invent new API that is somewhat equivalent to a sub test is just making it worse, not better imho.

I really liked that node:test and node --test foo.mjs had the core principle that every test is just a JavaScript program and that there weren't any gotcha's. It's just JavaScript code as any other JavaScript code. After #56664 I think this is no longer true. You have to carefully keep within the bounds of the "framework" or your tests won't work as expected.

[mcollina]

I concur on a quick revert.

[mcollina]

@romainmenke can you please amend the other tests that are failing?

[aduh95]

Tests are failing on GHA, I don't think it makes sense to run Jenkins CI now

[pmarchini]

As far as I can tell, it seems that #57672 is the origin of the failing tests (cc @jakecastelli). @romainmenke, I saw that you updated the test in this commit 0c3e8dc, but it requires a snapshot regeneration as well.

You can do it via NODE_REGENERATE_SNAPSHOTS=1 out/Release/node --expose-internals test/parallel/test-runner-output.mjs.

Unfortunately, I just tried and it seems that the behavior introduced by #57672 is broken after the revert.

[pmarchini]

If nobody precedes me, I'll take a look at the failing tests ASAP.

[pmarchini]

Hey @romainmenke, if you want, you can cherry-pick my latest commit from https://github.com/nodejs/node/pull/58320.

After this commit, I think everything is in place for a potential revert.

IMHO, reverting is still not the right choice, and finding an alternative way to provide this capability would be much better from a DevEx/consistency perspective.

I won't accept this PR for that reason. I provided support to avoid blocking progress in case a decision is made to revert.

I've seen that we have a tsc-agenda set, so let's see!

[romainmenke]

@pmarchini Thank you so much for helping, even when you would prefer to keep the now current behavior :)

[mrazauskas]

A middle ground idea: make subtests with async callback awaitable. Simplified signatures:

declare function test(cb: () => Promise<unknown>): Promise<void>;
declare function test(cb: () => unknown): void;

Might be I miss something, but on surface only the async callbacks seem to be a problem. So instead of a revert this could qualify as a fix only for the async cases.

[nodejs-github-bot]

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

[joyeecheung]

I don't have much opinion about whether it should be reverted, just wondering if all the commits need be reverted for the happiest outcome or only some of them must be reverted?

[mhdawson]

Based on the discussion in the TSC meeting today (8 TSC members) there were no objections to reverting

@nodejs/tsc please comment if you have an objection/concern, otherwise we'll assume if there are not objections by the end of the week that it can be reverted and backported to 24.x

[romainmenke]

I don't have much opinion about whether it should be reverted, just wondering if all the commits need be reverted for the happiest outcome or only some of them must be reverted?

Maybe https://github.com/nodejs/node/pull/56664/commits/bbfce04a3be2fdd49349a52d9af12c997fa41cb0 can be kept? Is this what you were referring to @joyeecheung?

I somehow assumed that the three commits formed a whole and should be reverted together. Happy to make changes.

[joyeecheung]

Maybe https://github.com/nodejs/node/commit/bbfce04a3be2fdd49349a52d9af12c997fa41cb0 can be kept? Is this what you were referring to?

I don't know enough about the breakage so can only leave it to those who are affected to decide but if keeping commit doesn't cause regression, it seems there is no need to revert it?

[LiviaMedeiros]

I confirm that a build keeping only https://github.com/nodejs/node/commit/aa3523ec22c95dfb11ef93c8333ced41cd431f89 passes all Node.js tests locally, and does not break userland tests that use await context.test(). AFAICT the only "breakage" it may cause is waiting forever for hanged promise; can't come up with any scenario where leaving such promise could be intentional and desired.

[pmarchini]

Maybe bbfce04 can be kept? Is this what you were referring to?

I don't know enough about the breakage so can only leave it to those who are affected to decide but if keeping commit doesn't cause regression, it seems there is no need to revert it?

I think we should consider this block of work as a single "thing" as reverting only one of the commits would still introduce breaking changes

[joyeecheung]

So is the verdict still that it's better to revert them together? (I have no opinions since I don't know how much the breakages differ, just asking in case that's also an option, but if someone who actually knows what the breakage entails think it's better to revert them together and others aren't objecting to it it seems the best way forward is to revert them together)

[nodejs-github-bot]

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

[panva]

@cjihrig There were no objections to a revert in the previous TSC meeting, but we'd like to know whether all of the three commits in your original PR need to be reverted to resolve the reported issues or whether only a partial revert is possible, namely aa3523ec22c95dfb11ef93c8333ced41cd431f89 seems to stand on its own and appears as a valuable addition to keep.

[nodejs-github-bot]

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

[alcuadrado]

Reverting feels like optimizing for the edge case instead of the most common case. I'm not a contributor, so I don't know how much my opinion matters, but not being able to lint with no-floating-promises in tests is a huge drawback.

[alcuadrado]

Paraphrasing, @mcollina, it feels a bit like this: