test_runner: ensure proper teardown when tests run without isolation

[pmarchini]

This should address #57234.

Adding the teardown in case of isolation: none addresses this specific issue, and I see no potential negative effects in having it.

@cjihrig, was there a reason why we avoided it in the first place?

[nodejs-github-bot]

Review requested:

• @nodejs/test_runner

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.23%. Comparing base (9df0ff7) to head (dd7af59).
⚠️ Report is 3924 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #57394      +/-   ##
==========================================
- Coverage   90.23%   90.23%   -0.01%     
==========================================
  Files         630      630              
  Lines      185200   185214      +14     
  Branches    36233    36243      +10     
==========================================
+ Hits       167108   167120      +12     
- Misses      11056    11057       +1     
- Partials     7036     7037       +1     

Files with missing lines	Coverage Δ
lib/internal/test_runner/runner.js	89.52% <100.00%> (+0.01%)	⬆️

... and 22 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]

These changes have broken the following test:

node/test/parallel/test-runner-extraneous-async-activity.js

Lines 52 to 68 in 96457b4

	{
	const child = spawnSync(process.execPath, [
	'--test',
	'--test-isolation=none',
	fixtures.path('test-runner', 'async-error-in-test-hook.mjs'),
	]);
	const stdout = child.stdout.toString();
	assert.match(stdout, /Error: Test hook "before" at .+async-error-in-test-hook\.mjs:3:1 generated asynchronous activity after the test ended/m);
	assert.match(stdout, /Error: Test hook "beforeEach" at .+async-error-in-test-hook\.mjs:9:1 generated asynchronous activity after the test ended/m);
	assert.match(stdout, /Error: Test hook "after" at .+async-error-in-test-hook\.mjs:15:1 generated asynchronous activity after the test ended/m);
	assert.match(stdout, /Error: Test hook "afterEach" at .+async-error-in-test-hook\.mjs:21:1 generated asynchronous activity after the test ended/m);
	assert.match(stdout, /pass 1$/m);
	assert.match(stdout, /fail 0$/m);
	assert.match(stdout, /cancelled 0$/m);
	assert.strictEqual(child.status, 1);
	assert.strictEqual(child.signal, null);
	}

I'm taking a look!

[mcollina]

lgtm

[github-actions]

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

[inukshuk]

I'm using the test-runner in Electron, where usually isolation=none is the preferred way to use it (especially in the renderer process). It would be great to have this merged. Currently, we have to jump through hoops, like emitting a fake 'beforeExit' after the tests to get the runner to shut down in isolation.

#62269 is also related.

[pmarchini]

Hey @inukshuk, I’ll take another look at the issue ASAP.

The PR got stuck due to some unexpected changes in behavior

[nodejs-github-bot]

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

[aduh95]

CI is failing