test_runner: skip `--require` for test orchestration process by MoLow · Pull Request #52099 · nodejs/node · GitHub

Review requested:

@nodejs/test_runner

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 () => { 
        
             const file = createTmpFile(); 
        
             const required = createTmpFile('process._rawDebug(\'pid\', process.pid);'); 
        
             const args = ['--require', required, file]; 
        
             const { stdout, pid } = await runWriteSucceed({ file, watchedFile: file, args }); 
        
             const importPid = 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 () => { 
        
             const file = createTmpFile(); 
        
             const imported = "data:text/javascript,process._rawDebug('pid', process.pid);"; 
        
             const args = ['--import', imported, file]; 
        
             const { stdout, pid } = await runWriteSucceed({ file, watchedFile: file, args }); 
        
             const importPid = 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)}`, 
        
             ]); 
        
           });

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

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

Yes but it means that this is a breaking change

Adding label but if you think differently we can discuss

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

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

lgtm

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

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

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.

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

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

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

Can we revisit this PR?

MoLow requested review from atlowChemi, benjamingr and cjihrig March 15, 2024 13:37

nodejs-github-bot added needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. labels Mar 15, 2024

MoLow force-pushed the test-runner-require branch 2 times, most recently from 0fc431e to ee77010 Compare March 15, 2024 13:40

atlowChemi reviewed Mar 15, 2024

View reviewed changes

Comment thread lib/internal/process/pre_execution.js Outdated

atlowChemi reviewed Mar 15, 2024

View reviewed changes

Comment thread test/parallel/test-runner-cli.js Outdated

cjihrig approved these changes Mar 15, 2024

View reviewed changes

Comment thread test/parallel/test-runner-cli.js Outdated

Comment thread lib/internal/process/pre_execution.js Outdated

rluvaton added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 17, 2024

MoLow added the test_runner Issues and PRs related to the test runner subsystem. label Mar 17, 2024

MoLow added 2 commits April 1, 2024 09:52

test_runner: skip --require for test orchestration process

4fd6635

CR

ff52556

MoLow force-pushed the test-runner-require branch from ee77010 to ff52556 Compare April 1, 2024 07:58

MoLow requested review from aduh95, atlowChemi and cjihrig April 1, 2024 07:59

MoLow added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Apr 1, 2024

atlowChemi approved these changes Apr 1, 2024

View reviewed changes

cjihrig approved these changes Apr 1, 2024

View reviewed changes

MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 1, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 1, 2024

MoLow added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Apr 1, 2024

mcollina approved these changes Apr 1, 2024

View reviewed changes

fixup! CR

42049a2

MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 1, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 1, 2024

atlowChemi approved these changes Apr 1, 2024

View reviewed changes

MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 1, 2024

GeoffreyBooth requested changes Apr 1, 2024

View reviewed changes

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 1, 2024

nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Apr 1, 2024

MoLow closed this May 24, 2024

MoLow deleted the test-runner-require branch May 24, 2024 09:01

MoLow restored the test-runner-require branch May 24, 2024 09:01

test_runner: skip `--require` for test orchestration process by MoLow · Pull Request #52099 · nodejs/node · GitHub

Uh oh!

Uh oh!

test_runner: skip --require for test orchestration process - #52099

test_runner: skip --require for test orchestration process#52099MoLow wants to merge 3 commits intonodejs:mainnodejs/node:mainfrom MoLow:test-runner-requireMoLow/node:test-runner-requireCopy head branch name to clipboard

Conversation

MoLow commented Mar 15, 2024

Uh oh!

nodejs-github-bot commented Mar 15, 2024

Uh oh!

MoLow commented Mar 15, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rluvaton commented Mar 16, 2024

Uh oh!

MoLow commented Mar 17, 2024

Uh oh!

rluvaton commented Mar 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 1, 2024

Uh oh!

MoLow commented Apr 1, 2024

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Apr 1, 2024

Uh oh!

nodejs-github-bot commented Apr 1, 2024

Uh oh!

GeoffreyBooth left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Apr 1, 2024

Uh oh!

MoLow commented Apr 2, 2024

Uh oh!

GeoffreyBooth commented Apr 2, 2024

Uh oh!

avivkeller commented Jul 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

test_runner: skip --require for test orchestration process#52099
MoLow wants to merge 3 commits into
nodejs:mainfrom
MoLow:test-runner-require

rluvaton commented Mar 17, 2024 •
edited
Loading

GeoffreyBooth left a comment •
edited
Loading