fix: include uncovered files in report when using --all with --include#589
Open
wellwelwel wants to merge 10 commits intobcoe:mainfrom
Open
fix: include uncovered files in report when using --all with --include#589wellwelwel wants to merge 10 commits intobcoe:mainfrom
--all with --include#589wellwelwel wants to merge 10 commits intobcoe:mainfrom
Conversation
Author
|
Just as a proof of concept, here is the current test failure without the fix applied: 2 failing
1) c8
--all
should only track files that pass shouldInstrument in fileIndex:
AssertionError: expected true to equal false
+ expected - actual
-true
+false
at Context.<anonymous> (test/integration.js:543:44)
at process.processImmediate (node:internal/timers:504:21)
2) c8 mergeAsync
--all
should only track files that pass shouldInstrument in fileIndex:
AssertionError: expected true to equal false
+ expected - actual
-true
+false
at Context.<anonymous> (test/integration.js:543:44)
at process.processImmediate (node:internal/timers:504:21) |
--all with --include
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Checklist
npm test, tests passingnpm run test:snap(to update the snapshot)Fixes #588.
In
lib/report.js, the_normalizeProcessCovmethod adds files tofileIndexbefore checkingshouldInstrument():Then:
lib/results_stream.jsduring execution (viarequire()chain)fileIndex.add(v8ScriptCov.url)is called unconditionallyshouldInstrument()returnsfalse, so the file is not added toresult_includeUncoveredFilesfinds the file via glob, butfileIndex.has()istrue, so it's skipped ❌The fix moves
fileIndex.add()to inside theshouldInstrumentblock, so only files that actually pass the filter are tracked.About the test approach
The test uses
createReportand_normalizeProcessCovdirectly instead of thespawnSync+matchSnapshotpattern used in most integration tests.Since
test-exclude'sglobSyncinternally callsshouldInstrumenton every file it returns, the paths that failshouldInstrumentin_normalizeProcessCovwon't collide with the paths fromglobSyncin_includeUncoveredFiles. Because of that, a regular integration test would pass with or without the fix, so I couldn't find a way to catch a regression through the CLI alone, for example: wellwelwel#1 (comment).By calling
_normalizeProcessCovdirectly with fake V8 coverage entries, we can verify thatfileIndexis only populated with files that passshouldInstrument.Note
I can also move this test to a separate file for unit tests if that makes more sense 🙋🏻♂️