Repository navigation
Fix MTP test builds after updating MSTest dependencies - #12309
Conversation
…261109.7 On relative base path root Microsoft.Testing.Platform From Version 3.0.0-preview.26556.8 -> To Version 3.0.0-preview.26559.7 MSTest From Version 5.0.0-preview.26556.8 -> To Version 5.0.0-preview.26559.7
Remove redundant object casts and stale compatibility comments after the Assert.IsNotNull API revert flows into the test dependencies. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🧪 Test quality review — PR #12309
Compared merge base ebe065a133d6beac1b57e72f6419f21595c8577b with head ec207e9caffda8b68fac1a2c3f2d5c7a3f9063ee. Evidence applies to this head only.
Test-value evidence
| Contract | Evidence | Status |
|---|---|---|
MSTest and MTP toolset bump (26556.8 → 26559.7) |
Dependency version change only. | Not applicable |
Four Assert.IsNotNull((object?)x) calls become Assert.IsNotNull(x) (behavior-preserving) |
Same assertion and the same null check before and after. The change relies on the bumped MSTest providing the typed overload and [NotNull] flow. No execution records were available. |
Static prediction: passes before and after (unverified) |
No behavior changed, so no regression test was needed. This workflow does not run tests or builds.
🧵 Parallel safety
No parallelization configuration or lifecycle changes. The edits are single-line assertion changes. I did not run the specialist for this assertion-only diff. Status: no parallel-safety hazards identified (static read of the diff only).
Per-test assessment
Pseudo-mutation evidence is not applicable: these are behavior-preserving assertion simplifications.
| Result | Quality | Test | Notes | How to improve |
|---|---|---|---|---|
| Pass | — | mod MtpServerClientInProcessTests.ShutdownServer_CancellationRegistration_UsesOnlyTheRemainingVi... |
Only the null-check cast was removed. The contract assertion is unchanged. | None |
| Pass | — | mod MtpServerClientInProcessTests.LaunchInProcessAsync_CallbackIsCanceledBeforeConnecting_Preser... |
Only the null-check cast was removed. | None |
| Pass | — | mod TestHostControllersTestHostFinalizationTests.ControllerIpc_UnsupportedRequestReachesFailFastAndRethrowsOrig... |
Only the null-check cast was removed. | None |
| Pass | — | mod TestHostExecutionOrchestratorMiddlewarePipelineTests.RunAsync_NestedMiddlewareFault_DelayedAncestorCancellationStil... |
Only the null-check cast was removed. | None |
The method names are truncated in the table. Letter grades were not computed for these one-line cast removals.
<sub>This advisory review is heuristic and non-blocking.
Re-run the combined review with /review-tests, or only the parallel-safety specialist with /parallel-audit.</sub>
🤖 Automated content by GitHub Copilot. Generated by the Test Quality Review on PR (on open / sync) workflow. · copilot · auto · ~25 AIC · ⌖ ~1 AIC · ⊞ 8.1K
There was a problem hiding this comment.
🟢 Approval recommended
No outstanding review issues remain with the dependency updates and redundant-cast cleanup.
No open issues 🎉
What changed in this PR
This PR repairs MTP unit-test builds after the MSTest dependency update in #12300. It removes casts made redundant by the restored void Assert.IsNotNull API while preserving null checks and lifecycle logic.
Changes:
- Update MSTest and MTP dependency versions and producer commit metadata.
- Remove four obsolete object casts and their compatibility comments across three test files.
Test-value evidence: For head ec207e9, the PR reports successful Debug/Release builds and 18 passing focused test cases. These results were not independently executed during this review; no runtime behavior change is introduced by the assertion edits.
| File | Description |
|---|---|
test/UnitTests/Microsoft.Testing.Platform.UnitTests/TestHostExecutionOrchestratorMiddlewarePipelineTests.cs |
Removes the redundant cast from the downstream-task null check. |
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Hosts/TestHostControllersTestHostFinalizationTests.cs |
Removes the redundant cast from the peer-task null check. |
test/UnitTests/Microsoft.Testing.Platform.ServerMode.Client.Sources.UnitTests/MtpServerClientInProcessTests.cs |
Removes redundant casts from two cancellation-task null checks. |
eng/Versions.props |
Updates the consumed MSTest and MTP package versions. |
eng/Version.Details.xml |
Aligns dependency tracking versions and producer commit hashes. |
Review effort: Balanced
Pull request
Summary
The MSTest dependency update in #12300 exposes four
IDE0004: Cast is redundanterrors in MTP unit tests. Update the flowed MSTest/MTP dependencies and remove the obsolete object casts and compatibility comments from the affectedAssert.IsNotNullcalls. Null assertions, lifecycle logic, and parallel scheduling are retained; no analyzer is suppressed.Relates to #12300. This PR publishes the same validated head from the session branch as a separate delivery option. Both PRs currently contain the dependency bump and repair, rather than additive changes.
Design and decision record
The producer reverted the return-value
Assert.IsNotNullAPI in #12273, so the object casts used to accommodate the earlier package API are now unnecessary. Removing the casts is preferable to rolling back the dependency update or disablingIDE0004. This is a test-source build correction, not a feature or architecture change, so no separate decision record is needed.The flowed versions are MSTest
5.0.0-preview.26559.7and Microsoft.Testing.Platform3.0.0-preview.26559.7.Demonstration
The original Azure build 1672135 and GitHub attachment run 42739984728 stopped on the four redundant casts before tests ran.
With this head, attachment run 42802362174 passes, including packing the shipping products, exercising the packaged runner/report extension, uploading and downloading the real attachment archive, and verifying its bytes and test associations. Screenshots are not applicable because this change has no UI or layout surface.
Validation and regression proof
Tested head:
ec207e9caffda8b68fac1a2c3f2d5c7a3f9063ee; repair parent:6ead5f026f1ed8d402792974370a8fe8dec1b986. Toolchain: repository-pinned SDK12.0.100-alpha.1.26556.115bootstrapped through the repository build scripts. Effective restore assets confirm the updated MSTest packages.Both affected projects build in Debug and Release for
net10.0,net11.0, andnet462: all four build commands exit 0 with zero warnings and errors. The six focused Release test-host runs execute 18 cases: 18 passed, 0 failed, 0 skipped, retaining the existing method-level parallelization (32 observed workers).Executed local build commands and test selection
Each build command below succeeded for all three configured target frameworks:
The exact test invocation used the following filters and matrix;
$resultswas the session-owned report directory. Each host invocation exited 0:The client count includes all three data rows of
ShutdownServer_CancellationRegistration_UsesOnlyTheRemainingVirtualGrace:(3,2),(5,0), and(6,-1). Local binlogs and the six TRX files are retained in the session; they are not attached publicly. The inherited local PATH was shortened only for child build/test processes to avoid cmd.exe's path-length limit; no machine-wide setting or repository configuration was changed.Existing CI evidence for this exact head through #12300: Azure build 1672661 has passed every compilation step, Linux Release, Windows Debug, Windows application-model acceptance, and the BuildXL leg. Windows Release runtime tests were still in progress when last checked, so full Azure success is not claimed. These are existing same-head runs, not a claim that this new PR's checks have already completed.
Test-value evidence
The reported defect is an analyzer/build failure, not a failing runtime assertion. All four
IDE0004locations are verified in the original CI logs; the platform project's two locations were also reproduced locally against the updated package. After the repair, all configured build variants and all 18 selected runtime cases pass, with the original assertions retained.No new runtime behavior or test case is introduced, and no runtime red/green sensitivity or paired green-before/green-after proof is claimed: the pre-fix source does not compile with the updated package. The initial local baseline also used a global TargetFramework override that produced unrelated
NU1510diagnostics in dependencies; final validation uses the projects' configured target frameworks without that override and has no errors.