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 #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: #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: #51292 (comment)

[codecov]

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 ⚠️

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%)	⬇️
lib/internal/test_runner/harness.js	93.13% <85.71%> (-0.22%)	⬇️

... and 41 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 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]

lgtm

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

[marco-ippolito]

I agree the previous behavior was more sane

[pmarchini]

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

After thinking about it I changed my mind, the use case provided is very uncommon and not worth reverting. Maybe we should provide another setting/api that returns a promise

[pmarchini]

Hey @romainmenke, if you want, you can cherry-pick my latest commit from #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!

[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:

[LiviaMedeiros]

@nodejs/tsc given that this was on the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. for several meetings and no objections were raised, is there any reason to keep this PR in its current state?

The consensus seems to be that there's slight chance of partial revert to be breaking, I suggest to simply merge this right away as-is. After that,

• We can re-introduce aa3523e (I'll be happy to open follow-up PR with @cjihrig as the commit author, and watch that it gets in the same release as this PR)
• If there's still interest in revisiting node:test APIs returning Promises clashes with no-floating-promises lint rule #51292 and fixing it on the node:test side, the issue can be reopened (or a new one created). If there will be consensus that it shouldn't return promises, we can initiate a deprecation cycle.

[nodejs-github-bot]

Landed in af66574...04cb572

[LiviaMedeiros]

@romainmenke thanks for the contribution!

Opened #58800 to re-introduce aa3523e.

[romainmenke]

Thank you everyone for the help and thoughtful approach to this change 🙇

[github-actions]

The notable-change PRs with changes that should be highlighted in changelogs. label has been added by @LiviaMedeiros.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.

[LiviaMedeiros]

Retroactively adding notable-change PRs with changes that should be highlighted in changelogs. , we probably should reflect in the changelog that subtests can/should be awaited again.