Skip to content

fix: make the documented demo test command actually run tests - #456

Merged
twcclegg merged 3 commits into
mainfrom
fix/demo-test-command-runs-no-tests
Sep 6, 2026
Merged

twcclegg merged 3 commits into
mainfrom
fix/demo-test-command-runs-no-tests

Conversation

@twcclegg

@twcclegg twcclegg commented Sep 5, 2026

Copy link
Copy Markdown
Owner

Problem

csharp/PhoneNumbers.Demo/AGENTS.md makes running the demo suite a mandatory step, with this command:

dotnet test csharp/PhoneNumbers.Demo.Tests -p:TargetFrameworks=net10.0

That command runs zero tests and exits 0:

A total of 1 test files matched the specified pattern.
No test is available in .../PhoneNumbers.Demo.Tests.dll. Make sure that test discoverer & executors
are registered and platform & framework version settings are appropriate and try again.

exit code: 0

PhoneNumbers.Demo.Tests declares the singular <TargetFramework>net10.0</TargetFramework>, so -p:TargetFrameworks=net10.0 sets a property the project never reads and promotes the build to a cross-targeting outer build, where no tests are discovered. Because dotnet test exits 0 on zero discovery, the result reads as a pass.

The net effect is that anyone — human or agent — following the mandatory workflow gets a green light for a suite that never ran. That is the precise failure the workflow exists to prevent.

Fix

The -p:TargetFrameworks=net10.0 override is vestigial here — it's copy-pasted from the repo-root convention (where PhoneNumbers.Test/PhoneNumbers.Extensions.Test genuinely multi-target net8.0;net10.0 and the switch does real work), but PhoneNumbers.Demo.Tests only ever targets net10.0 and gets nothing from declaring it plural. So the fix removes the override from the one documented command that doesn't need it, rather than reshaping the csproj into permanent cross-targeting form to tolerate a flag that shouldn't be there — that alternative would pay outer/inner cross-targeting build overhead forever for a project that will never multi-target, and reads as "designed to support multiple frameworks" to the next contributor.

AGENTS.md additionally notes that a zero-discovery run still exits 0 regardless, so the count is worth checking either way.

Verification

Command Before After
dotnet test csharp/PhoneNumbers.Demo.Tests -p:TargetFrameworks=net10.0 0 tests, exit 0 n/a — no longer the documented command
dotnet test csharp/PhoneNumbers.Demo.Tests 67 passed 67 passed
CI sequence (restore / build --no-restore / test --no-build) 67 passed 67 passed

CI (build_and_run_demo_tests.yml) never passed -p:TargetFrameworks in the first place, so it was never exposed to this bug and isn't affected either way.

Scope note: the repo-wide dotnet test csharp/PhoneNumbers.slnx -p:TargetFrameworks=net10.0 in the root AGENTS.md, README.md and CONTRIBUTING.md is not affected — the demo projects aren't in PhoneNumbers.slnx, and both solution test projects genuinely multi-target, so the switch does real work there.

The demo's mandatory testing workflow documents

    dotnet test csharp/PhoneNumbers.Demo.Tests -p:TargetFrameworks=net10.0

but PhoneNumbers.Demo.Tests declared the singular TargetFramework, so that
switch set an otherwise-unused property and promoted the project to a
cross-targeting outer build. The run discovered zero tests, printed "No test
is available in ...", and exited 0 -- a green result for a suite that never
ran, which is exactly the failure mode the workflow is meant to prevent.

Declare TargetFrameworks instead, matching PhoneNumbers.Test and
PhoneNumbers.Extensions.Test, so the switch overrides a real property. Also
warn in AGENTS.md that a zero-discovery run still exits 0.

The solution-wide command is unaffected: the demo projects aren't in
PhoneNumbers.slnx, and both solution test projects already multi-target.
@codecov

codecov Bot commented Sep 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.68%. Comparing base (8ad7fa2) to head (c403a0d).
⚠️ Report is 14 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #456      +/-   ##
==========================================
+ Coverage   87.51%   87.68%   +0.16%     
==========================================
  Files          43       43              
  Lines        3886     3889       +3     
  Branches      991      992       +1     
==========================================
+ Hits         3401     3410       +9     
+ Misses        280      276       -4     
+ Partials      205      203       -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…ping the csproj

The prior fix made PhoneNumbers.Demo.Tests declare TargetFrameworks (plural)
purely so a copy-pasted -p:TargetFrameworks=net10.0 override - which does
nothing useful here since this project only ever has one TFM - wouldn't turn
the build into a no-op cross-targeting outer build. That permanently pays
cross-targeting overhead for a project that will never multi-target, and
reads as "designed to support multiple frameworks" to the next contributor.

Drop the override from the one documented command that doesn't need it
instead, and revert the csproj to the singular TargetFramework it always
should have had. Same fix, smaller and pointed at the actual root cause.
@twcclegg
twcclegg force-pushed the fix/demo-test-command-runs-no-tests branch from 159ddf1 to 60444d2 Compare September 5, 2026 19:38
The command this recommends is right, but the reason given for it was not.
Passing -p:TargetFrameworks=net10.0 does not produce a cross-targeting outer
build: the build log shows it compiling the inner
bin/Debug/net10.0/PhoneNumbers.Demo.Tests.dll and VSTest running against it.

What it actually does is stop xunit.runner.visualstudio's build assets from
being applied, so the adapter never lands in the output directory -
xunit.runner.reporters.netcoreapp10.dll and xunit.runner.utility.netcoreapp10.dll
are present without the switch and absent with it. That is what VSTest's "Make
sure that test discoverer & executors are registered" is telling us, and it is
where the next person should look.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K1NSXom6AbfgVknJDtfR9C
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants