Uh oh!
There was an error while loading. Please reload this page.
Let explicit --minimum-expected-tests govern the zero-tests verdict (#7457) - #9709
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR (testfx side of #7457) makes --minimum-expected-tests take precedence over the "zero tests ran" (exit code 8) verdict in TestApplicationResult.GetProcessExitCode(). Previously the ZeroTests verdict was computed independently and always won for empty runs, and --minimum-expected-tests 0 was rejected by validation. Now an explicitly-provided --minimum-expected-tests governs the count-based verdict: 0 accepts an empty run (like --ignore-exit-code 8), while N with zero tests reports a minimum-expected violation (exit code 9) instead of 8. This is the enabling precondition for a dotnet test --test-modules orchestrator (SDK-side follow-up) to control zero-tests behavior for a whole run rather than per module.
Changes:
- Allow
--minimum-expected-tests 0(validation now rejects only negative / non-integer values) and update the validation message wording. - When
--minimum-expected-testsis set, use it to decide the count-based verdict and skip the ZeroTests/--zero-tests-policybranch; otherwise the existing zero-tests behavior is unchanged. - Regenerate localization (resx + 13 xlf) and add unit + acceptance coverage.
Show a summary per file
| File | Description |
|---|---|
src/Platform/Microsoft.Testing.Platform/Services/TestApplicationResult.cs | Core change: explicit --minimum-expected-tests governs the verdict via an if/else, superseding the ZeroTests branch. |
src/Platform/Microsoft.Testing.Platform/CommandLine/PlatformCommandLineProvider.cs | Validation relaxed from value <= 0 to value < 0 so 0 is accepted. |
src/Platform/Microsoft.Testing.Platform/Resources/PlatformResources.resx | Updated error message to "non-negative integer". |
src/Platform/.../Resources/xlf/PlatformResources.*.xlf (13 files) | Regenerated via UpdateXlf; source updated, targets marked needs-review-translation. |
test/UnitTests/.../Services/TestApplicationResultTests.cs | Added tests for min 0 + zero tests → Success and min N + zero tests → violation. |
test/UnitTests/.../CommandLine/PlatformCommandLineProviderTests.cs | 0 now valid; negatives/non-integers invalid; renamed/added test cases. |
test/IntegrationTests/.../ExecutionTests.cs | Acceptance coverage for zero-test scenarios via --filter-uid 2; updated validation-message expectation. |
docs/Changelog-Platform.md | Changelog entry for the behavior change. |
I verified the branching logic, argument-parsing safety (arity + Length==1 validation guards argumentList[0]), the test asset (uids 0/1, so --filter-uid 2 yields zero tests), and that no stale references to the old validation message remain. I also confirmed the terminal-reporter verdict divergence for --minimum-expected-tests 0 matches the intended --ignore-exit-code 8 equivalence. No blocking issues were found, but the change alters a shipped CLI option's exit-code semantics (a public behavioral contract) and coordinates with a cross-repo dotnet/sdk change, which warrants human sign-off.
Review details
- Files reviewed: 20/20 changed files
- Comments generated: 0
- Review effort level: Medium
…7457) In TestApplicationResult.GetProcessExitCode(), an explicitly-provided --minimum-expected-tests now governs the count-based verdict and supersedes the ZeroTests (exit code 8) verdict: a run of fewer than N tests yields MinimumExpectedTestsPolicyViolation (exit code 9) even when zero tests ran. This lets a 'dotnet test --test-modules' orchestrator tell a stricter per-module minimum (forwarded via '-- --minimum-expected-tests N') apart from a module that simply matched no tests, which is the precondition for deciding the 'zero tests ran' verdict at the whole-run level instead of per module. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
c595e67 to
cdefc18CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Defensively guard the --minimum-expected-tests value extraction in TestApplicationResult.GetProcessExitCode() with a single-argument pattern match plus int.TryParse, instead of indexing argumentList[0] and int.Parse (which could throw if the option ever reached here present-but-empty or non-integer). Malformed values now fall back to the zero-tests verdict; they are still rejected earlier by option validation in practice. - Fix changelog grammar: 'no test ran' -> 'no tests ran'. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Note
🤖 Automated review by GitHub Copilot. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.
| # | Dimension | Verdict |
|---|---|---|
| 1 | Algorithmic Correctness | 🟡 1 MODERATE |
| 21 | Scope & PR Discipline | ⚪ 1 NIT |
✅ 20/22 dimensions clean.
- Algorithmic Correctness — Terminal reporter verdict ("Zero tests ran" /
IsRunFailed=true) diverges from exit code (Success) when--minimum-expected-tests 0is used with an empty run.GetMinimumExpectedTests()returns0for both "not set" and "explicitly set to 0", so the terminal reporter can't distinguish the two and always shows the failure-like verdict. Low-priority follow-up. - Scope — Changelog entry for PR #9724 is included in this PR (#9709), coupling merge ordering.
Overall: The core logic change is correct and well-tested. The if/else refactoring cleanly separates the --minimum-expected-tests path from the --zero-tests-policy path, and the boundary condition change (<= 0 → < 0) correctly enables the zero-minimum scenario. Tests cover the key edge cases (zero with no tests → success, N>0 with no tests → violation). The --zero-tests-policy is silently superseded when --minimum-expected-tests is explicitly set, which is the documented intent.
No blocking issues.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
This comment has been minimized.
This comment has been minimized.
Reverts the #9724 change that allowed the literal value '--minimum-expected-tests 0'. Keeping '0' invalid restores an unambiguous meaning for GetMinimumExpectedTests() == 0 ('option not set'), which avoids the terminal-reporter verdict divergence tracked in #9744: with '--min 0' the exit code was 0 (success) while the terminal still rendered a failed 'Zero tests ran' verdict, because the shared verdict helpers could not distinguish 'unset' from 'explicit 0' without a cross-repo change to the vendored TerminalTestReporterOptions. Issue #7457 item #2 ('--min 0 == --ignore-exit-code 8') remains served by the existing '--ignore-exit-code 8'. The load-bearing behavior for the 'dotnet test --test-modules' orchestrator -- an explicit '--minimum-expected-tests N' governs the zero-tests verdict (exit code 9, not 8) -- is unaffected and stays. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
b288da5 to
f5c9072Compare| * Re-print errored assemblies in dotnet test end-of-run recap by @Evangelink in [#9545](https://github.com/microsoft/testfx/pull/9545) | ||
| * Add server-initiated session cancellation to the dotnet test IPC protocol by @Evangelink in [#9549](https://github.com/microsoft/testfx/pull/9549) | ||
| * Emit `::warning` annotations for skipped tests in `Microsoft.Testing.Extensions.GitHubActionsReport` by @Evangelink in [#9641](https://github.com/microsoft/testfx/pull/9641) | ||
| * Let an explicit `--minimum-expected-tests N` govern the zero-tests verdict, so a run of fewer than N tests reports the minimum-expected violation (exit code 9) instead of "zero tests ran" (exit code 8) even when no tests ran. This lets a `dotnet test --test-modules` orchestrator tell a stricter local-minimum violation apart from an empty module (#7457) by @Copilot in [#9709](https://github.com/microsoft/testfx/pull/9709) |
🧪 Test quality grade — PR #9709
This advisory comment was generated automatically. Grades are heuristic
|
Uh oh!
There was an error while loading. Please reload this page.
Summary
testfx side of #7457. Makes an explicit
--minimum-expected-tests Ngovern the "zero tests ran" (exit code 8) verdict so that adotnet test --test-modules/ global-filter orchestrator can control the zero-tests behavior for the whole run rather than failing once per module.Problem
In
TestApplicationResult.GetProcessExitCode()the ZeroTests (8) verdict was computed before and independently of--minimum-expected-tests, so--minimum-expected-tests Nwith zero tests returned 8, not 9. An orchestrator that normalizes a per-module8(empty module) could not distinguish "empty because the global filter matched nothing here" from "empty but the user asked for a stricter local minimum".Change
An explicit
--minimum-expected-tests Nnow governs the count-based verdict and supersedes ZeroTests(8): a run of fewer than N tests yieldsMinimumExpectedTestsPolicyViolation(exit code 9), even when zero tests ran. When the option is not set, the existing ZeroTests /--zero-tests-policybehavior is unchanged. (The value parse is guarded with a single-argument pattern match +int.TryParse.)This is exactly what the
dotnet testorchestrator (dotnet/sdk#55167) relies on to normalize a per-module8to success without swallowing a stricter per-module minimum forwarded via-- --minimum-expected-tests N, which now surfaces as exit code 9. Bothdotnet test --minimum-expected-tests 10 -- --minimum-expected-tests 2verdicts remain independently observable.Why
--minimum-expected-tests 0was droppedAllowing an explicit
0created a UX divergence (originally raised in review): the exit code was0(success) but the terminal reporter still rendered a failed "Zero tests ran" verdict, becauseGetMinimumExpectedTests()returns0for both "unset" and "explicitly 0" and the shared verdict helpers couldn't tell them apart. Fixing that cleanly needs a change toTerminalTestReporterOptions.MinimumExpectedTests, which is vendored to dotnet/sdk (a cross-repo contract change).Since allowing
0is not required by the orchestrator design (that relies only on the "explicit N → exit 9" behavior above) and #7457 item #2 ("--min 0==--ignore-exit-code 8") is already served by the existing--ignore-exit-code 8, we reverted the0support. Keeping0invalid preserves the unambiguous meaning ofGetMinimumExpectedTests() == 0("unset") and avoids the divergence entirely. Tracking issue #9744 is closed as obsolete.Tests
TestApplicationResultTests:--minimum-expected-tests N+ zero tests → exit code 9.ExecutionTests(acceptance):--filter-uidmatching nothing +--minimum-expected-tests 3→ exit code 9.Unit tests pass locally (
TestApplicationResultTests+PlatformCommandLineProviderTests: 103/103). Acceptance tests require-packand run in CI.Related
Co-authored-by: Copilot App 223556219+Copilot@users.noreply.github.com