test: add --test-files-glob flag

[p-mcgowan]

test: add --test-files-glob flag

Overrides the default kDefaultPattern for test file globs to allow for
default file pattern matching, while still allowing other options or
single, targeted file names for testing.

This also might resolve issues with the defaults when using strip-types
since the defaults can, currently, only be overridden using the positional
arguments (see #56546).

Fixes: #51384
Ref: #56546

I believe this is a notable-change, I'll leave that up the reviewers though.

I also considered modifying the arg parser when the --test flag is present to continue parsing options that come after positionals.
I think it's still (maybe) a decent solution (even if it behaves differently than other commands) since the test positionals are globs instead of files (like most other commands).

Happy to cleanup and push that up alternatively if that's preferred, or try something else if neither work - would like to resolve the current issues so that the default runner is a drop-in replacement for everything else.

Also note: still getting #44003 locally, I believe the solution is to enable another interface but I didn't dig too far into it, admittedly.

Failed tests:
out/Release/node --expose-internals --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout /home/patrickmcgowan/source/p-mcgowan/node/test/parallel/test-http2-invalid-last-stream-id.js
out/Release/node --expose-internals --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout /home/patrickmcgowan/source/p-mcgowan/node/test/parallel/test-http2-premature-close.js
out/Release/node --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout /home/patrickmcgowan/source/p-mcgowan/node/test/parallel/test-net-socket-connect-without-cb.js
out/Release/node --expose-internals --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout /home/patrickmcgowan/source/p-mcgowan/node/test/parallel/test-tcp-wrap-listen.js
make[1]: *** [Makefile:315: jstest] Error 1
make: *** [Makefile:342: test] Error 2

[nodejs-github-bot]

Review requested:

• @nodejs/config
• @nodejs/test_runner

[p-mcgowan]

Just adding tests now that I forgot for the changed test-coverage behaviour.

[marco-ippolito]

Thanks for the PR can you add a empty line at the of the fixtures?

[p-mcgowan]

Yeah I was wondering about that - my editor is using the top-level .editorconfig which has:

[{test/fixtures,deps,tools/eslint/node_modules,tools/gyp,tools/icu,tools/msvs}/**]
insert_final_newline = false

Should I also remove the js fixtures from that glob? Or just manually add the newline and leave .editorconfig alone?

[p-mcgowan]

I've just added the newlines manually - I will say it's out of scope to fix the editorconfig / fixture files (that could be a lit-fix for someone else I think).

I've also fixed the js lint and commit message failing runs, and added test for the coverage stuff that the flag affects.

[marco-ippolito]

How is different from users passing the positional glob argument?

[p-mcgowan]

How is different from users passing the positional glob argument?

In short: it's an option instead of positional, which sounds a bit stupid but actually solves some problems.

Because the cli splits argv and execArgv when the first positional is hit, flags need to come before positionals.
If you don't want to use the default glob (eg don't run all files in the test/ folder unless they end with .spec), there is no way to override it without passing positionals, so this provides a way to override the default without changing the cli parsing behaviour.

I mentioned that it seems duplicated in the comment on #51384 and suggested alternatives in another related ticket, but ultimately it provides a solution (even if only until a better one comes along) to 2 things:

• it allows for people to set "positional globs" in the options, which then allow for more options afterwards or globs (solving --test-name-pattern needing to come before filenames is hostile to npm scripts #51384)
• it also provides a solution to some issues where the default glob patterns match files that shouldn't be matched (eg .ts files) that is unexpected.

So it largely serves the same purpose, but as an option instead of positional it means you can do something like default the glob to dist/**/*.spec.js and then pass in --test-only afterwards.

I'm open to other possible solutions, I just wanted to get something in and working but I think this also brings it more inline to how people are familiar with using test runners (mocha, vite, deno, etc). Or rather, the original proposed solution on #51384 that I created a different branch for was closer to that (simply continuing to parse cli flags after seeing positionals) but I felt that was too disruptive a change, but I'd honestly prefer that option.

My expectation (possibly incorrectly) is that node --test 'dist/**/*.spec.js' --test-only would just use the positional glob as defaults, then .only applies when focusing a single test.
So I figured 2 options: new, semi-duplicated option and not changing the argv/execArgv cli parsing, or change the cli parsing only for --test which felt like it might be harder to land.

[p-mcgowan]

Actually fixed the commit message this time, and ran core-validate-commit to tripple check.

[p-mcgowan]

Actually to expand on the above reasoning:

The original idea was to continue to parse node flags after positionals when --test was set, but this introduced 2 new issues:

1. It meant that the cli handles arguments differently when using --test, which was an inconsistency that felt kind of wrong.
2. It meant that even though you could provide a default glob now and options after, single files would just add to the list.

For example, with an npm script "test": "NODE_ENV=test node --test --experimental-strip-types 'test/**/*.spec.ts'" (or any other default flags you might be using), if you ran npm test -- --test-only it would work, but if you ran npm test some/single/file.ts it would execute all tests, and the one provided.

So the core issue is not that positionals have to come after flags, but that there is no other way to override the default glob other than positionals. So by providing that as an option, the npm script would then be "test": "NODE_ENV=test node --test --experimental-strip-types --test-files-glob='test/**/*.spec.ts'", and npm test -- --test-only works as well as npm test some/single/file.ts.

[p-mcgowan]

I'm not sure why some actions appear to be failing, am I not able to run them on my fork for testing?
Edit: found it
Edit2: found the issue - test reporter output has extra spaces in the table (| 100.00 | ',), which I trimmed unknowingly

[codecov]

Codecov Report

Attention: Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.

Project coverage is 89.56%. Comparing base (eaebfab) to head (00192a6).

Files with missing lines	Patch %	Lines
lib/internal/test_runner/utils.js	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #58860      +/-   ##
==========================================
- Coverage   90.09%   89.56%   -0.54%     
==========================================
  Files         640      640              
  Lines      188450   188458       +8     
  Branches    36966    36664     -302     
==========================================
- Hits       169789   168790     -999     
- Misses      11364    12333     +969     
- Partials     7297     7335      +38     

Files with missing lines	Coverage Δ
lib/internal/test_runner/runner.js	92.72% <100.00%> (+0.04%)	⬆️
src/node_options.cc	84.48% <100.00%> (+0.01%)	⬆️
src/node_options.h	97.86% <ø> (ø)
lib/internal/test_runner/utils.js	60.59% <66.66%> (+0.12%)	⬆️

... and 100 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.

[p-mcgowan]

Not sure why codecov is reporting that check as uncovered, I wrote tests specifically for it. Grep'd the logs, looks like all possible branches are either already covered, or covered by one of the 2 new tests.

Maybe because it's not rebased off upstream/main any more - will rebase again just in case.

[marco-ippolito]

Suggested change

	AddOption("--test-files-glob",
	"set the default glob pattern for matching test files (default: "
	"'**/{test,test/**/*,test-*,*[._-]test}.{<extensions>}' where "
	"<extensions> is 'js,mjs,cjs' or 'js,mjs,cjs,ts,mts,cts' when using"
	" --experimental-strip-types)",
	&EnvironmentOptions::test_files_glob,
	kAllowedInEnvvar,
	OptionNamespaces::kTestRunnerNamespace);
	AddOption("--test-files-glob",
	"set the default glob pattern for matching test files",
	&EnvironmentOptions::test_files_glob,
	kAllowedInEnvvar,
	OptionNamespaces::kTestRunnerNamespace);

[marco-ippolito]

The man needs to be updated too https://github.com/nodejs/node/blob/main/doc/node.1

[p-mcgowan]

Awesome, thanks. I don't think I knew node had a manpage...

It's also not clear in the manpage which options that take an argument should be documented as such - it does not appear that every option taking an argument has an associated arg, so I went with copying the style of the nearest similar option. I would have assumed every option that takes an argument should be shown as such but maybe there are reasons not to.

Happy to change it.