Uh oh!
There was an error while loading. Please reload this page.
Add report generator lifecycle tests - #10860
Conversation
Cover ReportGeneratorBase session enablement, ordering, message consumption, artifact publication, and output behavior through CtrfReportGenerator. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c7572463-f383-4eeb-82b4-15b66d4c02f8
This comment has been minimized.
This comment has been minimized.
🧪 Expert test review — PR #10860Reviewed the 5 new test methods in
This advisory comment was generated automatically. Grades are heuristic and informational — they do not block merging. Suggestions on the Files changed tab can be applied with one click. Re-run with
|
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.
Review Summary
New unit tests for CtrfReportGenerator lifecycle — clean, well-structured test file that covers enable/disable, the unreachable-exception guard, full lifecycle with artifact publishing and JSON verification, and the no-warning path.
| # | Dimension | Verdict |
|---|---|---|
| 1 | Algorithmic Correctness | ✅ Clean |
| 2 | Error Handling | ✅ Clean — UnreachableException assertion uses full-name comparison with a clear comment explaining the cross-assembly polyfill issue |
| 3 | Concurrency & Thread Safety | N/A |
| 4 | Performance & Allocations | ✅ Clean |
| 5 | API Design & Public Surface | N/A — test-only code, no public API |
| 6 | Binary & Wire Compatibility | N/A |
| 7 | Cross-TFM Correctness | ✅ Clean — #if NETCOREAPP guard on DisposeAsync |
| 8 | Naming & Conventions | ✅ Clean — follows Method_Condition_ExpectedAsync naming |
| 9 | Assertions & Test Quality | ✅ Clean — uses MSTest assertions per BannedSymbols.txt; good use of Assert.HasCount, Assert.EndsWith, JSON structural assertions |
| 10 | Resource Management | MemoryFileStream.Dispose() is a no-op; see inline comment |
| 11 | Localization | N/A |
| 12 | Security | ✅ Clean |
| 13 | Documentation | ✅ Clean — the polyfill comment on the UnreachableException test is helpful |
| 14 | Null Safety | ✅ Clean — null! for unused IDataProducer parameters is acceptable |
| 15 | Code Duplication | MemoryFileStream and TestSessionContextStub are duplicated across multiple test files; see inline comment |
| 16 | Backward Compatibility | N/A |
| 17 | Configuration | ✅ Clean |
| 18 | init Accessor Ban | N/A — no public API |
| 19 | PublicAPI.Unshipped.txt | N/A |
| 20 | StyleCop / EditorConfig | ✅ Clean |
| 21 | CLI Option Changes | N/A |
| 22 | Scope Discipline | ✅ Clean — single-concern PR adding tests for one component |
Overall: Good test coverage addition. One minor inline comment about MemoryFileStream disposal consistency and potential deduplication — no blockers.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: None
What changed in this PR
Adds direct unit coverage for shared report-generator lifecycle orchestration through CtrfReportGenerator.
Changes:
- Tests option-based enablement and invalid lifecycle ordering.
- Verifies result consumption, artifact publication, session UID, and warning-free output behavior.
| File | Description |
|---|---|
test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportGeneratorLifecycleTests.cs | Adds five focused lifecycle tests and supporting test doubles. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Delegate synchronous and asynchronous disposal to the inner memory stream for consistency with the existing report-engine test helper. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c7572463-f383-4eeb-82b4-15b66d4c02f8
🧵 Parallel-safety audit — PR #10860Parallelization — audited assembly:
Nothing audited here touches process-global state, shared filesystem paths, or The new file
No findings to report. Advisory only — heuristic, non-blocking. Re-run with
|
🧪 Expert test review — PR #10860Reviewed the 5 new test methods in
No new high-confidence issues found. One inline finding (the duplicated This advisory comment was generated automatically. Grades are heuristic and informational — they do not block merging. Re-run with
|
Uh oh!
There was an error while loading. Please reload this page.
ReportGeneratorBaseowns the session lifecycle shared by every report-generator extension, but that orchestration had no direct unit coverage. This adds focused coverage throughCtrfReportGenerator, the smallest concrete implementation, without changing production code.The tests verify:
--report-ctrfoptionThe test harness uses the real internal service provider with mocked platform dependencies. The unreachable-state assertion compares exception full names because the exception polyfill has separate type identities across target assemblies.
Tests:
Microsoft.Testing.Extensions.UnitTests(1,495 passed, 7 skipped) and the focused lifecycle suite (5 passed).Fixes#10850