Add Assert.ContainsAll and Assert.DoesNotContainAll with structured assertion messages#8234
Add Assert.ContainsAll and Assert.DoesNotContainAll with structured assertion messages#8234Evangelink wants to merge 9 commits into
Conversation
…ion messages Mirrors the existing CollectionAssert.IsSubsetOf / CollectionAssert.IsNotSubsetOf APIs but uses the RFC 012 structured assertion message format. Multiplicity-aware diffing surfaces every excess element (with null support). When a non-default IEqualityComparer is supplied, its short type name is added to the evidence block and a <comparer> placeholder is appended to the call-site, per RFC 012.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds new Assert.IsSubsetOf / Assert.IsNotSubsetOf overloads (generic + non-generic, with optional comparers) using RFC 012 structured assertion messages, plus tests and localized resource strings.
Changes:
- Introduces 8 new
Assert.*SubsetOf*overloads with structured evidence/call-site rendering. - Adds a dedicated unit test suite validating pass/fail behavior, null handling, comparer rendering, and
AssertFailedExceptionExpected/Actual text population. - Adds new message resources (
IsSubsetOfFailedSummary,IsNotSubsetOfFailedSummary) and updates localization files + public API declarations.
Show a summary per file
| File | Description |
|---|---|
| test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.IsSubsetOf.cs | Adds coverage for new subset/not-subset asserts including structured message verification. |
| src/TestFramework/TestFramework/Assertions/Assert.IsSubsetOf.cs | Implements the new Assert APIs, diffing logic, evidence creation, and comparer adaptation for non-generic enumerables. |
| src/TestFramework/TestFramework/Resources/FrameworkMessages.resx | Adds 2 new message summary resources used by structured assertion messages. |
| src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.*.xlf | Propagates the 2 new resource entries into localized XLF files. |
| src/TestFramework/TestFramework/PublicAPI/PublicAPI.Unshipped.txt | Declares the 8 new public overloads for API tracking. |
Copilot's findings
- Files reviewed: 17/17 changed files
- Comments generated: 4
Evangelink
left a comment
There was a problem hiding this comment.
PR #8234 — Assert.IsSubsetOf / Assert.IsNotSubsetOf
Good implementation. Multiplicity-aware diffing, single-enumeration snapshots, null-safe counting, proper structured messages, correct PublicAPI.Unshipped.txt entries, auto-generated XLF. The test coverage is solid (35 tests covering generic/non-generic, comparer, null inputs, edge cases, and ExpectedText/ActualText population).
Verdict Table
| # | Dimension | Status | Notes |
|---|---|---|---|
| 1 | Algorithmic Correctness | ✅ LGTM | Empty subset, null handling, multiplicity all correct |
| 2 | Threading & Concurrency | ✅ N/A | No shared mutable state |
| 3 | Security | ✅ N/A | No IPC/deserialization |
| 4 | Public API & Binary Compat | ✅ LGTM | 8 entries in PublicAPI.Unshipped.txt, no init accessors, sealed class |
| 5 | Performance & Allocations | ✅ LGTM | Single enumeration, List<T?> snapshot reuse |
| 6 | Cross-TFM Compatibility | ✅ LGTM | No TFM-gated API used |
| 7 | Resource & IDisposable | ✅ N/A | No disposables |
| 8 | Defensive Coding | ✅ LGTM | All params null-checked before use |
| 9 | Localization & Resources | ✅ LGTM | .resx updated, XLF auto-generated |
| 10 | Test Isolation | ✅ LGTM | No shared static mutable state |
| 11 | Assertion Quality | ✅ LGTM | Uses TestFramework.ForTestingMSTest + AwesomeAssertions |
| 12 | Flakiness | ✅ N/A | No sleeps or order-dependent collection asserts |
| 13 | Test Completeness | ✅ LGTM | All 8 overloads exercised; null/edge cases covered |
| 14 | Data-Driven Coverage | ✅ N/A | Direct tests sufficient |
| 15 | Code Structure | ✅ LGTM | Pattern matching, is null/is not null throughout |
| 16 | Naming & Conventions | ✅ LGTM | File-scoped namespace, consistent with codebase |
| 17 | Documentation | ✅ LGTM | Full XML docs on all 8 public overloads |
| 18 | Analyzer Quality | ✅ N/A | No analyzer changes |
| 19 | IPC Wire Compat | ✅ N/A | No serialization |
| 20 | Build Infrastructure | ✅ N/A | No eng/ changes |
| 21 | Scope & PR Discipline | ✅ LGTM | Single focused concern |
Key Findings
[MODERATE] BuildCallSiteWithComparer (line 454) performs string surgery by stripping the trailing ) from FormatCallSiteExpression's output and appending , <comparer>). This silently produces a malformed call-site string if the format ever changes. The best fix is a new FormatCallSiteExpression overload that accepts a third argument expression; a defensive Debug.Assert(callSite.EndsWith(')')) is a reasonable short-term guard. See inline comment.
[NIT] public new bool Equals in NonGenericEqualityComparerAdapter is correct and intentional (implements IEqualityComparer<object?>), but the new keyword warrants a brief comment so future readers don't question it. See inline comment.
[NIT — test gap] The non-generic + comparer happy-path cases (IsSubsetOf_NonGeneric_WithComparer_AllPresent_ShouldPass and IsNotSubsetOf_NonGeneric_WithComparer_MissingElement_ShouldPass) are absent. Not blocking, but the symmetry is worth closing.
Generated by Expert Code Review (on open) for issue #8234 · ● 10.7M
- Drop comparer.GetType().Name from failure evidence (Dim 16): the CLR type name is fragile across TFMs (e.g. StringComparer.OrdinalIgnoreCase reports OrdinalComparer on .NET Framework, OrdinalIgnoreCaseComparer on .NET 5+). The <comparer> placeholder in the call-site already conveys that a custom comparer was used. Fixes net48 test failures. - Document that null elements are skipped in AllItemsAreInstancesOfType (Dim 17). - Add TODO for NonGenericEqualityComparerAdapter dedup with PR #8234 (Dim 21). - Note why callSite[..^1] cannot be used (Dim 16 NIT was invalid: System.Range/System.Index not available on net462/netstandard2.0). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add 3-expression overload of FormatCallSiteExpression and use it in BuildCallSiteWithComparer (no more brittle string surgery on the trailing parenthesis). - Rewrite TryFindMissingElements to walk the subset in source order, so 'missing' is in true first-seen order with multiplicity (no longer relies on Dictionary enumeration order). - Add an inline comment on NonGenericEqualityComparerAdapter.Equals explaining why 'new' is required. - Use a custom CaseInsensitiveStringComparer in tests so the comparer name is stable across .NET Framework and .NET (StringComparer.OrdinalIgnoreCase reports OrdinalComparer on netfx). - Add IsSubsetOf_NonGeneric_WithComparer_AllPresent_ShouldPass and IsNotSubsetOf_NonGeneric_WithComparer_MissingElement_ShouldPass happy-path tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Clarify multiplicity semantics in XML docs of all 8 IsSubsetOf/IsNotSubsetOf overloads. - Add HasAnyMissingElement fast path used by IsNotSubsetOfImpl that short-circuits on the first uncovered element and avoids allocating the missing-elements list. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add IsNotSubsetOf multiplicity-contract tests (DuplicatesExceedSupersetMultiplicity_ShouldPass, DuplicatesWithinSupersetMultiplicity_ShouldFail) — directly guard the HasAnyMissingElement quota arithmetic. - Add multi-element 'missing:' rendering tests (MultipleMissing_PreservesFirstSeenOrderAndMultiplicity, ExcessInMiddleOfMatchingRun_ReportsOnlyExcess) to lock in the documented first-seen positional order and multiplicity preservation. - Tighten the HasAnyMissingElement summary comment to accurately describe what is actually short-circuited (the subset walk, not the O(|superset|) count-dictionary build). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Renames the recently-added Assert.IsSubsetOf and Assert.IsNotSubsetOf APIs to Assert.ContainsAll and Assert.DoesNotContainAll. Parameter rename: subset -> expected, superset -> collection. This matches the existing Assert.Contains(expected, collection) convention. Failure-message evidence labels updated accordingly: subset: -> expected:, superset: -> collection:. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Resolve conflicts in FrameworkMessages.resx, PublicAPI.Unshipped.txt, and XLF files. Regenerated XLF via UpdateXlf to include ContainsAll/DoesNotContainAll entries and pick up AreEquivalent entries added on main. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
|
||
| private static string? BuildCallSiteWithComparer(string assertionMethodName, string expectedExpression, string collectionExpression, bool hasComparer) | ||
| => hasComparer | ||
| ? FormatCallSiteExpression(assertionMethodName, expectedExpression, collectionExpression, expression3: string.Empty, "<expected>", "<collection>", "<comparer>") |
There was a problem hiding this comment.
Good catch. Fixed in d007f56 by passing "<comparer>" (instead of string.Empty) as expression3 in BuildCallSiteWithComparer. Because the comparer parameter doesn't carry a [CallerArgumentExpression], the third slot was always empty, which combined with the all-empty short-circuit in FormatCallSiteExpression could suppress the call site entirely when callers (e.g. wrappers or non-C# languages) don't propagate caller expressions. With this change the call site now always renders, and degrades to Assert.ContainsAll(<expected>, <collection>, <comparer>) (and likewise for DoesNotContainAll) when no expressions are available. Added two unit tests in AssertTests.ContainsAll.cs covering the missing-caller-info case for both APIs.
Pass <comparer> as expression3 in BuildCallSiteWithComparer so the structured assertion call site is always rendered when the comparer overload is used, even if callers do not propagate [CallerArgumentExpression]. Previously, when all three expression slots were empty, the call site was suppressed entirely, losing the useful signal that the comparer overload was invoked. Added two unit tests covering the no-CallerArgumentExpression case for both ContainsAll and DoesNotContainAll. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Adds
Assert.ContainsAllandAssert.DoesNotContainAll(generic and non-generic, with and without anIEqualityComparer) on theAssertclass, providing the same containment semantics asCollectionAssert.IsSubsetOf/CollectionAssert.IsNotSubsetOfbut with the RFC 012 structured assertion message format and naming consistent with the existingAssert.Contains(expected, collection)API.This is the first in a small series of PRs lifting
CollectionAssert.*APIs ontoAssert.*with the new message format.API surface
8 new public overloads:
All overloads support
[CallerArgumentExpression]capture forexpectedandcollection.The parameter naming mirrors the existing
Assert.Contains(expected, collection)API:expectedis the (collection of) item(s) we expect to find,collectionis the container being searched.Behavior
expected(with multiplicity) appears in themissing:evidence line — a true diff rather than a set-membership check.expectedare matched against nulls incollection(by count) without going through the user-suppliedIEqualityComparer, matching the existingCollectionAssertsemantics.expectedandcollectionare snapshotted toList<T?>once before counting, so lazy / single-pass enumerables behave deterministically and the failure path doesn't re-enumerate.<comparer>placeholder in the rendered call-site. Both are emitted.Example failure messages
With a non-default comparer:
Tests
35+ unit tests in
AssertTests.ContainsAll.cscovering:expected, multiplicity withincollection).collection.expected, nulls only incollection.IEqualityComparer<T>(StringComparer.OrdinalIgnoreCase) + comparer-line +<comparer>placeholder in call-site.IEnumerablevariants (with and without comparer).[NotNull]parameter contracts (expected,collection,comparer) for all 8 overloads.DoesNotContainAll(both empty / emptyexpectedboth correctly fail).AssertFailedException.ExpectedText/ActualTextproperties are populated correctly.All tests pass; full repo
Build.cmdsucceeds with 0 warnings, 0 errors.Notes
FrameworkMessages.resx;.xlffiles were regenerated by the build (not hand-edited).PublicAPI.Unshipped.txt.