Skip to content

Fix nullable collection assertion constraints - #11374

Merged
Amaury Levé (Evangelink) merged 3 commits into
microsoft:mainfrom
Sergio0694:user/sergiopedri/nullable-collection-assertions
Sep 21, 2026
Merged

Amaury Levé (Evangelink) merged 3 commits into
microsoft:mainfrom
Sergio0694:user/sergiopedri/nullable-collection-assertions

Conversation

@Sergio0694

@Sergio0694 Sergio Pedri (Sergio0694) commented Sep 18, 2026

Copy link
Copy Markdown
Member

Summary

Fix CS8631 when a non-null value is asserted against a collection of nullable elements, such as:

List<string?> values = ["Name", null];
Assert.Contains("Name", values);
  • Allow nullable elements in the concrete-collection Assert.Contains and Assert.DoesNotContain value overloads, including comparer overloads.
  • Preserve the C# 12 overload compatibility introduced by Fix array assertion overload ambiguity with older C# versions #11038, existing comparer and predicate contracts, collection-specific comparers, and failure messages/caller expressions.
  • Add ten runtime regression tests and extend the warnings-as-errors C# 12 consumer fixture for nullable/non-nullable reference collections, nullable value collections, arrays, lists, sets, collection interfaces, ArraySegment, ImmutableArray, and dual-convertible collections.

