Skip to content

Add Assert.AreAllNotNull / AreAllDistinct / AreAllOfType#8237

Open
Evangelink wants to merge 8 commits into
mainfrom
dev/amauryleve/assert-allitems
Open

Add Assert.AreAllNotNull / AreAllDistinct / AreAllOfType#8237
Evangelink wants to merge 8 commits into
mainfrom
dev/amauryleve/assert-allitems

Conversation

@Evangelink
Copy link
Copy Markdown
Member

@Evangelink Evangelink commented May 14, 2026

Summary

Mirror the existing CollectionAssert.AllItems* APIs onto Assert, producing structured assertion messages per RFC 012.

This is the second PR in a series adding collection-style assertions to Assert. PR1 (#8234) introduced Assert.IsSubsetOf / IsNotSubsetOf.

APIs added

  • Assert.AreAllNotNull(IEnumerable, ...) and Assert.AreAllNotNull<T>(IEnumerable<T>, ...)
  • Assert.AreAllDistinct(IEnumerable, [IEqualityComparer], ...) and Assert.AreAllDistinct<T>(IEnumerable<T>, [IEqualityComparer<T>], ...)
  • Assert.AreAllOfType(Type, IEnumerable, ...) and Assert.AreAllOfType<TExpected>(IEnumerable, ...)

All overloads accept an optional string? message and use [CallerArgumentExpression] to surface the original source-text in the rendered call site.

Output format

Assertion failed. Expected all items in collection to be distinct.

duplicates: [5]
collection: [5, 5, 5, 5]

Assert.AreAllDistinct(new[] { 5, 5, 5, 5 })
  • null is reported a single time even when multiple null entries are duplicated.
  • Duplicate values are likewise reported once.
  • For AreAllOfType, the generic call site renders as Assert.AreAllOfType<TExpected>(...) (we cannot know the user-typed T from typeof(T).Name, so we use a stable literal placeholder).

Tests

Unit tests covering: pass paths, empty collections, null collection (parameter-validation), null comparer, user messages, lazy IEnumerable<T>, T = int?, derived types, comparer (custom + null-handling), 3+ nulls reported once, repeated-duplicates reported once, both generic and non-generic dispatch.

Notes for reviewer

Relates to #7956

Mirror the CollectionAssert.AllItems* APIs onto Assert per RFC 012.

Adds:

- Assert.AllItemsAreNotNull (generic + non-generic IEnumerable)

- Assert.AllItemsAreUnique (generic + non-generic, both with optional comparer)

- Assert.AllItemsAreInstancesOfType (generic <TExpected> + non-generic Type)

All produce structured assertion messages including evidence (null indices / duplicates / mismatches), source collection, optional comparer, and expression-based call site.

Notes:

- NonGenericEqualityComparerAdapter is duplicated with PR #8234 (Assert.IsSubsetOf); will be deduped post-merge.
Copilot AI review requested due to automatic review settings May 14, 2026 20:10
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@Evangelink
Copy link
Copy Markdown
Member Author

Review — Dimensions 16–21


Dimension 16 · Naming & Conventions — NIT

callSite.Substring(...) vs range operator

BuildCallSiteWithComparerForCollection uses callSite.Substring(0, callSite.Length - 1) instead of the modern C# range syntax. Prefer callSite[..^1] for consistency with the rest of the codebase.

// current
return string.Concat(callSite.Substring(0, callSite.Length - 1), ", <comparer>)");

// preferred
return string.Concat(callSite[..^1], ", <comparer>)");

comparer.GetType().Name as display name

In AllItemsAreUnique<T>(..., IEqualityComparer<T>? comparer, ...) and the non-generic overload, comparerName is populated with comparer.GetType().Name. For closed-generic comparers (e.g. EqualityComparer<String>) this produces a raw CLR name like EqualityComparer\1, not something meaningful to the test author. Consider comparer.GetType().ToString()` (includes type arguments) or omitting the name entirely when it would render unreadably. This also affects the evidence block shown on failure.

Everything else — method names (*Impl, ReportAssert*Failed), parameter names (collectionExpression, expectedTypeExpression, genericTypeArgumentName), file-scoped namespaces, #region/#endregion labels — all consistent with existing Assert files. ✓


Dimension 17 · Documentation Accuracy — MODERATE

Null elements are silently skipped in AllItemsAreInstancesOfType, but the XML doc says they cause a throw.

The implementation:

if (element is not null && !expectedType.IsInstanceOfType(element))

So a null element is not treated as a failure. The public doc says:

<exception cref="AssertFailedException"> Thrown if ... some elements of collection do not inherit from / implement expectedType.

A null element does not inherit from expectedType, yet it is deliberately excluded. This is a semantic choice (mirroring NUnit/MSTestV1 behavior) but the contract as documented does not mention it. The test file has a comment // A null element is NOT an instance of any type, so it does not fail this assertion., confirming the intentional behaviour — but that intent is missing from the public XML doc.

Suggested addition to both the non-generic and generic overloads' <exception> block:

/// Null elements are skipped and do not cause the assertion to fail.

(or a dedicated <remarks> element).


Dimension 18 · Analyzer & Code Fix Quality

N/A — no changes under src/Analyzers/.


Dimension 19 · IPC Wire Compatibility

N/A — no changes to serialization/IPC types in src/Platform/.


Dimension 20 · Build Infrastructure & Dependencies — LGTM

  • No eng/Versions.props or Directory.Packages.props changes. ✓
  • No new <PackageReference> entries. ✓
  • Three new .resx entries (AllItemsAreNotNullFailedSummary, AllItemsAreUniqueFailedSummary, AllItemsAreInstancesOfTypeFailedSummary) — straightforward, correctly placed at end of file. ✓
  • All 14 .xlf files regenerated by build (not hand-edited). ✓
  • PublicAPI.Unshipped.txt updated with all 8 new signatures; nullability annotations look correct. ✓

Dimension 21 · Scope & PR Discipline — NIT

Intentional NonGenericEqualityComparerAdapter duplication — no in-code tracking

The PR description says:

NonGenericEqualityComparerAdapter will be deduplicated once both PRs (#8234 and this one) land.

However there is no // TODO comment in the source to make this discoverable when PR #8234 is merged or if the dedup work is ever deprioritised. Recommend adding one:

// TODO: deduplicate with the same adapter in Assert.CollectionEquivalence.cs once PR #8234 is merged.
private sealed class NonGenericEqualityComparerAdapter : IEqualityComparer<object?>

Scope — the PR is tightly focused on the three AllItems* APIs plus their resources and tests. No unrelated changes observed. ✓


Summary

Dim Verdict Finding
16 NIT callSite[..^1] over Substring; GetType().Name gives raw CLR name for generic comparers
17 MODERATE Null-element skip behaviour in AllItemsAreInstancesOfType is undocumented in public XML docs
18 N/A
19 N/A
20 LGTM
21 NIT No in-code TODO tracking the planned NonGenericEqualityComparerAdapter deduplication

Generated by Expert Code Review (on open) for issue #8237 · ● 24.2M ·

Copy link
Copy Markdown
Member Author

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

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

Review: Dimensions 6–10

Dim 6 — Cross-TFM Compatibility ✅ LGTM

All APIs used ([CallerArgumentExpression], Cast<>(), HashSet<T>(IEqualityComparer<T>), collection expressions [.. collection], StringBuilder) are already used elsewhere in the codebase across net462/netstandard2.0/net8.0/net9.0. No new TFM gaps introduced.

#pragma warning disable CS8714 is correct: HashSet<T> requires notnull, but T here can be nullable (e.g. string?). This suppression is the right approach rather than constraining the overloads.


Dim 7 — Resource & IDisposable Management ✅ LGTM

List<T>, HashSet<T>, and StringBuilder are not IDisposable. No file handles, streams, or other disposable resources created.


Dim 8 — Defensive Coding at Boundaries ✅ LGTM

  • Custom comparer throws: exceptions from user-supplied IEqualityComparer/IEqualityComparer<T> propagating out of the assertion is consistent with how HashSet<T> behaves everywhere in the framework. Acceptable.
  • collection is List<T> list ? list : [.. collection]: Aliases the original list rather than copying — indices remain stable within the single-threaded execution of the assertion. Non-thread-safe behavior is expected and documented by convention.
  • collection.Cast<object?>(): safe — CheckParameterNotNull already guards against null.

Dim 9 — Localization & Resources ✅ LGTM

New .resx summary entries (AllItemsAreNotNullFailedSummary, AllItemsAreUniqueFailedSummary, AllItemsAreInstancesOfTypeFailedSummary) are correctly added and used via FrameworkMessages.*. .xlf files are left for the build to regenerate.

Evidence labels ("null indices:", "expected type:", "mismatches:", "collection:", "duplicates:", "comparer:") and the "(or derived)" fragment are hardcoded string literals — but this is consistent with the established pattern: Assert.IsInstanceOfType.cs, Assert.IsNull.cs, Assert.ThrowsException.cs, etc. all hardcode the same class of label strings. The .resx boundary (summary/user-facing message vs. structural label) is applied uniformly.

The "index " prefix inside the mismatch StringBuilder is similarly a structural formatting string, consistent with the pattern throughout.


Dim 10 — Test Isolation ✅ LGTM

No static mutable state. Each test creates its input data and assertion action locally. No shared setup that could leak between test cases.

Generated by Expert Code Review (on open) for issue #8237 · ● 24.2M

Amaury Levé and others added 2 commits May 15, 2026 09:47
- 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>
- Assert.AllItemsAreNotNull -> Assert.AreAllNotNull
- Assert.AllItemsAreUnique  -> Assert.AreAllDistinct (LINQ-aligned vocabulary)
- Assert.AllItemsAreInstancesOfType -> Assert.AreAllOfType
  - Param order flipped to (expectedType, collection) to match
    the modern (expected, actual, options) Assert convention.
  - Null elements now FAIL (previously skipped) to match the
    semantics of the singular Assert.IsInstanceOfType ([NotNull])
    and the LINQ Enumerable.OfType<T> contract that null is T == false.
  - Mismatch evidence renders null elements as 'index N: <null>'.

Also:
- Renamed resx entries (AllItems* -> AreAll*) and regenerated xlf.
- Updated PublicAPI.Unshipped.txt (8 entries, alphabetical).
- Updated unit tests (renamed file + methods, adjusted expected
  failure messages, added _NullElement_ShouldFail tests for
  AreAllOfType).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 15, 2026 15:23
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot's findings

Comments suppressed due to low confidence (1)

src/TestFramework/TestFramework/Assertions/Assert.AreAllDistinct.cs:165

  • duplicatesSeen ??= new HashSet<T>(comparer); can be simplified to target-typed new(comparer) (matching seen) so the IDE0028 suppression isn’t needed here either.
#pragma warning disable IDE0028
                duplicatesSeen ??= new HashSet<T>(comparer);
#pragma warning restore IDE0028
  • Files reviewed: 20/20 changed files
  • Comments generated: 3

Comment thread src/TestFramework/TestFramework/Assertions/Assert.AreAllDistinct.cs Outdated
Comment thread src/TestFramework/TestFramework/Assertions/Assert.AreAllDistinct.cs Outdated
Comment thread src/TestFramework/TestFramework/PublicAPI/PublicAPI.Unshipped.txt
@Evangelink Evangelink force-pushed the dev/amauryleve/assert-allitems branch from 3d5f95d to 53d0687 Compare May 15, 2026 15:35
…ng and expand test coverage

- Replace parallel List<int>/List<Type?> with a single List<TypeMismatch>

  using a private readonly struct (avoids ValueTuple which is unavailable

  on net462).

- Add Debug.Assert pinning the FormatCallSiteExpression contract that the

  rendered call-site ends with ')'.

- Expand AreAllOfType tests to cover boxed value types, Nullable<T> null

  elements, struct mismatches, non-generic value-type elements, and

  interface expected types.

- Add AreAllDistinct tests pinning the null short-circuit semantics for

  user-provided comparers (null elements are not passed to the comparer).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 15, 2026 15:51
@Evangelink Evangelink changed the title Add Assert.AllItemsAreNotNull / AreUnique / AreInstancesOfType Add Assert.AreAllNotNull / AreAllDistinct / AreAllOfType May 15, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot's findings

  • Files reviewed: 19/19 changed files
  • Comments generated: 3

Comment thread src/TestFramework/TestFramework/Assertions/Assert.AreAllOfType.cs Outdated
Comment thread src/TestFramework/TestFramework/PublicAPI/PublicAPI.Unshipped.txt
Comment thread src/TestFramework/TestFramework/Assertions/Assert.AreAllDistinct.cs Outdated
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Evangelink
Copy link
Copy Markdown
Member Author

@copilot resolve the merge conflicts in this pull request

Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI commented May 16, 2026

@copilot resolve the merge conflicts in this pull request

Resolved in c36a08523 by merging origin/main into this branch and resolving the two content conflicts in:

  • src/TestFramework/TestFramework/PublicAPI/PublicAPI.Unshipped.txt
  • src/TestFramework/TestFramework/Resources/FrameworkMessages.resx

Review — Dimensions 16–21

---...

Addressed in c36a08523.

  • Switched to callSite[..^1] in BuildCallSiteWithComparerForCollection.
  • Updated comparer evidence text to use comparer.GetType().ToString() in both generic and non-generic overloads.
  • AreAllOfType currently treats null elements as mismatches (not skipped), and the XML docs reflect that behavior.

Copilot AI review requested due to automatic review settings May 16, 2026 11:06
@Evangelink Evangelink review requested due to automatic review settings May 16, 2026 11:06
@Evangelink
Copy link
Copy Markdown
Member Author

@copilot fix these build issues

@azure-pipelines
azure-pipelines
/ microsoft.testfx (Build Linux Debug)
src/TestFramework/TestFramework/Assertions/Assert.AreAllDistinct.cs#L132

src/TestFramework/TestFramework/Assertions/Assert.AreAllDistinct.cs(132,9): error IDE0007: (NETCORE_ENGINEERING_TELEMETRY=Build) use 'var' instead of explicit type (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0007)
Check failure on line 132 in src/TestFramework/TestFramework/Assertions/Assert.AreAllDistinct.cs

@azure-pipelines
azure-pipelines
/ microsoft.testfx (Build Linux Debug)
src/TestFramework/TestFramework/Assertions/Assert.AreAllDistinct.cs#L132

src/TestFramework/TestFramework/Assertions/Assert.AreAllDistinct.cs(132,27): error IDE0028: (NETCORE_ENGINEERING_TELEMETRY=Build) Collection initialization can be simplified (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0028)
Check failure on line 161 in src/TestFramework/TestFramework/Assertions/Assert.AreAllDistinct.cs

@azure-pipelines
azure-pipelines
/ microsoft.testfx (Build Linux Debug)
src/TestFramework/TestFramework/Assertions/Assert.AreAllDistinct.cs#L161

src/TestFramework/TestFramework/Assertions/Assert.AreAllDistinct.cs(161,36): error IDE0028: (NETCORE_ENGINEERING_TELEMETRY=Build) Collection initialization can be simplified (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0028)
Check failure on line 132 in src/TestFramework/TestFramework/Assertions/Assert.AreAllDistinct.cs

@azure-pipelines
azure-pipelines
/ microsoft.testfx (Build Linux Debug)
src/TestFramework/TestFramework/Assertions/Assert.AreAllDistinct.cs#L132

src/TestFramework/TestFramework/Assertions/Assert.AreAllDistinct.cs(132,9): error IDE0007: (NETCORE_ENGINEERING_TELEMETRY=Build) use 'var' instead of explicit type (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0007)
Check failure on line 132 in src/TestFramework/TestFramework/Assertions/Assert.AreAllDistinct.cs

@azure-pipelines
azure-pipelines
/ microsoft.testfx (Build Linux Debug)
src/TestFramework/TestFramework/Assertions/Assert.AreAllDistinct.cs#L132

src/TestFramework/TestFramework/Assertions/Assert.AreAllDistinct.cs(132,27): error IDE0028: (NETCORE_ENGINEERING_TELEMETRY=Build) Collection initialization can be simplified (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0028)

Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 16, 2026 11:58
@Evangelink Evangelink review requested due to automatic review settings May 16, 2026 11:58
Copy link
Copy Markdown
Contributor

Copilot AI commented May 16, 2026

@copilot fix these build issues

@azure-pipelines
...

Fixed in 297979e: Assert.AreAllDistinct now satisfies IDE0007/IDE0028 while preserving the comparer-aware HashSet<T> construction. I also replaced the range-based call-site trimming with a net462/netstandard2.0-compatible equivalent. Local dotnet build src/TestFramework/TestFramework/TestFramework.csproj -c Debug passes with 0 warnings/errors.

@Evangelink
Copy link
Copy Markdown
Member Author

@copilot resolve the merge conflicts in this pull request

…-allitems

# Conflicts:
#	src/TestFramework/TestFramework/Resources/FrameworkMessages.resx

Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 16, 2026 14:32
@Evangelink Evangelink review requested due to automatic review settings May 16, 2026 14:32
Copy link
Copy Markdown
Contributor

Copilot AI commented May 16, 2026

@copilot resolve the merge conflicts in this pull request

Resolved in 8ff7b77: merged current origin/main into the branch and resolved the FrameworkMessages.resx conflict by keeping both the AreAll assertion summaries and the regex assertion summaries. Local dotnet build src/TestFramework/TestFramework/TestFramework.csproj -c Debug passes with 0 warnings/errors.

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