test_runner: exclude test files from code coverage by default #55633
Conversation
|
Review requested:
|
nodejs-github-bot
added
needs-ci
|
I guess that this is a |
avivkeller
added
notable-change
|
The
notable-change
Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section. |
|
I've added This is breaking change, but since code coverage is experimental, it is not semver-major |
There was a problem hiding this comment.
I'm not sure that this is the right approach. Since we already have coverage excluding functionality, this should probably be more integrated with that. I think that would lead to less invasive changes than this PR currently has.
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?
The problem with that is that if I set the "coverage exclude" based on the "test include", if someone sets an exclude, it is going to include all test files again, and is pretty usual to set a simple exclude for a utils file or some small things.
Very good question and I did not think about that. I would say to exclude that file too by default, but then I have to find another approach. |
I second this opinion. Regardless of the approach, IMO it should follow the same way that node_modules/ files are excluding, that is, exclude after flags, not with. |
|
Everything updated as stated. It is cleaner now :-) One thing I can't resolve easily is a test file that requires another test file which is not directly included in the test globs. I do not have an unhacky way to track this (maybe checking the call stack when the test method is called? I don't like it...). Still, the documentation is pretty clear about the "inluded/excluded test files" now. I think that with this feature (at least in the first version with the known limitation) we have to aim for the majority of the userland, which is usually one test file for a batch of tests. EDIT: There are more file changes because of merging again from master to this PR, as the test_runner was modified while this PR was discussed and it conflicted with the changes... :-/ |
|
Additionally: Can you rebase the merge commits, the github bot doesn't like them. |
In my opinion, this feature isn't really needed, at least in this PR. If that's something that is chosen to be added, I believe it should be its own discussion. For now, we can safely say that the test files themselves are not counted for code coverage. Nice and simple. In the future, this change might be preferable, but for now I don't feel strongly that it's needed. However, if someone feels strongly for it, a theoretical solution would be to add any files that import node:test and run a test to the files list, and this should cover imports as well. But still, IMO a change like that deserves its own PR/discussion. |
Sure! EDIT: Done. Way cleaner now, yep. |
Llorx
force-pushed
the
coverage-exclude-test-files
branch
2 times, most recently
from
cc09908 to
c5c8ea7
Compare
|
Done. Also added the cli entrypoint as a testFile to follow the same logic (and edited the affected tests as the behaviour changed) |
|
The implementation LGTM :-) I've removed needs-CITGM, as this is an experimental feature |
Is not usual to test the code coverage of test files. Fixes: nodejs#53508
Llorx
force-pushed
the
coverage-exclude-test-files
branch
from
9c6e862 to
2df9e6e
Compare
| By default, the files being tested are excluded from code coverage. They can be explicitly | ||
| included via this flag. | ||
|
|
There was a problem hiding this comment.
| By default, the files being tested are excluded from code coverage. They can be explicitly | |
| included via this flag. | |
| By default, the test files are excluded from code coverage. They can be explicitly | |
| included via this flag. |
| @@ -1359,6 +1359,8 @@ changes: | |||
| This property is only applicable when `coverage` was set to `true`. | |||
| If both `coverageExcludeGlobs` and `coverageIncludeGlobs` are provided, | |||
| files must meet **both** criteria to be included in the coverage report. | |||
| By default, the files being tested are excluded from code coverage. They can be explicitly | |||
There was a problem hiding this comment.
| By default, the files being tested are excluded from code coverage. They can be explicitly | |
| By default, the test files are excluded from code coverage. They can be explicitly |
| @@ -61,6 +61,8 @@ for (const coverage of coverages) { | |||
| const result = spawnSync(process.execPath, [ | |||
| '--test', | |||
| '--experimental-test-coverage', | |||
| '--no-warnings', | |||
| '--test-coverage-include=**', | |||
There was a problem hiding this comment.
I have mixed feelings about all these --test-coverage-include=** flags.
IMHO, since we're changing the default behaviour, it might make sense to avoid a large number of tests focused on a 'non-default' behaviour.
Although we could address this in a separate PR, I think it would be better, if it makes sense to you, to handle it here to avoid a "test-refactor only" PR later.
WDYT?
cc @nodejs/test_runner @redyetidev
There was a problem hiding this comment.
I also have mixed feelings about it, as ** would also include anything in node_modules. I'm honestly okay with keeping the test files in the coverage report, and having the user exclude them manually with the exclude flag.
There was a problem hiding this comment.
--test-coverage-include=** was a quick update here as for testing I prefer to update the arrange/act to behave exactly equal as before the update, than modify the assert to handle a different result than before. In this case easier to understand the arrange/act and update it accordingly so it behaves equally as before, than understanding the internals and what result it should output to be correct.
The thing with the filters is that I can't think of any reason a user would want the test files in the coverage report, so we are going to force all the developers to handle a dirty output that they don't actually want, or force them to use the --test-coverage-include flag, but either way is not comfortable. I always liked the premise of nodejs and how convenient it is without too much hassle, not like other languages/frameworks. It usually aims for the majority of user needs, so it is not usual to find yourself fiddling too much as there's high chance that the defaults fit your needs. In the case of the code coverage is the contrary right now.
There was a problem hiding this comment.
In the Node.js test files, the ** is fine.
I've self-addressed my concerns:
My concern was that users that want to include test files would use --test-coverage-include=**, which will also include node_modules/, which they may not want. I suppose it's not that bad for users who, for some reason, need the test files (and only the test files) to use --test-coverage-include=** --test-coverage-exclude=node_modules/.
There was a problem hiding this comment.
Ahhh I see, that's true, and that's not comfortable neither... I think that the more we are talking, the more this is heading to have an exclusive --coverage-include-test-files flag (or similar) that is disabled by default... Makes sense as right now test files is an exclusive option named "testFiles" in the TestCoverage class not linked to the include or exclude in any direct way, but just when applying the filters.
There was a problem hiding this comment.
Maybe, but that could be done in a followup if users show that they need it. For now, maybe this is okay?
There was a problem hiding this comment.
Well, for me it is okay obviosuly because it fits my needs haha. We can see how people reacts to this. This is my first PR so I don't know how this works. This first goes to the non LTS and when people complain/debate/whatever it goes to the LTS if okay, right?
There was a problem hiding this comment.
This first goes to the non LTS and when people complain/debate/whatever it goes to the LTS if okay, right?
Yes, that's how it works. I'm happy to see where this goes.
|
+1 on @cjihrig concerns. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #55633 +/- ##
==========================================
- Coverage 88.43% 87.90% -0.54%
==========================================
Files 654 654
Lines 187720 187724 +4
Branches 36145 35822 -323
==========================================
- Hits 166015 165018 -997
- Misses 14949 15888 +939
- Partials 6756 6818 +62
|
|
It looks like this has been superseded by #56060, so I'll close this out. Thanks for the PR though. |
Is not usual to test the code coverage of test files.
To include test files in code coverage, they must be explicitly included by using
coverageIncludeGlobsor--test-coverage-include.This way, your default code coverage will not include the test files, so you can work out your include/exclude filters based on the first coverage output. I didn't like the idea of suddenly including all the test files just by adding an exclude flag to exclude that_util_file.js that you don't want to test.
What could happen is that you could set an include filter just to test one file, and then you notice that the test file is also being tested. Maybe we can add an extra flag
--include-test-filesor something like that, so the test files are completely invisible to the user until the user wants to explicitly include them. Although I think that this may be too granular and with the--test-coverage-include/excludeparameters is enough.Fixes: #53508