[Diagnostic] Strengthen #52029 repro and unskip IncludeTestAssembly=true cases#54578
[Diagnostic] Strengthen #52029 repro and unskip IncludeTestAssembly=true cases#54578Evangelink wants to merge 2 commits into
Conversation
PR #54514 parameterized the code-coverage test on IncludeTestAssembly and skipped the =true cases pointing at issue #52029 (BadImageFormatException on Linux/macOS). However, the minimal test asset never actually triggered the static managed instrumenter (StaticManagedInstrumenter.InstrumentAsync short-circuits when both <IncludeDirectories> and <AdditionalFiles> are empty), so the skip was not meaningful: CI would have been green even with the bug present. This change is a diagnostic to prove-or-disprove the bug: - Adds a Lib class library with async methods (Task.Yield/Delay loops, IAsyncEnumerable). - Adds async test methods exercising those methods so the test assembly has real async state machines. - Writes a richer coverage.config that explicitly enables static managed instrumentation and populates <ModulePaths><IncludeDirectories> so the instrumenter actually rewrites the test assembly and Lib. - Unskips both IncludeTestAssembly=true cases. If CI fails on Linux/macOS, the bug is confirmed and we will re-skip with the stronger repro committed; if green across the board, the unskip stands. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CI results: the bug does NOT reproduce even with the strengthened reproI dug into the macOS x64 + Windows x64 Helix runs of build 1448073. The previously-skipped Windows x64 (Helix job macOS x64 (Helix job What the inner test produced (snippet from The strengthened test asset (Lib with About the other red checks (all unrelated)
RecommendationBug doesn't reproduce. I'm marking this PR Ready for Review — the unskip stands and #52029 can stay closed. Happy to drop the diagnostic test-asset enrichment (extra Lib + async tests + stronger coverage.config) before merge if you'd rather keep the asset minimal; let me know. cc @Youssef1313 |
There was a problem hiding this comment.
Pull request overview
Strengthens the existing dotnet test --coverage regression/diagnostic for #52029 by ensuring static managed instrumentation actually performs IL rewriting (via non-empty <ModulePaths><IncludeDirectories>) and by making the test asset contain real async state machines in both a referenced library and the test assembly itself. This makes the unskipped IncludeTestAssembly=true rows meaningful on Linux/macOS instead of being effectively no-ops.
Changes:
- Expanded the
TestProjectSolutionWithCodeCoveragetest asset with a newLibproject containing async patterns (Task.Yield,Task.Delay,IAsyncEnumerable) and added async MSTest methods that exercise it. - Updated the coverage settings written by the CLI test to enable static managed instrumentation and to include the test project
bin/<Configuration>directory recursively, preventing the instrumenter’s early short-circuit. - Unskipped the
IncludeTestAssembly=truecases and updated the expected test run summary counts (now 6 total tests, 1 failing).
Show a summary per file
| File | Description |
|---|---|
| test/TestAssets/TestProjects/TestProjectSolutionWithCodeCoverage/TestProjectSolutionWithCodeCoverage.sln | Adds the new Lib project to the solution and configuration mappings. |
| test/TestAssets/TestProjects/TestProjectSolutionWithCodeCoverage/TestProject/TestProject.csproj | References Lib so coverage instrumentation and test execution traverse both assemblies. |
| test/TestAssets/TestProjects/TestProjectSolutionWithCodeCoverage/TestProject/Test1.cs | Adds async tests and an await foreach path to produce realistic async state machines in the test assembly. |
| test/TestAssets/TestProjects/TestProjectSolutionWithCodeCoverage/Lib/Lib.csproj | Introduces a class library project in the test asset (TFM via $(CurrentTargetFramework)). |
| test/TestAssets/TestProjects/TestProjectSolutionWithCodeCoverage/Lib/AsyncCalculator.cs | Implements async APIs designed to force IL rewriting by static instrumentation. |
| test/dotnet.Tests/CommandTests/Test/GivenDotnetTestBuildsAndRunsTestsWithArtifacts.cs | Unskips IncludeTestAssembly=true, writes richer coverage.config, and updates assertions for the new test count. |
Copilot's findings
- Files reviewed: 6/6 changed files
- Comments generated: 0
Follow-up to #54514 / #52029 — diagnostic to prove-or-disprove the bug.
Why
Reviewer @Youssef1313 pointed out on #54514 (comment) that it's unclear whether issue #52029 (
BadImageFormatException: Index not foundon Linux/macOS whenIncludeTestAssembly=true) actually still reproduces. PR #54514 just parameterized the existing test onIncludeTestAssemblyand skipped the=truecases with a link to the issue, but the original test asset is so minimal that the bug would never trigger anyway — so a green CI on those skips would have proved nothing.Concretely,
StaticManagedInstrumenter.InstrumentAsyncshort-circuits when both<IncludeDirectories>and<AdditionalFiles>are empty in the coverage config. The merged coverage.config only sets<IncludeTestAssembly>, so the instrumenter never actually rewrote any IL and the historical crash path was never exercised.What this PR does (diagnostic)
Libclass library with async methods (Task.Yield/Task.Delayloops,IAsyncEnumerable) so there are real async state machines for the static instrumenter to rewrite.Libso the test assembly itself has async patterns (matching the original crash stack which was during async cleanup).coverage.configthat explicitly enables static managed instrumentation and populates<ModulePaths><IncludeDirectories>pointing at the test projectbinfolder, defeating the early-return.IncludeTestAssembly=truecases.What to do with the CI results
BadImageFormatException/ exit code 134 → bug is confirmed. I'll re-skip the=truerows but with the stronger repro committed, so the skip is actually meaningful and a future fix can be validated against this test.Marking as Draft until CI tells us which branch we're on.
cc @Youssef1313 @Evangelink
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com