[node:test] Infinite loop occurs when `files` is empty #48823

koh110 · 2023-07-18T07:13:38Z

Version

v20.4.0

Platform

Darwin MacBook-Pro.local 22.5.0 Darwin Kernel Version 22.5.0: Thu Jun 8 22:22:20 PDT 2023; root:xnu-8796.121.3~7/RELEASE_ARM64_T6000 arm64

Subsystem

No response

What steps will reproduce the bug?

This code occurs infinite loop.

import { run } from 'node:test' 
import { tap } from 'node:test/reporters';

const stream = run({
  // files: undefined
}).compose(tap);

stream.pipe(process.stdout);

How often does it reproduce? Is there a required condition?

everytime

What is the expected behavior? Why is that the expected behavior?

Default is written astest runner execution model. But it does not work.
https://nodejs.org/api/test.html#runoptions

files: An array containing the list of files to run. Default matching files from test runner execution model

It seems that tests that are run during node --test should be run when files is empty.

What do you see instead?

https://github.com/koh110/minimum-nodejs-test

$ node test-run-null-files.mjs
TAP version 13
# TAP version 13
# \# TAP version 13
# \# \\\# TAP version 13
# \# \\\# \\\\\\\# TAP version 13
# \# \\\# \\\\\\\# \\\\\\\\\\\\\\\# TAP version 13
# \# \\\# \\\\\\\# \\\\\\\\\\\\\\\# \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\# TAP version 13

Additional information

createTestFileList gets its own file path.
It seems that it have to exclude own file path when executing in run.

const hasUserSuppliedPattern = process.argv.length > 1;
const patterns = hasUserSuppliedPattern ? ArrayPrototypeSlice(process.argv, 1) : [kDefaultPattern];
console.log('createTestFileList:', hasUserSuppliedPattern, patterns);

node/lib/internal/test_runner/runner.js

Lines 90 to 92 in 9988130

    
           const cwd = process.cwd(); 
        
           const hasUserSuppliedPattern = process.argv.length > 1; 
        
           const patterns = hasUserSuppliedPattern ? ArrayPrototypeSlice(process.argv, 1) : [kDefaultPattern];

let testFiles = files ?? createTestFileList();

if (shard) {
  testFiles = ArrayPrototypeFilter(testFiles, (_, index) => index % shard.total === shard.index - 1);
}
console.log('testFiles: ', testFiles);

node/lib/internal/test_runner/runner.js

Lines 473 to 477 in 9988130

    
           let testFiles = files ?? createTestFileList(); 
        
           if (shard) { 
        
             testFiles = ArrayPrototypeFilter(testFiles, (_, index) => index % shard.total === shard.index - 1); 
        
           }

$ ~/dev/node/out/Release/node ./test-run-null-files.mjs
createTestFileList: true [ '/xxx/minimum-nodejs-test/test-run-null-files.mjs' ]
testFiles:  [ '/xxx/minimum-nodejs-test/test-run-null-files.mjs' ]
TAP version 13
# createTestFileList: true [ '/xxx/minimum-nodejs-test/test-run-null-files.mjs' ]
# testFiles:  [ '/xxx/minimum-nodejs-test/test-run-null-files.mjs' ]
# TAP version 13

The text was updated successfully, but these errors were encountered:

MoLow · 2023-07-18T07:44:58Z

I am not sure I would consider this a bug, you should either specify files or name the root file in some way that the default execution model won't detect it as a test file.
and, due to the fact the the default will change to be a glob pattern starting node 21 - I don't think it should be excluded in any way

koh110 · 2023-07-18T09:13:42Z

Does it mean that the files should be specified? Or does it mean that it should not be excluded own file path?

I don't think it should be excluded in any way

Would you prefer to modify the document in that case?

Default is written astest runner execution model

MoLow · 2023-07-18T09:18:47Z

the file calling run should not be excluded

koh110 · 2023-07-18T09:27:30Z

So, I think Default matching in the document should be removed. What do you think about it?

MoLow · 2023-07-18T10:09:34Z

why? it is still correct

koh110 · 2023-07-18T10:42:41Z

I think Default is the initial value that works that way if not specified.
For example, the initial values for concurrency and inspectPort, timeout are described in the document.
However, files will not work if it is excluded.
I think this is different compared to other Default.

So I think it should state that it cannot be omitted instead of Default.

koh110 · 2023-07-22T12:55:39Z

How do I call the default file pattern like node --test for run?

MoLow · 2023-07-22T21:58:22Z

run({file: undefined}) or just run({}) will use the default behavior

