Add single-item assertion chaining#6395
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 6 |
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58b1b5eaf2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| get | ||
| { | ||
| Context.ExpressionBuilder.Append(".Item"); | ||
| Context.SetPendingLink(InternalWrappedExecution ?? this, CombinerType.And); |
There was a problem hiding this comment.
Guard .Item against existing Or chains
When .Item is reached after an Or chain (for example Assert.That(Array.Empty<int>()).IsEmpty().Or.HasSingleItem().Item.IsEqualTo(0)), this unconditionally wraps the existing OrAssertion in an And link. Because OrAssertion can short-circuit without executing HasSingleItem, _singleItem remains default, so the item assertion can pass even though no single item was captured. This should reject the mixed Or/drill-in case, like the normal .And path does, before installing the pending And link.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Review: Add single-item assertion chaining (#6395)
Clean, well-scoped feature. The core mechanism — capturing _singleItem as a side effect of CheckAsync(), then reading it lazily via Context.Map<TItem>(_ => _singleItem) gated by Context.SetPendingLink(InternalWrappedExecution ?? this, CombinerType.And) — is correct: Assertion.ExecuteCoreAsync() always runs PendingPreWork (which executes the whole preceding .And/.Or chain, including wrapped executions) before it calls Context.GetAsync() on the new context, so _singleItem is guaranteed to be populated before .Item's value is read. This deliberately diverges from the existing DictionaryValueSource.Value drill-in (which re-derives the value from the raw parent value inside the Map callback rather than from a mutable field) — that's the right call here, since re-deriving via re-enumeration isn't possible for one-shot IEnumerable<T> sources. Good test coverage of that exact scenario (Test_List_HasSingleItem_Item_Does_Not_Reenumerate), plus predicate, chaining-preservation, and collection-shape cases. Snapshot and docs updates are in sync with the code change.
Finding: .Item silently evaluates against default(TItem) inside Assert.Multiple when the preceding HasSingleItem fails
Repro (verified locally, not in the diff):
using (Assert.Multiple())
{
await Assert.That(new List<int>()).HasSingleItem().Item.IsEqualTo(999);
}Outside Assert.Multiple, a failing HasSingleItem() throws before .Item's mapper ever runs (ThrowOrAccumulateFailure throws synchronously, so PendingPreWork propagates the exception and Context.GetAsync() on the new context is never reached) — this path is correctly tested. But inside Assert.Multiple, ThrowOrAccumulateFailure accumulates the failure into the AssertionScope instead of throwing, so ExecuteCoreAsync() returns normally. The .Item mapper (_ => _singleItem) then runs against the untouched field, which is default(TItem) (0/null/etc. — never reset in the failure path since it's just never assigned), and the downstream assertion (.IsEqualTo(999)) evaluates for real. In the repro above this produces two accumulated failures with IsEqualTo(999), and — more concerning — if the expected value happens to equal default(TItem) (e.g. .IsEqualTo(0) for an int), the item-level assertion silently passes, masking that no item was ever captured.
This is a narrow edge case (only surfaces inside Assert.Multiple), and it may be an existing limitation shared by other side-effect-captured drill-ins in this file (e.g. the GetAwaiter() "await for the item" pattern has the same shape), so it's not necessarily a blocker for this PR alone. But given .Item is the new public surface being added here, it'd be worth either: (a) short-circuiting .Item's evaluation when the preceding check failed (e.g. have the mapper reference the actual AssertionResult/failure state, not just the raw field), or at minimum (b) adding a regression test under Assert.Multiple so this doesn't quietly ship as an untested gap.
Minor: duplicated Item property (not blocking)
The Item get-accessor is duplicated verbatim across HasSingleItemAssertion, HasSingleItemPredicateAssertion, and MemoryHasSingleItemAssertion (4 identical lines × 3). Given the codebase already centralizes the analogous .And/.Or continuation wiring in shared base-class constructors (SetPendingLink is called once, in the continuation's base ctor, for every other assertion type), consider a small shared helper (e.g. a protected method on a common base, or a static helper taking Assertion<TValue>, AssertionContext, and the captured item) so a future change to this drill-in mechanism only needs to happen once. Not urgent given the current size, but worth avoiding a fourth copy if more drill-ins are added.
Everything else — naming, nullability, AOT/source-gen compatibility ([GenerateCollectionShapeAssertions] reuses the existing generator path), and the public API diff — looks consistent with the rest of the assertions library.
There was a problem hiding this comment.
Follow-up review: fix commit cfde8e9
This addresses the finding from my previous review (.Item silently evaluating against default(TItem) inside Assert.Multiple when the preceding HasSingleItem failed).
Fix verified correct. Assertion.ExecuteCoreAsync() (TUnit.Assertions/Core/Assertion.cs:130-147) now snapshots AssertionScope.ExceptionCount before running Context.PendingPreWork, and short-circuits with return default; if it grew — i.e. the parent assertion accumulated a failure into the scope instead of throwing. This is the same exception-count-delta idiom already used in AndAssertion.AssertAsync() and OrAssertion.AssertAsync() (TUnit.Assertions/Chaining/{And,Or}Assertion.cs), so it's consistent with the rest of the codebase rather than a one-off special case. Because the guard lives in the shared PendingPreWork path (not SingleItemSource specifically), it also closes the same latent gap for other cross-type drill-ins built on PendingPreWork (e.g. WhenParsedInto<T>), which is a nice bonus beyond the immediate .Item case.
The new regression test Test_List_HasSingleItem_Item_Is_Not_Evaluated_After_Failure_In_Assert_Multiple (ListAssertionTests.cs) exercises exactly the reported scenario and asserts the downstream .Satisfies body never runs. Good coverage.
Still open (non-blocking, as noted previously): the Item get-accessor remains duplicated verbatim across HasSingleItemAssertion, HasSingleItemPredicateAssertion, and MemoryHasSingleItemAssertion. Not a blocker for this PR, but worth a shared helper if a fourth drill-in shows up.
No new issues found in this delta. Looks good to merge from my side.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cfde8e9933
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ThrowIfMixingCombiner<Chaining.OrAssertion<TCollection>>(); | ||
| Context.ExpressionBuilder.Append(".Item"); | ||
| Context.SetPendingLink(InternalWrappedExecution ?? this, CombinerType.And); | ||
| return new SingleItemSource<TItem>(Context.Map<TItem>(_ => _singleItem)); |
There was a problem hiding this comment.
Ensure item Or chains cannot bypass HasSingleItem
When an item-level .Or is added, the parent HasSingleItem runs as PendingPreWork only for the first branch; its failure is caught by OrAssertion as if the first item assertion failed, then the prework is cleared and the second branch reads _singleItem's default value. For example, await Assert.That(Array.Empty<int>()).HasSingleItem().Item.IsEqualTo(1).Or.IsEqualTo(0) can pass even though there is no single item. The parent check needs to gate the whole item assertion chain rather than just one Or alternative.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff74d0054f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ThrowIfMixingCombiner<Chaining.OrAssertion<TCollection>>(); | ||
| Context.ExpressionBuilder.Append(".Item"); | ||
| Context.SetPendingLink(InternalWrappedExecution ?? this, CombinerType.And); | ||
| return new SingleItemSource<TItem>(Context.Map<TItem>(_ => _singleItem)); |
There was a problem hiding this comment.
Preserve HasSingleItem prework through Item mappings
The .Item source relies on PendingPreWork to run HasSingleItem, but many first operations on an IAssertionSource immediately map the context again (for example the documented .Item.Member(...), .Item.Length(), or .Item.IsTypeOf<T>() paths), and those maps do not carry this pending prework. In those cases the parent assertion never executes, so _singleItem is still its default value: the docs sample fails for a valid single User, and inputs such as Array.Empty<string>().HasSingleItem().Item.Length().IsEqualTo(0) can pass against the default item instead of failing the missing single-item check. Please make the item source preserve or execute the parent prework across subsequent mappings.
Useful? React with 👍 / 👎.
Follow-up review: fix commit
|
Review: Add single-item assertion chaining (#6395)The Finding: the
|
e31e77a to
9846aef
Compare
Summary
.Itemdrill-in afterHasSingleItem()and its predicate overloadValidation
dotnet test TUnit.Assertions.Tests/TUnit.Assertions.Tests.csproj— 6,541 passed across net8.0, net9.0, and net10.0dotnet test TUnit.PublicAPI --no-build— 18 passed across net472, net8.0, net9.0, and net10.0Closes #6221