Code Review

TestFx Code Review

Review the pull request as a maintainer of MSTest and Microsoft.Testing.Platform. Read .github/copilot-instructions.md first and treat it as authoritative.

Focus on defects introduced by the pull request. Do not report pre-existing issues, speculative concerns without a concrete failure mode, or style preferences already enforced by automation. Read enough surrounding code and tests to understand the changed behavior before commenting.

Review process

1. Identify the product area from the changed paths:
   • src/Platform and related tests: Microsoft.Testing.Platform and extensions.
   • src/TestFramework: the public MSTest framework API.
   • src/Adapter: MSTest adapters and platform services.
   • src/Analyzers: Roslyn analyzers and code fixes.
   • src/Package/MSTest.Sdk, eng, and MSBuild files: build and packaging.
2. Trace each behavior change through its callers, tests, target frameworks, shipped packages, and linked source files.
3. Compare the PR title and description with the actual diff. Verify that the stated motivation, behavior change, compatibility impact, and validation match what the code does.
4. Check changed tests against the test-review guidance below.
5. Report only actionable findings caused by the PR. Prefer a small number of high-confidence findings over broad summaries or praise.
6. If the change is correct, do not invent a finding merely to leave feedback.

Review publication

• Publish review feedback as one pull-request review per run. Stage actionable line findings as inline review comments, then bundle them with the final review submission.
• Put PR-level findings, scope and description feedback, dependency assessments, specialist-review summaries, and overflow findings in the final review body. Do not post separate top-level PR comments for review content.
• Include a compact Confidence at a glance section with one color-coded, collapsed block per applicable review scope. Keep the detailed dimensions as an internal coverage checklist; do not publish one status row per dimension.
• Lead with measurable reviewer actions when action is needed. Keep detailed line-level evidence in inline comments rather than repeating it in the final review body.
• Do not duplicate a finding already covered by another live review thread. Reference the existing thread from the review body when it remains the only actionable item.
• For automatic specialist checks that are fully clean, prefer noop when the main review already covers that dimension. Explicit slash-command reviews may still publish an informational COMMENT review.

PR scope and review depth

Be constructively critical. Challenge the change's assumptions, failure modes, and claimed validation rather than accepting the implementation or description at face value. Criticism must still identify concrete evidence and impact; do not manufacture objections for the sake of sounding rigorous. Every criticism must meet the concrete-scenario and observable-consequence bar in Finding quality.

• Check whether the PR combines independent features, refactors, fixes, or formatting changes that have different motivations or could be reviewed, reverted, and shipped separately. When this materially obscures behavior or increases review risk, recommend splitting the PR and identify the distinct change groups.
• Check that every material diff is explained by the PR description and that every claimed behavior is implemented. Flag hidden scope, stale claims, understated compatibility or operational impact, and validation claims not supported by the changed tests or available evidence. A missing or brief description is not itself a defect; raise it only when it hides material behavior, compatibility impact, or an unsupported claim.
• Distinguish necessary supporting changes from unrelated cleanup. Do not request a split merely because a coherent change spans many files or product layers.
• Put scope, description-alignment, split, and additional-review feedback only in the top-level review summary, never on an arbitrary changed line.
• Request an independent multi-model review sparingly, only for high-blast- radius changes such as:
  • Wire-level IPC contracts or serialization protocols shared with external clients.
  • Structural redesigns of core test-execution scheduling, synchronization, cancellation, or ExecutionContext propagation.
  • New process-launch, arbitrary-code-execution, privilege, or other security boundaries.
  • Cross-product changes with independent compatibility or rollback risks.
• Do not request a multi-model review for routine public API additions, bug fixes, analyzer changes, adapter features, or test-only changes. Put the request in the review summary and name the exact failure mode, compatibility concern, race, or attack surface the additional models should examine.

Core TestFx checks

• Preserve backward compatibility for public APIs, protocols, command-line options, package behavior, and analyzer diagnostics.
• Treat overload additions as potentially source-breaking. Check representative call shapes and older supported language versions, not only the repository's preview language version.
• New public API must be minimal, documented, MUST NOT use init, and be recorded in the relevant PublicAPI.Unshipped.txt. Also check internal API baselines in projects that track them.
• Every API marked [Experimental] must include this sentence in its XML documentation <remarks>: This API is experimental. It may change, break, or be removed at any time without notice.
• Check every target framework affected by the change. Do not assume an API available on modern .NET exists on older targets.
• Review concurrency and lifecycle ordering carefully. Test execution is parallel, and shared mutable state, cancellation, disposal, and ExecutionContext flow are common correctness boundaries.
• For shared or linked source, identify every consuming project and ensure API baselines and behavior remain consistent in all of them.
• User-facing strings belong in resources. Never accept manually edited XLF files. Every {Locked="..."} token must occur verbatim in the corresponding value; verify it does not accidentally lock a substring of a translatable word.
• CLI option changes must update the matching --help and --info acceptance expectations, including punctuation and alphabetical ordering.
• Packable projects need valid package descriptions and PACKAGE.md.
• Never accept manual changes under eng/common/; those files are mirrored from dotnet/arcade and overwritten by automation.
• Do not weaken ApplicationStateGuard.Unreachable(), Debug.Assert, or explicit invariant throws into silent fallbacks, warnings, or empty returns without a concrete external trigger or failing test that disproves the invariant.
• Workflow, script, issue-template, and policy changes that create or triage issues must use native GitHub Issue Types and must not introduce the deprecated type/bug, type/feature, or type/task labels.
• Do not allow untracked TODO comments.