test_runner: exclude files with no matching tests from report#64171
test_runner: exclude files with no matching tests from report#64171Han5991 wants to merge 5 commits into
Conversation
Under process isolation, a test file whose tests are all filtered out by --test-name-pattern or --test-skip-pattern was still reported as a spurious passing entry and counted in the test totals. Drop such files from the parent's report, so they are neither shown nor counted. This matches the existing --test-isolation=none behavior. Refs: nodejs#64099 Signed-off-by: sangwook <rewq5991@gmail.com>
|
Review requested:
|
nodejs-github-bot
added
needs-ci
Signed-off-by: sangwook <rewq5991@gmail.com>
Signed-off-by: sangwook <rewq5991@gmail.com>
| const noError = !this.error || this.error.failureType === kSubtestsFailed; | ||
| if (!noError) { | ||
| return false; | ||
| } | ||
| if (this.#reportedChildren > 0) { | ||
| return true; | ||
| } | ||
| return this.root.harness.isFilteringByName; |
There was a problem hiding this comment.
I feel like this changes behavior, if I have a file empty.test.mjs, and it has no tests, it won't be included in the report if I'm filtering by name, but will if I'm not. IMO we should either go all the way or none of the way, not partially.
There was a problem hiding this comment.
Good catch — but I think this actually matches the existing --test-isolation=none behavior rather than introducing a new inconsistency. The same empty file is already filter-dependent there:
node --test --test-isolation=none empty.test.mjs # tests 1
node --test --test-isolation=none --test-name-pattern=x empty... # tests 0
Under isolation=none, an empty file gets a placeholder node (runner.js: "This file had no tests in it. Add the placeholder test."), which is then dropped when isFilteringByName is set — via willBeFilteredByName() → filtered → skipped in report() (test.js). This PR just brings process isolation in line with that, keyed on the same isFilteringByName flag.
So "going all the way" (never reporting zero-test files even without a filter) would change the no-filter behavior of both modes, which felt out of scope here. Happy to take that broader route instead if you'd prefer it — WDYT?
There was a problem hiding this comment.
We should probably discuss the expected behavior at https://github.com/nodejs/test-runner
Assuming this gets the behaviors to match I think this should land as is, and changing the behavior should be handled in a follow up based on discussions there
Signed-off-by: sangwook <rewq5991@gmail.com>
avivkeller
added
the
test_runner-agenda
4 participants
Under process isolation (the default), a test file whose tests are all filtered out by
--test-name-pattern/--test-skip-patternwas reported as a spurious passing entry (✔ file.test.js) and counted in thetests/passtotals. This drops such files from the parent's report so they are neither shown nor counted, matching what--test-isolation=nonealready does.Fixes: #64099