koh110 · 2023-07-23T07:26:40Z

When I run run({file: undefined}) or run({}), it's occurs Infinite loop. I want to solve it.

So, I think options.files has bug or Default in doc is not correct, when i create this issue.

Could you please exec this code?

$ node -v
v20.4.0

$ tree
.
└── run.js

$ cat run.js
require('node:test').run({ files: undefined }).compose(require('node:test/reporters').tap).pipe(process.stdout)

# this code occurs infinite loop
$ node run.js

run.js is included in process.argv in createFiles if i set files: undefined. It seems to refer to itself.
https://github.com/nodejs/node/blob/99881304c47138e09a06e6b2bd876cfdf840047e/lib/internal/test_runner/runner.js#L89C1-L93

This happens with any file name.
My first sample code with a file name starting with test- was bad. sorry.

koshic · 2023-07-23T23:14:13Z

run({file: undefined}) or just run({}) will use the default behavior

Is that mean that 'node run.js' (which files: undefined) is equal to 'node --test run.js', as @koh110 explained? I don't think that such 'expected' behaviour is really expected. We should have note in docs, or some asserts in test-runner code to brake the loop.

koh110 · 2023-07-28T06:01:11Z

@MoLow Hi! Could you please help me with what is wrong with this sample code?

@koshic Thanks for the supplement!

MoLow · 2023-07-28T09:52:15Z

