test_runner: skip --require for test orchestration process

[MoLow]

Fixes#48467

[nodejs-github-bot]

Review requested:

• @nodejs/test_runner

[MoLow]

Watch mode already has such tests in place and dosen't need this condition since it passes initializeModules=false to https://github.com/nodejs/node/blob/7002b3a1962e3061c70ac20d2a3ed305557e2023/lib/internal/process/pre_execution.js#L53
test runner must pass initializeModules=true for reporters to work

node/test/sequential/test-watch-mode.mjs

Lines 278 to 310 in 154afbe

	it('should load --require modules in the watched process, and not in the orchestrator process',async()=>{
	constfile=createTmpFile();
	constrequired=createTmpFile('process._rawDebug(\'pid\', process.pid);');
	constargs=['--require',required,file];
	const{ stdout, pid }=awaitrunWriteSucceed({ file,watchedFile: file, args });
	constimportPid=parseInt(stdout[0].split(' ')[1],10);
	assert.notStrictEqual(pid,importPid);
	assert.deepStrictEqual(stdout,[
	'running',
	`Completed running ${inspect(file)}`,
	`Restarting ${inspect(file)}`,
	'running',
	`Completed running ${inspect(file)}`,
	]);
	});
	it('should load --import modules in the watched process, and not in the orchestrator process',async()=>{
	constfile=createTmpFile();
	constimported="data:text/javascript,process._rawDebug('pid', process.pid);";
	constargs=['--import',imported,file];
	const{ stdout, pid }=awaitrunWriteSucceed({ file,watchedFile: file, args });
	constimportPid=parseInt(stdout[0].split(' ')[1],10);
	assert.notStrictEqual(pid,importPid);
	assert.deepStrictEqual(stdout,[
	'running',
	`Completed running ${inspect(file)}`,
	`Restarting ${inspect(file)}`,
	'running',
	`Completed running ${inspect(file)}`,
	]);
	});

[rluvaton]

Wouldn't it break the following

node --require ts-node/register --test-reporter=ts-reporter.ts --test test.js

Not near computer so can't verify at the moment

[MoLow]

Wouldn't it break the following

node --require ts-node/register --test-reporter=ts-reporter.ts --test test.js

Not near computer so can't verify at the moment

It will. I think the usecase of typescript reporters is less common than the issues reported by #48467 - and it can still be solved with --loader

[rluvaton]

Yes but it means that this is a breaking change

Adding label but if you think differently we can discuss

[nodejs-github-bot]

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

[MoLow]

I need another @nodejs/tsc approval since this is marked as semver-major. PTAL

[mcollina]

lgtm

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[GeoffreyBooth]

It will. I think the usecase of typescript reporters is less common than the issues reported by #48467 - and it can still be solved with --loader

--loader is experimental and de facto deprecated in favor of --import. Before this lands, can someone please confirm that the following works (note that both the reporter and the test are TypeScript files):

node --import ts-node/register --test-reporter=ts-reporter.ts --test test.ts

And add a test to that effect? If not, this shouldn’t land as it replaces one bug with another.

Also if the above command works but changing --import to --require causes it to break, well, that’s another bug. I’m not sure if it’s worse than the bug that this PR is aiming to fix, so I’m not sure it’s worth blocking on that point since the remediation would be to use --import, but it’s not like we document that --require always works except when used with --test.

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/52099
✔ Done loading data for nodejs/node/pull/52099
----------------------------------- PR info ------------------------------------
Title test_runner: skip `--require` for test orchestration process (#52099)
Author Moshe Atlow (@MoLow)
Branch MoLow:test-runner-require -> nodejs:main
Labels semver-major, process, author ready, needs-ci, commit-queue-squash, test_runner
Commits 3
- test_runner: skip `--require` for test orchestration process
- CR
- fixup! CR
Committers 1
- Moshe Atlow PR-URL: https://github.com/nodejs/node/pull/52099
Reviewed-By: Colin Ihrig Reviewed-By: Chemi Atlow Reviewed-By: Matteo Collina ------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/52099
Reviewed-By: Colin Ihrig Reviewed-By: Chemi Atlow Reviewed-By: Matteo Collina --------------------------------------------------------------------------------
ℹ This PR was created on Fri, 15 Mar 2024 13:37:00 GMT
✘ Requested Changes: 1
✘ - Geoffrey Booth (@GeoffreyBooth) (TSC): https://github.com/nodejs/node/pull/52099#pullrequestreview-1972231968
✔ Approvals: 3
✔ - Colin Ihrig (@cjihrig): https://github.com/nodejs/node/pull/52099#pullrequestreview-1971512653
✔ - Chemi Atlow (@atlowChemi): https://github.com/nodejs/node/pull/52099#pullrequestreview-1972004444
✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/52099#pullrequestreview-1971723293
✔ Last GitHub CI successful
ℹ Last Full PR CI on 2024-04-01T20:33:31Z: https://ci.nodejs.org/job/node-test-pull-request/58041/
- Querying data for job/node-test-pull-request/58041/
✔ Last Jenkins CI successful
--------------------------------------------------------------------------------
✔ Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/8514547329

[MoLow]

that is a good point. makes me rethink about #48467 (comment)

I can't think of a good reason why preloads need to run for the main process.

registering a module hook/loader is one reason.

any thoughts on a better solution for #48467? thing is a lot of IDE's inject code to connect the debugger which breaks here

[GeoffreyBooth]

I don't know. I'm dealing with a similar problem in #52242.

[avivkeller]

Can we revisit this PR?