test runner run() ignores testNamePatterns and testSkipPatterns when isolation is "none"

[galdebert]

Version

v22.14.0, v23.8.0

Platform

Microsoft Windows NT 10.0.19045.0 x64

Subsystem

test

What steps will reproduce the bug?

mytest.test.mjs

import { test } from "node:test";

test("a1", (t) => {
  console.log(t.name);
});

test("a2", (t) => {
  console.log(t.name);
});

test("b1", (t) => {
  console.log(t.name);
});

run.mjs

import * as test from "node:test";
import { spec } from "node:test/reporters";

test
  .run({
    globPatterns: ["mytest.test.mjs"],
    testNamePatterns: ["a"],
    testSkipPatterns: ["a2"],
    isolation: "none",
  })
  .compose(spec)
  .pipe(process.stdout);

output

a1
a2
b1
✔ a1 (1.1461ms)
✔ a2 (0.2373ms)
✔ b1 (0.2315ms)

that is incorrect, only a1 should run.

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

always

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

expected behavior is to run only a1

a1
✔ a1 (1.1336ms)

because

• a2 should be skipped by testSkipPatterns: ["a2"]
• b1 should be discarded by testNamePatterns: ["a"]

What do you see instead?

a1
a2
b1
✔ a1 (1.1461ms)
✔ a2 (0.2373ms)
✔ b1 (0.2315ms)

Additional information

Using isolation: "process" has the expected behavior

repor code: https://github.com/galdebert/repro-bug-node-test-runner

[cecia234]

Hi, first time contributor here, so take my words with a grain of salt.
I was able to reproduce the problem and, unless intended, I think that the problem lies in how the tests are launched when isolation is set to none.
In particular, the filtering based on testNamePatterns and testSkipPatterns is done inside Test.start() method (

node/lib/internal/test_runner/test.js

Line 929 in dba6fdc

this.applyFilters();

), calling Test.applyFilters().
The problem is that Test.start() is called only when isolation is set to process (

node/lib/internal/test_runner/runner.js

Line 456 in dba6fdc

const subTestEnded = subtest.start();

), since when isolation==='none' tests are ran with Test.run() (

node/lib/internal/test_runner/test.js

Line 746 in dba6fdc

await test.run();

) (note that Test.run() is called inside Test.start()).

If someone with more experience could confirm that this is the problem I'd gladly submit a PR to fix the issue.

[pmarchini]

Hey @cecia234,
Based on the documentation, I don't see any reason why it wouldn't work!
I would suggest starting by verifying these tests:
https://github.com/nodejs/node/blob/main/test/parallel/test-runner-no-isolation-filtering.js.

Something here feels off to me:

node/test/parallel/test-runner-no-isolation-filtering.js