Verification

  • Confirmed the handoff repro fails with CS8631 on MSTest 4.4.1 and compiles/passes on 4.4.0.
  • Confirmed the new runtime test source reports eight CS8631 errors against the unfixed source; the expanded consumer fixture also fails against the baseline assembly.
  • Warning-free, warnings-as-errors builds for the changed framework assets (net8.0/net9.0) and unit tests (net8.0/net48).
  • 199 containment-family tests pass on .NET 8 and 159 on .NET Framework 4.8. All ten new regression tests ran and passed on both, verified from TRX reports. Filter was /*/*/AssertTests/*Contain*.
  • Compiled the complete AssertSourceCompatibilityTests.ConsumerSource fixture directly against the locally built framework assemblies with SDK 8.0.424 / C# 12 / net8.0 and SDK 10.0.401 / C# 12 and C# 14 / net10.0. This was direct consumer compilation, not a claim that the entire acceptance test harness was run.
  • The original handoff test source compiles and executes successfully against the fixed framework on .NET 10.

CI follow-up

The Azure Pipelines retry showed that the Windows agent image no longer pre-creates the WER LocalDumps registry key. The pipeline now creates that key before configuring DumpFolder and DumpCount, allowing the Windows build legs to reach the actual build and test steps.

Validation build 1602211 passed all Windows, Linux, and macOS legs.

Allow nullable elements in the concrete Contains and DoesNotContain overloads while preserving comparer contracts and C# 12 overload compatibility. Add nullable consumer compilation coverage and runtime regressions.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings September 18, 2026 06:05
@Sergio0694
Sergio Pedri (Sergio0694) marked this pull request as ready for review September 18, 2026 06:05
@github-actions github-actions Bot added the state/needs-review Awaiting review from the team. label Sep 18, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The constraint changes are focused, compatibility-aware, and adequately covered by runtime and consumer-compilation tests.

Pull request overview

Fixes nullable collection constraints for MSTest containment assertions while preserving overload compatibility and behavior.

Changes:

  • Allows nullable collection elements in concrete collection overloads.
  • Adds runtime and C# 12 source-compatibility regressions.
  • Verifies comparer, message, expression, and collection-comparer behavior.
File summaries
File Description
Assert.Contains.cs Updates nullable constraints and forwarding.
Assert.DoesNotContain.cs Applies equivalent nullable support.
AssertTests.Contains.cs Adds runtime regression tests.
AssertSourceCompatibilityTests.cs Expands C# 12 consumer call coverage.
Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 0
  • Review effort level: Balanced

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@github-actions

Copy link
Copy Markdown
Contributor

Final test-triage resolution — Azure Pipelines build 1602049

This is a final (completed-build) analysis; no earlier preliminary comment was found to supersede.

Result: 1 failing test out of 43 flagged candidates; all others passed or were skipped/slow-only.

  • Test: Microsoft.Testing.Extensions.UnitTests.HotReloadTests.ShouldRunAsync_WaiterIsBlockedAndCancellationIsRequested_StopsNextRun
  • Leg: Linux Release, net9.0 (Microsoft.Testing.Extensions.UnitTests_net9.0.ctrf.json)
  • Failure: Assert.IsFalse(await shouldRunTask.WaitAsync(TimeSpan.FromSeconds(30), TestContext.CancellationToken)) — the 30s wait completed instead of remaining blocked, indicating the cancellation-triggered stop didn't happen in time (or at all) in this run.
  • History: Checked 12 prior completed builds (10 with a recorded result for this test) — all 10 passed, no prior failures or retries. No fail-then-pass pattern, so this is not a proven flaky test, just a single occurrence.
  • Relation to PR: This PR (Fix nullable collection assertion constraints) only touches nullable collection assertion code and has no relationship to Microsoft.Testing.Extensions HotReload logic — the failure is unrelated to the change under review.
  • Timeline signals: Two unrelated infra warnings (missing artifacts/log source folder, "Enable local dumps" PowerShell failures) — build/agent noise, not test-product failures.

Conclusion: Below the durable-issue threshold (single occurrence, clean history, no reproducible fail-then-pass or deterministic signature). No issue created. Recommend re-running the affected leg if it recurs; if it does, this looks like an async timing/cancellation race worth investigating in HotReloadTests.

🤖 Automated content by GitHub Copilot. Generated by the Pipeline Test Triage workflow. · copilot · auto · 40.4 AIC · ⌖ 7.31 AIC · ⊞ 8.9K

@Evangelink

Copy link
Copy Markdown
Member

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@Evangelink Amaury Levé (Evangelink) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review result

No actionable findings. The nullable constraint change preserves the C# 12 overload-resolution fix, reference/value nullable call shapes, comparer behavior, caller expressions, and failure messages.

The original Azure Pipelines build 1602049 was red only because two Windows jobs failed while enabling local dumps and Linux hit the unrelated HotReloadTests.ShouldRunAsync_WaiterIsBlockedAndCancellationIsRequested_StopsNextRun timing failure. A fresh validation build (1602198) has been queued.

Coverage inventory
  • Public API/source compatibility: generic constraints, explicit and implicit inference, arrays, interfaces, concrete collections, span-convertible collections, nullable reference/value elements, C# 12 consumers.
  • Runtime behavior: default/custom comparers, collection-owned comparers, null elements, predicates, diagnostics, caller argument expressions, telemetry forwarding.
  • Tests: assertion strength, negative paths, target-framework coverage, isolation/parallel safety, source-compatibility fixture effectiveness.
  • Repository checks: XML documentation, API baselines, linked source, localization, security boundaries, dependencies, formatting/diff hygiene.
  • Validation: focused framework unit tests passed on net48, net8.0, net8.0-windows, and net9.0; full build/pack completed with 0 warnings and 0 errors; both package-consuming C# 12 compatibility cases passed for net8.0 and net10.0.

@microsoft-github-policy-service microsoft-github-policy-service Bot added state/needs-review Awaiting review from the team. and removed state/needs-review Awaiting review from the team. labels Sep 18, 2026
@github-actions github-actions Bot added state/needs-review Awaiting review from the team. and removed state/needs-review Awaiting review from the team. labels Sep 18, 2026
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 18, 2026 10:22
@github-actions github-actions Bot added state/needs-review Awaiting review from the team. and removed state/needs-review Awaiting review from the team. labels Sep 18, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The implementation is focused, compatibility-sensitive paths are covered, and no unresolved defects were identified.

Review details
  • Files reviewed: 5/5 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@Evangelink Amaury Levé (Evangelink) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

🤖 Automated review by GitHub Copilot. To request a follow-up action, reply by tagging @copilot directly.

Review result

No actionable findings on current head aac076cd4.

The nullable TCollection constraint change fixes the C# 12 warning for non-null values used with nullable reference collections without changing the CLR generic constraint. The forwarding paths preserve default, collection-owned, nullable, and explicitly supplied comparer behavior, caller expressions, and failure messages. The added source-compatibility matrix exercises implicit overload resolution across arrays, interfaces, concrete collections, span-convertible collections, nullable references, and nullable values.

The separate Azure Pipelines change is also sound and documented in the PR description: it creates the WER LocalDumps registry key before assigning DumpFolder and DumpCount. Validation build 1602211 is green.

Verdict: ready for maintainer approval.

Coverage inventory
  • Public API and source compatibility: generic constraints, nullable annotations, explicit/implicit inference, comparer variance, C# 12 package consumers, API baselines.
  • Runtime behavior: default/custom comparers, HashSet<T> collection comparer preservation, null elements, predicates, diagnostics, caller expressions, telemetry forwarding.
  • Tests: all 10 added regressions are present and passing on net8.0 and net48; both package-consuming source-compatibility cases pass.
  • Cross-targeting: modern concrete-collection overloads and unchanged netstandard/.NET Framework fallback paths reviewed.
  • CI change: registry provider behavior, idempotency, and successful Windows/Linux/macOS validation reviewed.
  • Other dimensions: no concurrency, security-boundary, localization, dependency, serialization, or resource-lifetime impact.

@microsoft-github-policy-service microsoft-github-policy-service Bot added state/needs-review Awaiting review from the team. and removed state/needs-review Awaiting review from the team. labels Sep 21, 2026
@github-actions github-actions Bot added state/needs-review Awaiting review from the team. and removed state/needs-review Awaiting review from the team. labels Sep 21, 2026
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 21, 2026 11:02
@github-actions github-actions Bot added state/needs-review Awaiting review from the team. and removed state/needs-review Awaiting review from the team. labels Sep 21, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The implementation and tests consistently address the nullable constraint regression without changing established behavior.

Review effort: Balanced
Findings: None

@microsoft-github-policy-service microsoft-github-policy-service Bot added state/needs-review Awaiting review from the team. and removed state/needs-review Awaiting review from the team. labels Sep 21, 2026
@github-actions github-actions Bot added state/approved Proposal approved; ready for implementation. and removed state/needs-review Awaiting review from the team. labels Sep 21, 2026
@Evangelink

Copy link
Copy Markdown
Member

/backport to rel/4.4

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to rel/4.4: https://github.com/microsoft/testfx/actions/runs/35595117794

@Evangelink
Amaury Levé (Evangelink) merged commit 3be3f26 into microsoft:main Sep 21, 2026
35 checks passed
@github-actions github-actions Bot removed the state/approved Proposal approved; ready for implementation. label Sep 21, 2026
Amaury Levé (Evangelink) added a commit that referenced this pull request Sep 21, 2026
…(backport to rel/4.4) (#11408)

Co-authored-by: Sergio Pedri <sergio0694@live.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Amaury Levé <amauryleve@microsoft.com>
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.

3 participants