Conversation
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| BestPractice | 1 minor |
🟢 Metrics 38 complexity
Metric Results Complexity 38
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[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: Fix item-at Satisfies source typing (#5706)
The fix is correct and clearly addresses the root cause: Satisfies was constructing a ValueAssertion<TItem> (a bare wrapper over IAssertionSource<TItem>) rather than a richly-typed assertion source, so users of collection-type items lost all the specialized assertion API.
Architecture concern: combinatorial overload explosion
ListItemAtSatisfiesExtensions.cs is 311 lines covering 11 concrete collection types × 2 source types = 22 overloads. This approach works, but it won't scale. Every time a new assertion type is added to TUnit (e.g. SortedSetAssertion, ImmutableArrayAssertion, or any user-defined collection assertion type), this file must be updated with 2 more overloads. The pattern also doesn't compose — it says nothing about IList<IList<IList<T>>> or other nested structures.
A more maintainable design would expose a single generic overload that accepts a source factory:
// Single entry point — no enumeration of concrete types needed
public static ListItemAtSatisfiesAssertion<TList, TItem> Satisfies<TList, TItem, TSource>(
this ListItemAtSource<TList, TItem> source,
Func<TItem, string, TSource> sourceFactory,
Func<TSource, IAssertion?> assertion,
[CallerArgumentExpression(nameof(assertion))] string? expression = null)
where TList : IList<TItem>
where TSource : IAssertionSource<TItem>
{
// one implementation
}Usage becomes:
await Assert.That(items).ItemAt(0)
.Satisfies(
(item, label) => new CollectionAssertion<int>(item, label),
a => a.Count().IsEqualTo(3));This pushes the factory responsibility to the call site, but TUnit could still ship convenience helpers (e.g. static factory methods on each assertion type) rather than enumerating every combination. The key benefit: users can bring their own assertion source types and they'll work automatically — the current approach gives them no way to do that.
internal accessors as a design smell
// ListAssertions.cs (and ReadOnlyListAssertions.cs)
internal AssertionContext<TList> InternalListContext => _listContext;
internal int InternalIndex => _index;These were private before; they're now exposed internal solely to allow the extension class (in the same assembly) to bypass normal construction. The Internal prefix signals this awkwardness. A cleaner alternative: a factory/builder method directly on ListItemAtSource<TList, TItem> that creates the assertion given a raw factory:
internal ListItemAtSatisfiesAssertion<TList, TItem> CreateSatisfiesAssertion(
Func<TItem, int, IAssertion?> factory)
{
_listContext.ExpressionBuilder.Append(...)
return new ListItemAtSatisfiesAssertion<TList, TItem>(_listContext, _index, factory);
}This hides the internals and keeps the responsibility for mutating ExpressionBuilder inside the source type, where it belongs.
The internal assertion constructor is unguarded on the expression side
The new internal constructor of ListItemAtSatisfiesAssertion skips the expression-appending that SatisfiesSpecialised does manually:
// Extension helper appends expression here...
source.InternalListContext.ExpressionBuilder.Append($".Satisfies({expression})");
// ...then constructs via internal ctor which does NOT append anything
return new ListItemAtSatisfiesAssertion<TList, TItem>(
source.InternalListContext,
source.InternalIndex,
assertionFactory);If anyone constructs ListItemAtSatisfiesAssertion via the internal ctor without calling ExpressionBuilder.Append first, the expression will be silently incomplete. The factory-method approach above would eliminate this risk by co-locating both steps.
Test coverage gaps
The regression tests cover String, Collection, Dictionary, and Set for both IList and IReadOnlyList — solid baseline. Missing:
T[](theArrayAssertionoverload is present but untested)HashSet<T>(theHashSetAssertionoverload exists)Dictionary<K,V>concrete type (there's aMutableDictionaryAssertionoverload for it)
Given this is a regression fix, having at least a smoke test for each overload variant that was added would make future regressions much easier to catch.
Summary
The core fix is sound — changing the internal delegate from Func<IAssertionSource<TItem>, Assertion<TItem>?> to Func<TItem, int, IAssertion?> and building the specialized source at dispatch time is exactly right. The backward-compatible forwarding constructor preserves the existing public API cleanly.
The concern is that the mechanism chosen (enumerating all known concrete types as overloads) trades one problem (missing specialization) for another (a maintenance tax on every new assertion type). Worth considering whether a single generic sourceFactory parameter would be a better long-term design before this pattern spreads to other assertion combinators.
|
Addressed the review feedback in ce722f9:
Verification rerun:
|
There was a problem hiding this comment.
Code Review: Fix item-at Satisfies source typing (#5706)
The PR correctly identifies and fixes the root cause: ListItemAtSatisfiesAssertion was storing Func<IAssertionSource<TItem>, Assertion<TItem>?> which forced the lambda return type to be Assertion<TItem>? — making it impossible to use collection assertions that chain to a different Assertion<T> (e.g., Count().IsEqualTo(3) returns Assertion<int>, not Assertion<IEnumerable<int>>).
What's done well
- The internal refactor to
Func<TItem, int, IAssertion?>is clean and separates item retrieval from assertion creation correctly. - Constructor chaining preserves backward compatibility elegantly.
- The
CreateSatisfiesAssertionbridge approach (following feedback from the previous review) correctly avoids exposing internal index/context accessors. - Good test coverage across diverse concrete types.
- Public API snapshots updated properly.
Architectural Concern: The Root Fix May Be Simpler
The 303-line extension file is working around a return-type covariance constraint on the existing overload. The original signature requires Func<IAssertionSource<TItem>, Assertion<TItem>?>, but collection assertion chains return Assertion<int> (not Assertion<IEnumerable<int>>), so the lambda won't compile.
If most collection assertion extension methods (like Count()) are already defined on IAssertionSource<IEnumerable<T>> — rather than only on the concrete CollectionAssertion<T> — then adding a single overload with a relaxed return type directly on the Satisfies method would handle the majority of cases without any per-type dispatch:
// One new overload on ListItemAtSource<TList, TItem>
public ListItemAtSatisfiesAssertion<TList, TItem> Satisfies(
Func<IAssertionSource<TItem>, IAssertion?> assertion,
[CallerArgumentExpression(nameof(assertion))] string? expression = null)
{
return CreateSatisfiesAssertion(
(item, index) => assertion(new ValueAssertion<TItem>(item, $"item[{index}]")),
expression);
}With this, users can write item => item.Count().IsEqualTo(3) and the lambda compiles because IAssertion? is satisfied by the Assertion<int> return. The full 303-line extension file would only be needed if users specifically need the concrete assertion type (e.g., CollectionAssertion<int> has methods not available through IAssertionSource<IEnumerable<int>>).
Is the extension file truly necessary, or is the IAssertion? overload sufficient for all the test cases?
Scalability of the Extension File
If the per-type extension methods are genuinely needed, the static dispatch approach has inherent growth pressure:
- Missing today:
ICollection<T>,Queue<T>,Stack<T>,LinkedList<T>,SortedList<TKey,TValue>,ImmutableArray<T>,ImmutableList<T>, etc. - Any user-defined assertion source type will never work with this approach.
A more future-proof alternative for strongly-typed sources would be a factory overload:
public ListItemAtSatisfiesAssertion<TList, TItem> Satisfies<TSource>(
Func<TItem, string, TSource> sourceFactory,
Func<TSource, IAssertion?> assertion,
[CallerArgumentExpression(nameof(assertion))] string? expression = null)
where TSource : IAssertionSource<TItem>Usage:
.Satisfies(
(item, name) => new CollectionAssertion<int>(item, name),
col => col.Count().IsEqualTo(3))This covers all current and future assertion source types without adding overloads, at the cost of a slightly more verbose call site. The per-type extension methods could still be provided as ergonomic wrappers for the most common types.
Minor
- The
stringitem path works correctly through the existingFunc<IAssertionSource<TItem>, Assertion<TItem>?>overload (string assertions returnAssertion<string>). The behavior asymmetry between string items and collection items is worth a code comment noting why no specialized overload is needed forstring. #if NET5_0_OR_GREATERonIReadOnlySet<T>is correct; no issues there.
Overall the implementation is functionally correct and the previous review feedback has been addressed. The main open question is whether the simpler single-overload approach (Func<IAssertionSource<TItem>, IAssertion?>) would suffice, which would let you drop most of the extension file. If the specialized concrete types are genuinely needed for their extra methods, the factory overload pattern would scale better than the current per-type enumeration.
Replace per-type Satisfies overload bodies with delegators through a new static-abstract factory interface. Interface-shape overloads collapse to one-liners; concrete-type overloads (List<T>, Dictionary<K,V>) keep direct construction since their assertion sources implement IAssertionSourceFor against the matching interface, not the concrete type. Pattern is reusable for the remaining #5703-5707 family (Member, Count, etc.) — each new target needs only the generic Satisfies entry plus delegators, no per-shape construction logic. netstandard2.0 drops the specialised overloads (no static abstracts); net8+/net9+/net10+ get the full surface.
There was a problem hiding this comment.
Code Review
Previous feedback (constructor chaining, CreateSatisfiesAssertion encapsulation, test coverage gaps) has been addressed. The architecture is sound.
What's working well
IAssertionSourceFor<TItem, TSelf> CRTP interface — The right foundation. Using C# 11 static abstract members for a per-item factory enables fully generic dispatch without per-type overloads at the core level. The constraint where TSource : IAssertionSourceFor<TItem, TSource> correctly closes the loop.
CreateSatisfiesAssertion encapsulation — Resolves the previous concern about leaking InternalListContext/InternalIndex. The extension methods call an internal method from within the same assembly, which is the correct scope.
Backward-compatible constructor chaining — The old public ListItemAtSatisfiesAssertion(context, index, Func<IAssertionSource<TItem>, Assertion<TItem>?>) constructor chains to the new internal one. No binary breaking change, and the wrapping via new ValueAssertion<TItem> is transparent to callers.
Func<TItem, int, IAssertion?> — Passing index alongside item is essential for constructing the label $"item[{index}]" in the right place, rather than deferring label construction to CheckAsync. This is the key fix for #5706.
Remaining design concern: List<T> / Dictionary<TKey,TValue> bridging asymmetry
The comment explains the why well, but there is a cleanable root cause. ListAssertion<T> implements IAssertionSourceFor<IList<T>, ListAssertion<T>> (interface-typed), not IAssertionSourceFor<List<T>, ListAssertion<T>> (concrete-typed). So the generic Satisfies<TSource> path can't bind when the item is List<T>, forcing the explicit bridge:
// Current — two code paths
=> source.Satisfies<ListAssertion<TInner>>(assertion, expression); // via IAssertionSourceFor
=> source.CreateSatisfiesAssertion( // manual bridge
(item, index) => assertion(new ListAssertion<TInner>(item, ...)), expression);ListAssertion<T> could implement both interface variants without conflict, since the Create static methods differ in parameter type (IList<T> vs List<T>):
public class ListAssertion<TItem>
: ListAssertionBase<IList<TItem>, TItem>
#if !NETSTANDARD2_0
, IAssertionSourceFor<IList<TItem>, ListAssertion<TItem>> // existing
, IAssertionSourceFor<List<TItem>, ListAssertion<TItem>> // new — eliminates bridge
#endif
{
public static ListAssertion<TItem> Create(IList<TItem> item, string label) => new(item, label);
public static ListAssertion<TItem> Create(List<TItem> item, string label) => new(item, label);
}C# can implement two parameterizations of the same generic interface when the static abstract member signatures are distinct (different TItem → different parameter type). With this, the concrete List<T> extension overloads would reduce to simple Satisfies<ListAssertion<TInner>> calls, eliminating the asymmetric CreateSatisfiesAssertion bridge and making the code more consistent. Same pattern applies to MutableDictionaryAssertion. Not a blocker, but worth a follow-up — the inconsistency will likely surface again when users try to add their own concrete-type overloads.
Minor: extension duplication between list / readonly-list source types
Each of the 11 logical type mappings appears twice in ListItemAtSatisfiesExtensions.cs — once for ListItemAtSource<TList, TItem> and once for ReadOnlyListItemAtSource<TList, TItem>. This is intrinsic because the return types differ (ListItemAtSatisfiesAssertion vs ReadOnlyListItemAtSatisfiesAssertion). A shared interface over both source types exposing CreateSatisfiesAssertion could halve this, but the return-type variance makes that non-trivial. Noting for visibility rather than blocking the PR.
Test coverage note
Tests cover IEnumerable<int>, int[], IDictionary<string,int>, Dictionary<string,int>, ISet<int>, HashSet<int>, IReadOnlyDictionary<string,int>, IReadOnlySet<int>, and string. The IList<T> and IReadOnlyList<T> item overloads (which are among the cleanest — pure Satisfies<TSource> delegation) don't appear to have dedicated test cases. Low risk, but worth adding for completeness.
Summary
Functionally correct, all previous feedback has been addressed, and the IAssertionSourceFor CRTP foundation is genuinely extensible — users can write Satisfies<MyCustomAssertion>(...) for types TUnit doesn't know about. The main open item is the List<T>/Dictionary<TKey,TValue> concrete-type bridging asymmetry, which is cosmetic now but becomes a maintenance tax as the pattern propagates.
ListAssertion and MutableDictionaryAssertion now implement two IAssertionSourceFor parameterisations each (interface + concrete item types), enabling the concrete List<T> and Dictionary<K,V> Satisfies overloads to delegate through the generic Satisfies<TSource> path instead of bypassing it via direct CreateSatisfiesAssertion calls. This eliminates the per-type construction asymmetry that would otherwise propagate to every future combinator following the same pattern. IAssertionSourceFor no longer extends IAssertionSource<TItem> — the framework only ever calls TSource.Create(...), and the multi-variant implementation is what required dropping the base (an IAssertionSource<List<T>> Context property cannot be derived from AssertionContext<IList<T>> without invariance breakage). Concrete classes still expose IAssertionSource<TItem> through their existing base hierarchies, so user lambdas see no surface change. Adds nested-list test coverage: IList<IList<T>>, IList<IReadOnlyList<T>>, IReadOnlyList<IList<T>>, IReadOnlyList<IReadOnlyList<T>>.
There was a problem hiding this comment.
Code Review: Fix item-at Satisfies source typing — iteration 4
All prior feedback has been fully addressed. This review covers only the new commit (0080687e).
What changed since the last review
The latest commit does three things:
IAssertionSourceFor<TItem, TSelf>no longer extendsIAssertionSource<TItem>— required to allowListAssertion<T>andMutableDictionaryAssertion<T>to implement two parameterizations of the interface simultaneously.ListAssertion<T>andMutableDictionaryAssertion<TKey,TValue>implement both the interface-typed and concrete-typed variants (IAssertionSourceFor<IList<T>>andIAssertionSourceFor<List<T>>), so the concrete overloads in the extension file now delegate through the genericSatisfies<TSource>path without anyCreateSatisfiesAssertionbypass.- Test coverage added for
IList<IList<T>>,IList<IReadOnlyList<T>>,IReadOnlyList<IList<T>>, andIReadOnlyList<IReadOnlyList<T>>— directly addresses the gap flagged in the last review.
All three of these were explicitly flagged as open items in the previous review, so well done on the quick turnaround.
Removing IAssertionSourceFor : IAssertionSource<TItem>
This is the right call. The commit message explains the invariance problem precisely, and the <remarks> doc on the interface explains the design rationale for future maintainers. The public API snapshot reflects the change correctly. Since IAssertionSourceFor was never shipped in a release, removing the inheritance is not a breaking change.
One small thing to verify: the API snapshots show that ArrayAssertion<TItem>, CollectionAssertion<TItem>, ReadOnlyListAssertion<TItem>, etc. now list fewer interfaces in their type hierarchy (the transitively-inherited IAssertionSource and IAssertionSource<TItem> that came from IAssertionSourceFor are no longer shown). The underlying implementations are unaffected — those interfaces are still satisfied through the base class hierarchy — so this is a snapshot artifact, not a behavior regression.
Minor gap: IList<List<T>> case not tested
The refactored List<T> overloads now route through Satisfies<ListAssertion<TInner>> (via the new IAssertionSourceFor<List<T>, ListAssertion<T>> implementation). This is the code path most changed by this commit, yet it has no dedicated test. All existing tests use IList<IList<T>>, not IList<List<T>>. A minimal test case:
[Test]
public async Task List_ItemAt_Satisfies_Preserves_ConcreteList_Item_Source()
{
IList<List<int>> items = new List<List<int>> { new() { 1, 2, 3 } };
await Assert.That(items).ItemAt(0).Satisfies(item => item.Count().IsEqualTo(3));
}Same applies to IList<Dictionary<string,int>> (the concrete dictionary path). Low risk since the types are equivalent at the IList<T> / IDictionary<K,V> level, but a regression test for exactly the code path changed this round would be good practice.
Summary
The implementation is clean, the internal Satisfies<TSource> path is now used consistently for both concrete and interface item types, the CreateSatisfiesAssertion bypass is gone, and the test coverage is substantially improved. The only remaining suggestion is smoke tests for the List<T> and Dictionary<K,V> concrete-item paths that are specifically enabled by this commit. No blockers.
Adds the IList<List<int>> case the previous round missed — this is the concrete-list path now routed through Satisfies<ListAssertion<TInner>> via the new IAssertionSourceFor<List<T>, ListAssertion<T>> implementation. Also documents the why behind the interface- and concrete-shaped overload coexistence on ListItemAtSatisfiesExtensions: C# overload resolution is exact on TItem, so IList<List<int>> binds the List<TInner> overload and never the IList<TInner> one.
Code Review: Fix item-at Satisfies source typing (#5764)This is a well-structured fix for a real usability problem. The core issue is that What the PR does wellBackward compatibility is preserved correctly. The original public constructor
Expression string is correctly passed through. The
Issues and observations1.
|
1 participant
Summary
Verification
Closes #5706