test_runner: exclude test files from coverage by default

[pmarchini]

This should address #53508.

I'm opening this PR since #55633 seems to be stale.

This PR contains the feature and some changes to reduce its footprint.

Note: The reason behind lazyMinimatch is to avoid experimental warnings related to matchesGlob.

I'm not entirely convinced about the change I introduced to the --test-coverage-exclude and --test-coverage-include flags.

Regarding: @cjihrig https://github.com/nodejs/node/pull/55633#pullrequestreview-2410163982

Another thing that is not clear to me: what happens if someone runs a test file, but that test file imports another file that also includes tests? Should we exclude that imported file from the coverage report, or should we only exclude the files that the user specified?

I think we should just exclude the files specified by the user, as it's the most common scenario. If a user imports other tests from different files, I would expect them to exclude those manually. WDYT?

[nodejs-github-bot]

Review requested:

• [ ] @nodejs/test_runner

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 89.13043% with 5 lines in your changes missing coverage. Please review.

Project coverage is 88.53%. Comparing base (ce346b6) to head (70be970). Report is 473 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/test_runner/utils.js	0.00%	5 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #56060      +/-   ##
==========================================
+ Coverage   88.00%   88.53%   +0.52%     
==========================================
  Files         653      657       +4     
  Lines      188093   190226    +2133     
  Branches    35942    36521     +579     
==========================================
+ Hits       165537   168420    +2883     
+ Misses      15734    14986     -748     
+ Partials     6822     6820       -2     

Files with missing lines	Coverage Δ
lib/internal/fs/glob.js	92.63% <100.00%> (+1.93%)	:arrow_up:
lib/internal/test_runner/coverage.js	64.71% <100.00%> (+0.36%)	:arrow_up:
lib/path.js	96.04% <100.00%> (+0.82%)	:arrow_up:
lib/internal/test_runner/utils.js	56.20% <0.00%> (-0.47%)	:arrow_down:

... and 148 files with indirect coverage changes

[JakobJingleheimer]

I think we should just exclude the files specified by the user, as it's the most common scenario. If a user imports other tests from different files, I would expect them to exclude those manually. WDYT?

I agree :)

[pmarchini]

I just pushed a new implementation for this feature based on default coverageExcludeGlobs value.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 5ad2ca920cdc6d99a8d673724b35115fef925e78

[Llorx]

I think that this one's footprint is worse than the previous one as it touches a lot of different files outside of the test runner folder, like fs and path. I don't know why the other one went stale actually. Noone replied anything and then I found another PR which adresses the same feature but with a more scattered footprint. Being my first PR I was happy about following any indication, it was constructive for me, but then I find this... :-(

Apart from the footprint being too scattered (personal opinion), it doesn't seem that it excludes test files by default. It excludes a matching pattern by default, but if I change the globPatterns to include different patterns, those files are not excluded, which breaks the "exclude test files by default" feature. This is more a "there's a exclude pattern by default" but not "exclude test files by default".

Also, it doesn't exclude a test file ran by cli by default, and if you pass a test file array programmatically neither. That's why my approach was moving the final calculated testfiles array around, so it actually excludes test files by default. This PR and the other PR doesn't address the same feature.

[pmarchini]

Hey @Llorx, I proceeded as I saw no activity. I meant no offense 😞

Regarding the implementation footprint, the rationale behind the fs and path changes is that we decided to avoid a warning message related to glob usage.

The implementation follows the same logic as other tools, such as c8, and is based on the default exclusion mechanisms.

[Llorx]

Thank you for replying @pmarchini

I see your point now, specially the footprint. Didn't read this thread actually, just the diff and went directly to comment. Hot blooded I guess as I was hyped to be a small collaborator. To "have my bit of code in nodejs" haha. Sorry about that.

I understand the include/exclude logic as other tools. Is just an opinion as I prefer for a tool to be the less verbose as possible in a normal usage. For example, if I do a default test with the default options I see that the test files are not included in the coverage output (which is something expected, I guess), but if I change the include pattern for whatever reason I notice that they are included (not that expected as it differs from the default behaviour, and the coverage output is a feature directly linked to running tests), so I have to search why, and then add more arguments which they are going to be an exact copy of the files include pattern, which makes me feel that the coverage output is not that linked to the test runner anymore, like it is a different tool that needs its own parameters, although it is listed inside the test runner documentation. Obviously that's an opinion and I'm no collaborator, so I don't know what is the philosophy in nodejs about these things.

Maybe an easy way to do this (if is something that fits the nodejs philosophy) but without transfering the test files array along all the testing line down to the coverage one (a mid-point between one PR and the other), the default coverage exclude pattern could be the same pattern as the include pattern provided by the user (that's something that was already pointed before in the original issue), although this leaves running tests directly by the cli or programmatically with the run({files:[...]}) option unattended... but covers the majority of the test runner usage, which is by running using the node --test argument.

IDK just debating. Obviosuly, if my position is not clear enough, I would prefer it this way haha.

[pmarchini]

@Llorx don't worry, I totally understand your position, and I'm sorry you felt that way. I am sure that you will have a chance, and if you would like to contribute, https://github.com/nodejs/node/issues/56316 <-- this is an issue I was planning to work on. If you want to go ahead with it, I'm happy to support you 😁

Regarding setting the value equal to the input could be a solution, but I feel it would be misaligned with other similar tools. That said, I see no reason not to open a PR with your suggestion and see what other collaborators think 🚀

[aduh95]

Backporting on v22.x is blocked on https://github.com/nodejs/node/pull/54705#issuecomment-2626909553