fix(assertions): preserve specialised source in .Count(itemAssertion) (#5707)

[thomhurst]

Closes #5707

Summary

• Per-item Count overload now propagates the inner assertion's specialised source instead of falling back to a generic wrapper
• Failure messages keep the typed diagnostic (mirrors the spirit of [Bug]: Overload resolution on string members prefers equality of IEnumerable<char> over string #5702)

Test plan

• Snapshot updates accepted
• Unit test asserting message contains specialised source for Count(item-assertion)

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 19 complexity

Metric	Results
Complexity	19

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[claude]

Code Review

This PR fixes issue #5707 by refactoring CollectionCountSource to accept a raw Func<TItem, int, IAssertion?> factory instead of the typed Func<IAssertionSource<TItem>, Assertion<TItem>?>, then adds specialised extension method overloads for concrete item shapes. The core approach is the right one — the extension-method dispatch strategy is the correct C# idiom given the type-system constraints, and the test coverage for the happy path is solid.

Several issues are worth addressing:

---

Issue 1 — IReadOnlySet<TInner> items have no specialised overload (Correctness Gap)

AssertionExtensions.cs adds a Count overload for ISet<TInner> items, but IReadOnlySet<TInner> items (which Assert.That routes to ReadOnlySetAssertion<TInner>) have no matching overload. Calls with IReadOnlySet<TInner> items silently fall through to the generic instance method, leaving IsSubsetOf, IsSupersetOf, etc. unreachable — exactly the bug this PR claims to fix.

The existing test (Count_Set_Items_Reach_IsSubsetOf_On_Inner) only covers ISet<int>, not IReadOnlySet<int>. A companion overload backed by ReadOnlySetAssertion<TInner> is needed to close this gap.

---

Issue 2 — Redundant legacy constructor on CollectionCountEqualsAssertion (Design Debt)

CollectionCountSource.cs (lines ~172–191) now has two constructors: the new Func<TItem, int, IAssertion?> one and an old Func<IAssertionSource<TItem>, Assertion<TItem>?> shim that just calls WrapWithValueAssertion and delegates to the new one. If there are no external call sites for the legacy constructor, it should be removed — it adds cognitive overhead and the WrapWithValueAssertion static is only there to serve it. Keeping both also means the public snapshot files lock in an extra constructor that carries no semantic value.

---

Issue 3 — WrapWithValueAssertion is internal static on a public class (API Coupling)

WrapWithValueAssertion is declared internal static on CollectionCountSource<TCollection, TItem> (a public class) and then called by CollectionCountEqualsAssertion via a cross-class static reference. One public class reaching into the internals of a sibling public class makes ownership unclear. If Issues 2 and 3 are fixed together — drop the legacy constructor, inline WrapWithValueAssertion at its single remaining call site — this coupling disappears entirely.

---

Issue 4 — Bare catch swallows OperationCanceledException (Safety)

CollectionCountSource.cs (lines ~230–238):

try
{
    await resultingAssertion.AssertAsync();
    _actualCount++;
}
catch
{
    // Item did not satisfy the assertion, don't count it
}

This pre-existed the PR, but the PR now routes every specialised-source assertion through this path, making the impact broader. A cancellation during test teardown could be silently dropped here, causing a hang or wrong count. The fix is simple:

catch (Exception ex) when (ex is not OperationCanceledException)
{
    // Item did not satisfy the assertion
}

or narrowing to catch (AssertionException) if only assertion failures should be suppressed.

---

Issue 5 — Array<TItem> items have no specialised overload (Feature Parity Gap)

ArrayAssertion<TItem> inherits CollectionAssertionBase<TItem[], TItem> and provides index-based operations like HasItemAt. When a collection's items are arrays (e.g. List<int[]>), there is no specialised overload — the PR covers IList<TInner> and IReadOnlyList<TInner> but not TInner[]. Users wanting HasItemAt on inner arrays will hit the same wall the issue describes.

---

Issue 6 — The string overload widens the lambda return type silently (Subtle Contract Change)

The existing generic instance method expects Func<IAssertionSource<TItem>, Assertion<TItem>?>, but all the new extension overloads (including for string) accept Func<IAssertionSource<TItem>, IAssertion?>. A string lambda that returns Assertion<string>? still compiles and resolves to the extension method — but the compile-time contract is now different depending on which overload wins. This is harmless today, but documenting the widening explicitly (or standardising on IAssertion? in the instance method too, if that's the long-term direction) would prevent future confusion.

---

Issue 7 — CountSpecialised uses a fragile explicit interface cast (Minor Design)

var context = ((IAssertionSource<TCollection>)source).Context;

AssertionExtensions is in the same assembly as Assertion<TCollection>, so the protected Context property should be accessible directly as source.Context without the cast. The cast couples the helper to the detail of which interface exposes Context explicitly, and will break at runtime (not compile time) if that explicit implementation changes.

---

Issue 8 — Failure-message tests are missing for most specialised shapes (Test Coverage)

The PR's motivation includes preserving typed diagnostic messages in the failure path. Only Count_Specialised_Source_Failure_Message_Mentions_Inner_Expectation checks the message content, and only for the IEnumerable<TInner> shape. Without failure-path tests for IList, IDictionary, ISet, and IReadOnlyList shapes, regressions on message quality for those shapes would go undetected.

---

Overall Assessment

The core fix is correct and the extension-method dispatch strategy is the right design for this problem. The main items to address before merging:

1. Add the IReadOnlySet<TInner> specialised overload (Issue 1 — correctness gap).
2. Remove the legacy Func<IAssertionSource<TItem>, Assertion<TItem>?> constructor if it has no external callers (Issues 2 & 3 — simplification).
3. Narrow the bare catch to guard against OperationCanceledException (Issue 4 — safety).

Issues 5–8 are lower priority but worth a follow-up.

[claude]

Follow-up Review

The second commit addresses all three primary issues from the previous review cleanly:

• IReadOnlySet overload added - the IReadOnlySet<TInner> gap is closed, with a matching test.
• Legacy constructor on CollectionCountEqualsAssertion removed - the dead overload and its WrapWithValueAssertion helper are gone.
• Bare catch narrowed - OperationCanceledException is now preserved correctly.

A few smaller observations on what remains:

---

1 — Public constructor on CollectionCountSource still carries a legacy signature

The public constructor on CollectionCountSource<TCollection, TItem> still accepts Func<IAssertionSource<TItem>, Assertion<TItem>?>?. The internal design has moved to Func<TItem, int, IAssertion?>, and the public constructor just wraps it. Keeping the old signature in the public API surface (confirmed by the verified snapshot files) means external callers are permanently coupled to Assertion<TItem>? as the return type even though all new overloads use IAssertion?. If narrowing that constructor to internal is feasible (i.e., no external callers exist), it would keep the public surface consistent and prevent future divergence. The snapshot update would be small.

---

2 — Explicit interface cast in CountSpecialised is necessary but deceptive

var context = ((IAssertionSource<TCollection>)source).Context;

This is actually required because Context is protected on Assertion<TValue> and AssertionExtensions is a different class (even in the same assembly). The explicit interface implementation on CollectionAssertionBase re-exposes it publicly. However, Assertion<TValue> also declares internal AssertionContext<TValue> InternalContext => Context; — since AssertionExtensions is in the same assembly, source.InternalContext would be a cleaner and more self-documenting access pattern. The rest of the file takes IAssertionSource<T> parameters and calls .Context directly; CountSpecialised is the only method that takes a concrete base class and then has to cast it. This is a minor readability issue, not a correctness problem.

---

3 — Array-typed items still have no specialised overload (gap)

ArrayAssertion<TItem> inherits from CollectionAssertionBase<TItem[], TItem> and exposes the same collection surface as ListAssertion. When collection items are arrays (e.g. List<int[]>), there is no specialised Count overload — the generic instance method on CollectionAssertionBase wins, exposing only IAssertionSource<TItem[]> instead of ArrayAssertion<TItem>. This was raised in the previous review as Issue 5. It is a lower-priority follow-up, but it is worth a separate issue to track so it doesn't get forgotten — the same pattern that broke IReadOnlySet would apply here.

---

4 — Failure-message coverage for non-enumerable shapes remains missing

The only failure-path test (Count_Specialised_Source_Failure_Message_Mentions_Inner_Expectation) covers the IEnumerable<TInner> shape. For IList, IDictionary, ISet, and IReadOnlyList shapes there is no test asserting that the failure message contains the typed inner expectation. This means a regression (e.g., accidentally using ValueAssertion instead of the typed source) would only be caught for the enumerable shape. This was raised as Issue 8 in the previous review. It would be valuable to add at least one failure-message test per specialised shape, or to track this as a follow-up.

---

Overall

The core fix is correct, the addressed issues are cleanly resolved, and the PR is in good shape. Issues 3 and 4 above are lower-priority gaps that could land as follow-up issues rather than blocking this PR. Issue 1 (public constructor signature) is the only one worth considering before merge if API surface consistency matters.

[claude]

Code Review: fix(assertions): preserve specialised source in .Count(itemAssertion)

Summary

This PR fixes a real bug (#5707): when items inside a collection are themselves collections, dictionaries, or sets, the per-item .Count(itemAssertion) overload previously exposed only a generic IAssertionSource<TItem> (backed by ValueAssertion<TItem>), making specialised assertion methods like HasItemAt, ContainsKey, and IsSubsetOf unreachable. The fix is correct. The OperationCanceledException handling improvement is also valuable. However, there are architectural concerns with the chosen approach.

---

Correctness

OperationCanceledException fix — correct and important

The old bare catch { } in CollectionCountEqualsAssertion.CheckAsync would silently swallow task cancellations. The new catch (Exception ex) when (ex is not OperationCanceledException) is correct. Notably, CollectionAllSatisfyAssertion still uses the broader catch (Exception ex) which also swallows OperationCanceledException — that's a pre-existing inconsistency worth a follow-up.

---

Architectural Concern: Combinatorial Explosion at the API Surface

The chosen approach — one extension method overload per TItem shape — is a "closed world" enumeration. Every time a new specialised assertion source is introduced, a developer must remember to add a matching Count(...) extension. The PR already lists nine overloads and misses at least two practical cases.

Gap 1: Concrete types are not covered by their interface overloads.

C# generic type inference resolves TItem to the exact declared type, not to an interface. So if a user writes:

var listOfHashSets = new List<HashSet<int>> { ... };
await Assert.That(listOfHashSets).Count(s => s.IsSubsetOf(universe)).IsEqualTo(2);

TItem is inferred as HashSet<int>, not ISet<int>, so the ISet<TInner> extension overload does not match. The call falls through to the instance method and SetAssertion-specific members (IsSubsetOf, IsSupersetOf, Overlaps) are unreachable in the lambda. The same gap exists for List<T> items vs the IList<TInner> overload, Dictionary<K,V> items vs IDictionary<K,V>, etc.

Gap 2: The string overload appears redundant.

The description states the string overload prevents strings from being treated as IEnumerable<char>. In practice, C# type inference does not unify string with IEnumerable<TInner> when matching a generic constraint, so the IEnumerable<TInner> overload would never be selected for string items anyway. The instance method already wraps items in ValueAssertion<string>, which is identical to what the extension does.

---

Alternative Design Worth Considering

The root problem is that the instance method hardcodes IAssertionSource<TItem> as the lambda parameter type. The most maintainable solution would be a single method that accepts a user-supplied per-item factory:

public CollectionCountSource<TCollection, TItem> Count<TSource>(
    Func<TItem, TSource> sourceFactory,
    Func<TSource, IAssertion?> itemAssertion,
    [CallerArgumentExpression(nameof(itemAssertion))] string? expression = null)
    where TSource : IAssertionSource<TItem>

This removes the closed-world maintenance burden entirely. A middle-ground that preserves ergonomics: instead of per-type extension methods, shift ownership of "how to wrap an item" to the assertion source type itself (e.g., a static factory delegate), and have a single CountSpecialised helper call it. This way, adding a new assertion source type doesn't require touching the Count extension layer.

Neither is a blocker for merging — the current approach does solve the immediate problem — but the gap with concrete types (HashSet<T>, List<T>, Dictionary<K,V> as items) means the fix is incomplete for those cases.

---

Breaking Change — Public Constructor Removed

The public constructor of CollectionCountSource<TCollection, TItem> has been made internal. The snapshot tests correctly capture this. Since CollectionCountSource is returned from factory methods and rarely constructed directly by test authors, the real-world impact is low, but it should be noted in release notes if this library follows semver.

---

Minor: Inconsistency in Lambda Return Type

The instance method Count(Func<IAssertionSource<TItem>, Assertion<TItem>?> itemAssertion) uses Assertion<TItem>? as the lambda return type, while all new extension overloads use IAssertion?. The broader IAssertion? in the extensions is arguably better design. Worth unifying the instance method signature in a follow-up.

---

Tests

The 249-line test file is well-structured and the failure-message assertions (Contains(".Count(l => l.Count().IsEqualTo(3))")) are a good regression guard. Two suggestions:

1. Add test cases where TItem is a concrete type (List<int>, HashSet<int>, Dictionary<K,V>) rather than only interface types — this would document the current limitation (type inference gap) and detect if it's ever addressed.
2. The Count_String_Items_Use_String_Assertion_Source test would pass even without the string extension (the instance method handles it identically). If the extension is genuinely needed, the test should be designed to fail without it.

---

Summary

Concern	Severity
OperationCanceledException fix	✅ Correct, good improvement
Concrete-type gap (HashSet<T>, List<T>, etc.)	⚠️ Medium — fix is partially incomplete
String overload appears redundant	ℹ️ Low — harmless but adds noise
Removed public constructor is a breaking API change	ℹ️ Low — acceptable given usage patterns
catch in CollectionAllSatisfyAssertion still bare	📝 Pre-existing, out of scope
Test coverage of concrete types missing	ℹ️ Low

The core bug fix is correct and the approach is understandable, but the concrete-type inference gap means users relying on HashSet<int>, List<int>, etc. as item types will still hit the original issue. Consider either documenting this limitation or addressing it with a factory-based design that doesn't require per-type overloads.

[claude]

Code Review: Third Iteration

This iteration has addressed all the blocking issues raised in the previous two reviews. Here is the assessment of what remains.

What's been resolved

All three primary items from the previous review are cleanly addressed:

• Concrete-type overloads added — List<TInner>, HashSet<TInner>, and Dictionary<TKey, TValue> overloads are now present, with tests including the concrete-type cases that were previously a gap (Count_ConcreteList_Items_Reach_HasItemAt_On_Inner, Count_ConcreteHashSet_Items_Reach_IsSubsetOf_On_Inner, Count_ConcreteDictionary_Items_Reach_ContainsKey_On_Inner).
• IReadOnlySet<TInner> overload added — the gap from the first review is closed; the #if NET5_0_OR_GREATER guard is correct.
• Array<TInner> overload added — the gap from the second review is closed, with a matching test.
• Failure-message tests added for all specialised shapes — each new overload now has a corresponding failure-path test. This is good regression protection.
• OperationCanceledException fix is correct — catch (Exception ex) when (ex is not OperationCanceledException) is the right pattern for this "count items that pass" semantics.
• Public constructor removed from snapshot — the API snapshots correctly reflect the narrowing of the constructor from public to internal.

---

Remaining observations (non-blocking)

1. AllSatisfyAssertion still catches OperationCanceledException (pre-existing, out-of-scope)

CollectionAllSatisfyAssertion and CollectionAllSatisfyMappedAssertion both have catch (Exception ex) that wraps any exception — including OperationCanceledException — into AssertionResult.Failed. This is semantically different from the Count path (it's re-surfacing, not swallowing), but it still converts a cancellation into a test failure rather than propagating it. This is pre-existing and not touched by this PR; worth a follow-up issue.

2. The internal constructor ambiguity is a minor readability concern

CollectionCountSource<TCollection, TItem> now has two internal constructors:

• CollectionCountSource(AssertionContext<TCollection>, Func<IAssertionSource<TItem>, Assertion<TItem>?>?) — used by the generic instance method on CollectionAssertionBase
• CollectionCountSource(AssertionContext<TCollection>, Func<TItem, int, IAssertion?>?) — used by CountSpecialised

These are distinguished only by the delegate signature. The XML doc comments explain the distinction clearly, which is adequate. The design is functional; this is just worth flagging in case a future maintainer adds a Func<TItem, IAssertion?> variant and is surprised that it would match the first overload via implicit conversion. No action needed.

3. Instance-method lambda return type remains Assertion<TItem>? while extensions use IAssertion?

The instance method on CollectionAssertionBase:

public CollectionCountSource<TCollection, TItem> Count(
    Func<IAssertionSource<TItem>, Assertion<TItem>?> itemAssertion, ...)

...still has the narrower Assertion<TItem>? return type while all new extension overloads use the broader IAssertion?. This was raised in the first review. The asymmetry is harmless today (both resolve through the same internal factory path), but it creates an inconsistent contract for users who glance at the method signatures. A follow-up to unify the instance method to IAssertion? would be clean.

---

Overall

The bug fix is correct and complete for all the cases covered. The extension-method combinatorial approach has now been well-documented with inline comments explaining why both interface and concrete-type overloads are needed. Test coverage is solid including failure-message assertions and concrete-type cases. The API snapshot updates are correct. This is ready to merge.

[claude]

Code Review — PR #5749: fix(assertions): preserve specialised source in .Count(itemAssertion)

Summary

The PR correctly identifies and fixes a real usability bug: the Count(itemAssertion) overload was wrapping each item in a generic ValueAssertion<TItem>, which meant users working with collections-of-collections couldn't reach typed assertion methods like HasItemAt, ContainsKey, or IsSubsetOf inside the lambda.

The fix is broadly sound — but the chosen approach carries significant long-term maintenance costs that are worth discussing.

---

Primary Concern: The Overload-Explosion Pattern is a Scalability Trap

The fix adds 11 new extension overloads (interface shapes + concrete types) to the already large AssertionExtensions.cs (now ~2,077 lines). This is the same pattern that was used in #5702, and it is fragile by design.

Why this will continue to compound:

1. The same problem exists in All().Satisfy() right now. CollectionAllSatisfyAssertion also uses new ValueAssertion<TItem>(item, ...) and exposes only IAssertionSource<TItem> to its lambda — meaning users with collections-of-collections face the same limitation there. Every new *Satisfy-style method added in the future will repeat this gap.

2. Every new assertion source type (e.g. a hypothetical SortedSetAssertion, ImmutableListAssertion) would require adding more overloads here.

3. The pattern requires both interface-shape overloads and concrete-type overloads due to C# type inference not unifying concrete types with interfaces — so the number of overloads scales as O(2 x assertion_types).

Alternative worth considering: A factory-based approach would eliminate the overload explosion and apply universally. Rather than adding per-type extension methods, if Count accepted an Func<TItem, IAssertion?> where the user themselves calls Assert.That(item) to produce a typed assertion source, the dispatch would resolve to the right source type automatically — no new overloads needed per collection type. This would mean the issue dissolves naturally for all current and future collection types.

If that redesign is out of scope for this PR, it would be worth opening a tracking issue to avoid the overload pattern spreading further to All().Satisfy() and any future per-item assertion methods.

---

Correctness: Good Fix for OperationCanceledException

// Before
catch
{
    // Item did not satisfy the assertion, don't count it
}

// After
catch (Exception ex) when (ex is not OperationCanceledException)
{
    // Item did not satisfy the assertion, don't count it
}

This is a correct and important fix. Swallowing OperationCanceledException (and TaskCanceledException) would silently abort test cancellation, leaving tests hanging or producing misleading results. This fix is solid and should also be applied to the analogous catch (Exception ex) blocks in CollectionAllSatisfyAssertion and CollectionAllSatisfyMappedAssertion in CollectionAssertions.cs (lines ~689 and ~756), which currently catch Exception without filtering out cancellation exceptions.

---

Breaking Change: Public Constructor Removed

The public constructor CollectionCountSource(AssertionContext<TCollection>, Func<IAssertionSource<TItem>, Assertion<TItem>?>?) has been removed from the public API (visible in all four snapshot diffs). Both constructors are now internal.

This is a breaking change for any external code that was directly constructing CollectionCountSource. The change seems intentional but is worth explicitly calling out in the PR description.

---

Minor: Count() Null-Factory Cast

// After
return new CollectionCountSource<TCollection, TItem>(
    Context, (Func<TItem, int, IAssertion?>?)null);

The explicit cast disambiguates the two internal constructors. The approach is correct. As an alternative, a named static factory or a dedicated private constructor that takes no factory at all would eliminate the need for the cast, but this is a minor point.

---

Test Coverage

The regression tests in Issue5707Tests.cs are thorough — interface shapes, concrete types, failure message content, and array items are all covered. The approach of checking that exception messages contain the specialised lambda expression text is a good regression guard.

---

Verdict

The fix solves the reported bug correctly, the OperationCanceledException improvement is a genuine safety enhancement, and the tests are solid. The main concern is architectural: the overload-per-type pattern will need to be repeated for every similar method and every new collection assertion type going forward. It is worth discussing whether a user-side Assert.That(item) factory approach can avoid this before the pattern becomes further entrenched.