Lines 56 to 72 in 1b655c7

	test('works with --test-name-pattern', () => {
	const args = [
	'--test',
	'--test-reporter=tap',
	'--test-isolation=none',
	'--test-name-pattern=/test one/',
	fixture1,
	fixture2,
	];
	const child = spawnSync(process.execPath, args);
	const stdout = child.stdout.toString();
	assert.strictEqual(child.status, 0);
	assert.strictEqual(child.signal, null);
	assert.match(stdout, /# tests 0/);
	assert.match(stdout, /# suites 0/);
	});

I would expect assert.match(stdout, /# tests 0/); to fail.

If you want to try opening a PR, it's more than welcome 🚀

One suggestion: cover the missing cases in the tests and also add some tests to test/parallel/test-runner-run.mjs.

[cecia234]

Hi @pmarchini, thanks for all the hints! I'll check everything out on these days 😊

[danielmbrasil]

Hey @cecia234, are you currently working on this issue? If not, I'd like to give it a try! Let me know.

[cecia234]

Hi @danielmbrasil , yes I'm looking at it in this exact moment :) you can still try if you want!

EDIT: If you need some more info, calling the test file in the issue example from cli with the same parameters (e.g. node --test --test-reporter=tap --test-name-pattern=a --test-skip-pattern=a2) yields the correct result. Calling it as node run-isolation-none.mjs yields the wrong result.

[cecia234]

@pmarchini regarding the test that seemed wrong: calling node --test with isolation set to none automatically runs tests with only flag set to true. In our case this results in 0 tests being called, since the ones marked with {only: true} do not match the --test-name-pattern value. So no problem on that side.

[pmarchini]

@pmarchini regarding the test that seemed wrong: calling node --test with isolation set to none automatically runs tests with only flag set to true. In our case this results in 0 tests being called, since the ones marked with {only: true} do not match the --test-name-pattern value. So no problem on that side.

OK, thanks, I had only a quick look!
Could I please ask you to clarify it via a comment maybe? Just to increase clarity ☺️

[cecia234]

Hi @danielmbrasil, just want to tell you that you can work on this issue. I couldn't fix it these days. For some reason breakpoints do not trigger when I run the provided example, so I had some difficulties debugging (they do trigger for tests already present in tests/parallel folder though).
I created a test case in test-runner-no-isolation-filtering.js calling the executable like this

[ 
     '--test', 
     '--test-reporter=tap', 
     '--test-isolation=none', 
     '--test-name-pattern=/a/', 
     '--test-skip-pattern=/a2/', 
     testfile, // this is a file like https://github.com/galdebert/repro-bug-node-test-runner/blob/main/mytest.test.mjs
 ]

The tests run correctly in this setup. However, the issue arises when tests are executed using test.run(configuration), as seen in https://github.com/galdebert/repro-bug-node-test-runner/blob/main/run-isolation-none.mjs.
I won't be able to look at it until next week, so it's all yours if you want it 🙂

[danielmbrasil]

Hi @cecia234, thanks for the heads-up and for sharing all the information, that will be helpful.

[danielmbrasil]

@pmarchini
I noticed a key difference between using testNamePatterns and skipNamePatterns via CLI versus invoking test.run directly. When invoking run, these patterns are not passed to the globalOptions object before the test tree is created. However, when running tests via CLI, these options are included through the parseCommandLine() function spread.

node/lib/internal/test_runner/runner.js

Lines 668 to 682 in a46af03

	const globalOptions = {
	__proto__: null,
	// parseCommandLine() should not be used here. However, The existing run()
	// behavior has relied on it, so removing it must be done in a semver major.
	...parseCommandLine(),
	setup, // This line can be removed when parseCommandLine() is removed here.
	coverage,
	coverageExcludeGlobs,
	coverageIncludeGlobs,
	lineCoverage: lineCoverage,
	branchCoverage: branchCoverage,
	functionCoverage: functionCoverage,
	cwd,
	};
	const root = createTestTree(rootTestOptions, globalOptions);

When running with isolation set to 'process', the runner makes use of the runTestFile function, which handles testNamePatterns and testSkipPatterns via the args parameter passed to spawn. When isolation is none, the runner does not use runTestFile.

node/lib/internal/test_runner/runner.js

Lines 377 to 379 in a46af03

	const child = spawn(
	process.execPath, args,
	{

In this scenario, setting these patterns as global options isn’t necessary. In fact, doing so causes some tests to fail when isolation is set to 'process'.

To address this, I tried adding those arguments to globalOptions only when isolation is none:

diff --git a/lib/internal/test_runner/runner.js b/lib/internal/test_runner/runner.js
index 733a745a49..fe5ef0d376 100644
--- a/lib/internal/test_runner/runner.js
+++ b/lib/internal/test_runner/runner.js
@@ -679,6 +679,17 @@ function run(options = kEmptyObject) {
     functionCoverage: functionCoverage,
     cwd,
   };
+
+  if (isolation === 'none') {
+    if (testNamePatterns && !globalOptions.testNamePatterns) {
+      globalOptions.testNamePatterns = testNamePatterns
+    }
+
+    if (testSkipPatterns && !globalOptions.testSkipPatterns) {
+      globalOptions.testSkipPatterns = testSkipPatterns
+    }
+  }
+
   const root = createTestTree(rootTestOptions, globalOptions);
   let testFiles = files ?? createTestFileList(globPatterns, cwd);

This seems to fix the issue, but I feel like it's not ideal and could be improved. Do you have any suggestions for a better solution? Thanks!

[pmarchini]

hey @danielmbrasil, I'm sorry but I totally missed your latest comment 🤕
Would you like to try opening a PR as it seems you already have isolated the issue ? 🚀