@koh110 if you experience an infinate loop - that means the file called by run has a name that is detected as a test file (as described here - https://nodejs.org/api/test.html#test-runner-execution-model it probably has the word test in its name.
you can avoid that by changing the file name or not using the default files argument

koh110 · 2023-07-28T10:02:51Z

@MoLow It occurs with any file name. Could you please run this sample?
#48823 (comment)
This sample file is named run.js and it is located in the root directory.
I think this is not part of the pattern.

koh110 · 2023-07-28T10:21:18Z

run call createTestFileList when options.files is empty.

let testFiles = files ?? createTestFileList();

But hasUserSuppliedPattern in createTestFileList is always true when node --test run.js.

function createTestFileList() {
  const cwd = process.cwd();
  const hasUserSuppliedPattern = process.argv.length > 1;
  const patterns = hasUserSuppliedPattern ? ArrayPrototypeSlice(process.argv, 1) : [kDefaultPattern];

process.argv cannot be empty when trying to call a file containing run.

So, I think createTestFileList always returns path of run that named like root/a.js, root/b.js...

MoLow · 2023-07-28T12:51:47Z

you should not be running run.js with --test

koh110 · 2023-07-29T14:31:23Z

@MoLow It gives the same result without --test. node run.js occurs Infinite loop.
It seems difficult to tell. Can you give me a sample that can be run or Should I write a test and submit a PR for this pattern?

cjihrig · 2023-07-29T15:20:10Z

One way we could address this with a semver major change is to make options.files a required option. That would eliminate any surprise behavior.

koh110 · 2023-07-29T15:29:12Z

@cjihrig Do you mean that the default file pattern like node --test cannot be called from run?
#48823 (comment)

cjihrig · 2023-07-29T15:31:08Z

Correct. Although we could expose an API to get that list.

koh110 · 2023-07-29T15:38:55Z

@cjihrig Thanks. I understand thanks to your answer.
I look forward to seeing this issue resolved.

Should I send a PR to require options.files in the docs?

cjihrig · 2023-07-29T15:50:41Z

Should I send a PR to require options.files in the docs?

Making it a required argument would also require a code change, but first let's see how other collaborators feel. There isn't really a rush because it's a breaking change so it wouldn't be released until October anyway.

koh110 · 2023-07-31T09:13:05Z

OK, thanks.
I would like to create a draft PR how other collaborators feel. Is it more appropriate to create new issue?

MoLow · 2023-08-02T07:34:53Z

Should I send a PR to require options.files in the docs?

Making it a required argument would also require a code change, but first let's see how other collaborators feel. There isn't really a rush because it's a breaking change so it wouldn't be released until October anyway.

the next semver release will include glob support, and until we expose fs.glob (wich I am working on doing) - I dont think we should make this required

MoLow · 2023-08-02T07:36:29Z

hoever we can easily detect if run is being called when process.env.NODE_TEST_CONTEXT is set and warn/stop the execution

koh110 · 2023-08-06T15:31:53Z

It's sounds good for me. If you give me the sample, I will make a PR.

hoever we can easily detect if run is being called when process.env.NODE_TEST_CONTEXT is set and warn/stop the execution

pulkit-30 · 2023-11-28T18:48:13Z

I would like to work on this.
making options.files a required option in run() seems a good solution for this (ref ).
WDYT @MoLow.

MoLow · 2023-11-29T10:16:19Z

It should not be required - it has a default value

PR-URL: #51047 Fixes: #48823 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: James M Snell <jasnell@gmail.com>

MoLow added the test_runner Issues and PRs related to the test runner subsystem. label Jul 18, 2023

koh110 mentioned this issue Jul 20, 2023

test_runner: do not spawn run in child processes to prevent infinite loops #48849

Closed

pulkit-30 mentioned this issue Dec 4, 2023

test_runner: fix infinite loop when files are undefined in test runner #51047

Merged

nodejs-github-bot closed this as completed in #51047 Dec 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[node:test] Infinite loop occurs when `files` is empty #48823

[node:test] Infinite loop occurs when `files` is empty #48823

koh110 commented Jul 18, 2023 •

edited

Loading

MoLow commented Jul 18, 2023

koh110 commented Jul 18, 2023 •

edited

Loading

MoLow commented Jul 18, 2023

koh110 commented Jul 18, 2023

MoLow commented Jul 18, 2023

koh110 commented Jul 18, 2023 •

edited

Loading

koh110 commented Jul 22, 2023

MoLow commented Jul 22, 2023

koh110 commented Jul 23, 2023 •

edited

Loading

koshic commented Jul 23, 2023

koh110 commented Jul 28, 2023

MoLow commented Jul 28, 2023

koh110 commented Jul 28, 2023

koh110 commented Jul 28, 2023 •

edited

Loading

MoLow commented Jul 28, 2023

koh110 commented Jul 29, 2023 •

edited

Loading

cjihrig commented Jul 29, 2023

koh110 commented Jul 29, 2023 •

edited

Loading

cjihrig commented Jul 29, 2023

koh110 commented Jul 29, 2023 •

edited

Loading

cjihrig commented Jul 29, 2023

koh110 commented Jul 31, 2023 •

edited

Loading

MoLow commented Aug 2, 2023

MoLow commented Aug 2, 2023

koh110 commented Aug 6, 2023

pulkit-30 commented Nov 28, 2023

MoLow commented Nov 29, 2023

[node:test] Infinite loop occurs when files is empty #48823

[node:test] Infinite loop occurs when files is empty #48823

Comments

koh110 commented Jul 18, 2023 • edited Loading

Version

Platform

Subsystem

What steps will reproduce the bug?

How often does it reproduce? Is there a required condition?

What is the expected behavior? Why is that the expected behavior?

What do you see instead?

Additional information

MoLow commented Jul 18, 2023

koh110 commented Jul 18, 2023 • edited Loading

MoLow commented Jul 18, 2023

koh110 commented Jul 18, 2023

MoLow commented Jul 18, 2023

koh110 commented Jul 18, 2023 • edited Loading

koh110 commented Jul 22, 2023

MoLow commented Jul 22, 2023

koh110 commented Jul 23, 2023 • edited Loading

koshic commented Jul 23, 2023

koh110 commented Jul 28, 2023

MoLow commented Jul 28, 2023

koh110 commented Jul 28, 2023

koh110 commented Jul 28, 2023 • edited Loading

MoLow commented Jul 28, 2023

koh110 commented Jul 29, 2023 • edited Loading

cjihrig commented Jul 29, 2023

koh110 commented Jul 29, 2023 • edited Loading

cjihrig commented Jul 29, 2023

koh110 commented Jul 29, 2023 • edited Loading

cjihrig commented Jul 29, 2023

koh110 commented Jul 31, 2023 • edited Loading

MoLow commented Aug 2, 2023

MoLow commented Aug 2, 2023

koh110 commented Aug 6, 2023

pulkit-30 commented Nov 28, 2023

MoLow commented Nov 29, 2023

[node:test] Infinite loop occurs when `files` is empty #48823

[node:test] Infinite loop occurs when `files` is empty #48823

koh110 commented Jul 18, 2023 •

edited

Loading

koh110 commented Jul 18, 2023 •

edited

Loading

koh110 commented Jul 18, 2023 •

edited

Loading

koh110 commented Jul 23, 2023 •

edited

Loading

koh110 commented Jul 28, 2023 •

edited

Loading

koh110 commented Jul 29, 2023 •

edited

Loading

koh110 commented Jul 29, 2023 •

edited

Loading

koh110 commented Jul 29, 2023 •

edited

Loading

koh110 commented Jul 31, 2023 •

edited

Loading