Add AssertionResult.Failed overload that accepts an Exception

[thomhurst]

Summary

Closes #5381

• Add Exception? property to AssertionResult and AssertionResult<T> with a new Failed(string, Exception?) overload
• Thread the exception through Assertion<T>.CreateException() as an inner exception on AssertionException
• Update all 22 catch sites across assertions (collections, lists, mapped, regex, JSON, completesWithin, waitsFor, innerExceptions) and mock assertions to pass caught exceptions through

Test plan

• Full solution builds with 0 errors
• Public API snapshot tests updated and passing (net10.0, net472)
• Backward compatible — existing Failed(string) overload unchanged

[claude]

Code Review

This is a clean, well-executed feature that solves a real pain point — assertion failures that swallow the original cause. The implementation is backward-compatible, consistently applied across all 22 catch sites, and snapshot tests are updated correctly. Good work overall.

Design Concern: Double-Wrapping in WaitsForAssertion

The most interesting edge case is in WaitsForAssertion.cs:

// lastException is an AssertionException caught from assertion.AssertAsync()
catch (AssertionException ex) { lastException = ex; }

// ...then later:
return AssertionResult.Failed(..., lastException);

// ...which in Assertion.CreateException becomes:
new AssertionException(outerMessage, lastException) // AssertionException wrapping AssertionException

This creates an AssertionException whose InnerException is another AssertionException. The outer message already includes the last error text via ExtractAssertionMessage(lastException), so the inner exception is somewhat redundant and can produce confusing stack traces.

A cleaner approach might be to attach lastException.InnerException instead — preserving the original root cause without the redundant assertion wrapping layer. Alternatively, the WaitsFor case could be treated as a special case where the inner exception isn't threaded (keep null), since the error message is already extracted and embedded in the outer message. Worth deciding intentionally.

API Asymmetry: AssertionResult<T> Missing Public Failed Overload

AssertionResult gains a public Failed(string, Exception?) overload, but AssertionResult<T>.Failed is internal:

// AssertionResult — public ✓
public static AssertionResult Failed(string message, Exception? exception) => new(false, message, exception);

// AssertionResult<T> — internal only, no exception overload
internal static AssertionResult<T> Failed(string message) => new(false, message, default);

The implicit conversion operator correctly propagates the exception, so the common case is handled. But if a custom assertion ever returns AssertionResult<T> directly (rather than implicitly converting from AssertionResult), there's no way to attach an exception. This is a minor gap but worth noting if AssertionResult<T> is part of the extensibility surface.

Minor: FailIf Has No Exception Overload

FailIf(bool condition, string message) is a convenience helper. Since exceptions are typically only available in catch blocks (where you're already constructing the result directly), this is low priority — but worth keeping in mind for consistency if the API evolves further.

What's Working Well

• Backward compatibility is maintained cleanly with the additive overload
• The implicit conversion AssertionResult → AssertionResult<T> correctly threads result.Exception through (line 44 in AssertionResult{T}.cs)
• Assertion.CreateException() applies the inner exception only when present — no null safety issues
• All catch sites consistently updated, including the mock assertions in a separate project
• Public API snapshots updated across all target frameworks

The double-wrapping concern in WaitsForAssertion is the one item I'd recommend addressing before merge, as it's the only case where the attached exception is itself an AssertionException rather than an originating cause.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity

Metric	Results
Complexity	0

View in Codacy

_{TIP This summary will be updated as you push new changes. Give us feedback}

[claude]

Follow-up Review

The second commit ("Add context to GroupAssertion error messages") is a nice improvement — the error messages are now more contextual and consistent with how other assertion failures report their location (e.g. failed to get group 'name': ... rather than the bare exception message).

Outstanding: Double-Wrapping in WaitsForAssertion Still Present

My previous concern about WaitsForAssertion has not been addressed. The lastException being threaded through is itself an AssertionException (caught on line 63), so the resulting exception chain becomes:

AssertionException: "assertion did not pass within 1000ms after N attempts. Last error: ..."
  └── InnerException: AssertionException: "expected X but was Y"
                        └── (possibly another inner exception chain)

The outer message already includes the last assertion error text via ExtractAssertionMessage, so the InnerException is a duplicate. More importantly, it's misleading — the InnerException on an exception conventionally means "this exception happened because of the inner one," but here the inner exception is the last of potentially many failures.

Two cleaner options:

Option A — Unwrap to the original cause:

return AssertionResult.Failed(
    $"assertion did not pass within ...", 
    lastException?.InnerException ?? lastException);

This preserves the originating exception (e.g. a NullReferenceException that caused the assertion to fail) without the redundant AssertionException wrapper.

Option B — Skip the inner exception for this case (keep null):

return AssertionResult.Failed(
    $"assertion did not pass within ...");  // no exception — message is self-contained

Since ExtractAssertionMessage already embeds the last error in the message, the inner exception is low-value here and can produce confusing traces.

Option A is probably better if the original inner exception (e.g. the cause of the last AssertionException) is useful for debugging. Option B is simpler and avoids confusion if the inner exception is typically another AssertionException with no originating cause.

Everything else looks good — the GroupAssertion improvement is a nice touch, and the rest of the changes remain solid.

[claude]

Follow-up Review (commit 3c538ca)

The third commit is a significant expansion — propagating exceptions through the ~110 remaining metadata.Exception guard sites and adding the _passed static readonly cache. Both are good. The caching micro-optimization is the right call here (cold path is now two comparisons rather than a struct allocation).

Outstanding: WaitsForAssertion Double-Wrapping Still Present

My concern from the previous two reviews remains unaddressed. The current state of WaitsForAssertion.cs still passes lastException (an AssertionException) as the inner exception:

// lastException is caught as AssertionException (line 63)
catch (AssertionException ex) { lastException = ex; }

// ...then passed through:
return AssertionResult.Failed(
    $"assertion did not pass within {_timeout.TotalMilliseconds:F0}ms after {attemptCount} attempts. {lastErrorMessage}",
    lastException);  // ← this is itself an AssertionException

This produces a misleading chain:

AssertionException: "assertion did not pass within 1000ms after N attempts. Last error: expected X but was Y"
  └── InnerException: AssertionException: "expected X but was Y"   ← duplicate

The outer message already contains ExtractAssertionMessage(lastException), so the InnerException is pure duplication, and it's semantically wrong — InnerException conventionally means "the root cause", not "the last of N transient failures".

The fix is a one-liner:

// Option A — unwrap to the originating cause (keeps diagnostic value):
return AssertionResult.Failed(
    $"assertion did not pass within ...",
    lastException?.InnerException ?? lastException);

// Option B — omit the inner exception (message is self-contained):
return AssertionResult.Failed(
    $"assertion did not pass within ...");

New Finding: MatchIndexAssertion Inconsistency

The third commit updated MatchIndexAssertion.cs but left an inconsistency in the if (exception != null) block:

if (exception != null)
{
    if (exception is ArgumentOutOfRangeException || exception is InvalidOperationException)
    {
        return Task.FromResult(AssertionResult.Failed(exception.Message));  // exception NOT propagated
    }
    return Task.FromResult(AssertionResult.Failed($"threw {exception.GetType().Name}", exception));  // propagated ✓
}

The first path loses the exception object. Since the goal of this PR is to preserve original exceptions for debugging, this branch should also pass the exception:

return Task.FromResult(AssertionResult.Failed(exception.Message, exception));

These two exceptions (ArgumentOutOfRangeException, InvalidOperationException) represent legitimate error causes — not passing them through is inconsistent with everything else in this PR.

Summary

Concern	Status
Core API design (AssertionResult.Failed(string, Exception?))	✅ Good
Backward compatibility	✅ Maintained
~110 additional metadata.Exception sites	✅ Done in commit 3
_passed allocation optimization	✅ Nice
Implicit conversion propagation (AssertionResult<T>)	✅ Correct
WaitsForAssertion double-wrapping	❌ Still outstanding
MatchIndexAssertion inconsistency	❌ New finding

The WaitsForAssertion issue is the blocking item — it's the only place where the "inner exception" is itself an AssertionException, inverting the intent of the feature.

[claude]

Code Review: Add AssertionResult.Failed overload that accepts an Exception

The feature itself is well-motivated and the mechanical propagation across 40+ files is correct. A few observations on design and completeness:

---

1. Two missed propagation sites in StringAssertions.cs

Line 546 (HasLengthAssertion.CheckAsync) still uses the old overload without forwarding the exception:

// Line 546 - exception is in scope but not forwarded
return AssertionResult.Failed($"threw {exception.GetType().Name}");

Line 666 (IsMatchingRegexAssertion.CheckAsync) uses exception.Message but also omits the exception itself:

return Task.FromResult(AssertionResult.Failed(exception.Message));

These two sites were missed in the sweep. The second one also intentionally omits the exception type from the message, which is inconsistent with every other metadata.Exception guard (all of which say "threw {type.Name}").

---

2. AssertionResult<T> still has no public Failed(string, Exception?) overload

The non-generic AssertionResult gained public static Failed(string, Exception?), but AssertionResult<T> only has:

internal static AssertionResult<T> Failed(string message) => new(false, message, default);

The Exception property is propagated correctly through the implicit conversion operator, but any assertion that directly creates AssertionResult<T>.Failed(...) cannot attach an exception. If AssertionResult<T> is ever used in a catch site (generated assertions via [GenerateAssertion], for instance), there is no ergonomic path. Consider:

internal static AssertionResult<T> Failed(string message, Exception? exception = null)
    => new(false, message, default, exception);

---

3. FailIf has no exception-carrying overload

FailIf(bool condition, string message) is in the public API but has no FailIf(bool condition, string message, Exception? exception) counterpart. Any caller that wants to conditionally fail with a contextual exception must manually write condition ? Failed(msg, ex) : Passed, breaking the helper's value. The symmetric approach would be:

public static AssertionResult FailIf(bool condition, string message, Exception? exception)
    => condition ? Failed(message, exception) : Passed;

---

4. WaitsForAssertion unwrapping may lose the inner exception chain

lastException?.InnerException ?? lastException

WaitsForAssertion passes lastException.InnerException to avoid the AssertionException → AssertionException chain. This is reasonable, but if the original assertion itself wrapped a cause (e.g., an EqualityException that itself has an InnerException), the intermediate AssertionException is the richer object: it carries the formatted message the user wrote. Silently unwrapping one level means the inner exception in the final AssertionException is the raw comparison exception with no assertion context.

A safer approach would be to preserve the whole AssertionException as the inner exception and rely on the outer AssertionException's message (already including "Last error: …") to provide context, rather than reaching into its InnerException. If the concern is readability of the exception chain, add a helper property/method to distinguish "assertion failed" wrapping from "original cause" wrapping, rather than unwrapping by convention.

---

5. The _passed field initialization race for the _passedTask

private static readonly AssertionResult _passed = new(true, string.Empty);
internal static readonly Task<AssertionResult> _passedTask = Task.FromResult(Passed);

_passedTask calls the Passed property (=> _passed) at field-initializer time, which is fine because static field initializers run top-to-bottom. However, the intent of the _passed field is to cache the struct to avoid per-call allocations on the value type. Since AssertionResult is a readonly struct, returning it from a property copies the struct on every call — the cached field helps avoid the allocation of a new instance each time Passed is referenced. This is a good optimization but might benefit from a comment explaining why _passed exists (since the optimization is subtle for a readonly struct returned by value).

---

6. No tests added for the new Exception propagation

The PR updates public API snapshots and mentions builds pass, but there are no new test cases demonstrating that InnerException is actually populated on a thrown AssertionException. Given the breadth of catch sites changed, at least one integration test covering a representative case (e.g., a collection assertion where the evaluated delegate threw, verifying AssertionException.InnerException != null) would give confidence that the threading actually works end-to-end.

---

Summary

Area	Verdict
Core design (AssertionResult.Exception + threading)	Good — clean additive API, backward-compatible
Mechanical propagation across 40+ files	Correct, with two missed sites in StringAssertions.cs
AssertionResult<T> parity	Incomplete — no Failed(string, Exception?) overload
FailIf parity	Missing matching overload
WaitsForAssertion unwrapping	Works but may silently drop exception context
Test coverage	None added for the new behavior

The foundation is solid and the direction is right. The two missed sites and the AssertionResult<T> / FailIf gaps are the most actionable items before merging.

[claude]

Code Review — Final State (commit 225b65f)

This review covers the full PR as it stands after all commits. Overall this is a well-executed, backward-compatible feature that meaningfully improves debuggability by preserving original exceptions through the assertion pipeline. The iterative review/fix cycle landed in a good place.

---

What's Now Resolved

All concerns from previous review rounds have been addressed in the final commit:

• MatchIndexAssertion inconsistency — both branches in the if (exception != null) guard now correctly propagate the exception.
• StringAssertions.cs missed site (line 613, IsMatchingRegexAssertion) — now fixed.
• AssertionResult<T>.Failed(string, Exception?) overload — added for parity with the non-generic type.
• WaitsForAssertion unwrapping decision — reverted with a clear rationale in the commit message (the full AssertionException carries the formatted assertion message, so keeping it as InnerException is intentional).

---

Remaining Design Observations

1. WaitsFor: AssertionException as InnerException is a deliberate choice but worth documenting

The commit message explains the rationale for keeping the full AssertionException as InnerException in WaitsForAssertion. That reasoning is sound — the formatted assertion message (e.g. "Expected length to be 5 but received 3") is more useful than the raw originating exception alone. However, this means the InnerException convention is used differently here than everywhere else in the PR:

• All other sites: InnerException = the originating cause (e.g. NullReferenceException, JsonException)
• WaitsForAssertion: InnerException = an AssertionException describing the last failure

This inconsistency is acceptable given the semantics of WaitsFor, but it may surprise users who inspect the exception chain programmatically. A short XML doc comment on WaitsForAssertion.CheckAsync (or even just an inline // The inner exception is the last AssertionException, not an originating cause comment) would make the intent clear for future maintainers.

2. FailIf still has no exception-carrying overload

FailIf(bool condition, string message) is in the public API but there is no FailIf(bool condition, string message, Exception? exception) counterpart. This is a minor gap — catch blocks typically construct Failed directly rather than using FailIf — but the helper is now asymmetric with Failed. Worth adding for completeness if FailIf sees external use.

3. No regression test added for the new Exception propagation

The PR snapshot-tests the public API shape (good), but there are no behavioral tests verifying that AssertionException.InnerException is actually populated when an assertion fails on a delegate that threw. A single test like:

[Test]
public async Task AssertionException_Contains_InnerException_When_Delegate_Threw()
{
    var ex = await Assert.ThrowsAsync<AssertionException>(
        () => Assert.That(() => ThrowingMethod()).IsNotNull());
    
    await Assert.That(ex.InnerException).IsNotNull();
    await Assert.That(ex.InnerException).IsTypeOf<InvalidOperationException>();
}

would give confidence that the threading actually works end-to-end across the full evaluation pipeline (not just at the AssertionResult struct level).

4. Minor: _passed field and Passed property allocation subtlety

The _passed static field caches a readonly struct instance to back the Passed property. For a readonly struct returned by value, this avoids creating a new instance on each call (the property previously called new(true, string.Empty) on every access). The optimization is correct and worthwhile given Passed is called in every hot path. A brief comment alongside _passed would help future readers understand why the field exists alongside the property.

---

Summary

Item	Status
Core API design	Good — clean, additive, backward-compatible
All catch sites updated	Complete — no missed sites in final state
AssertionResult<T> parity	Done in final commit
Implicit conversion propagation	Correct
WaitsForAssertion inner exception	Intentional and documented in commit message; minor inline comment would help
MatchIndexAssertion inconsistency	Fixed
FailIf parity	Still missing, low priority
Behavioral test coverage	None added — worth a follow-up
Public API snapshots	Updated correctly

The PR is in good shape. The two actionable items I'd suggest before merge are: (1) a brief inline comment in WaitsForAssertion explaining the InnerException semantics, and (2) at least one integration test for the exception-threading behavior. Everything else can be follow-up work.

[claude]

Final State Review (commit 1a03024)

This is the follow-up review covering the last commit, which addresses the two remaining items from the previous round.

What Was Addressed

Integration tests added — AssertionResultExceptionPropagationTests.cs adds two behavioral tests:

• Nested_Collection_Assertion_Failure_Preserves_InnerException — verifies InnerException is set on a collection All().Satisfy() failure
• Mapped_Assertion_Failure_Preserves_InnerException — verifies InnerException is set on a Satisfies() mapped assertion failure

These cover the core threading path. The tests are clear and idiomatic.

WaitsForAssertion inline comment added — The // Inner exception is the last AssertionException (not an originating cause) — kept whole because it carries the formatted assertion message useful for debugging. comment makes the intentional design decision clear for future maintainers. Good.

---

Remaining Minor Gaps

FailIf still lacks an exception-carrying overload — FailIf(bool condition, string message) remains asymmetric with Failed(string, Exception?). This has been flagged in three consecutive reviews. It is genuinely low priority (catch blocks naturally call Failed directly), but it is a now-visible inconsistency in the public API surface. Consider adding:

public static AssertionResult FailIf(bool condition, string message, Exception? exception)
    => condition ? Failed(message, exception) : Passed;

Test coverage is narrow — The two added tests verify the happy path for two specific assertion types (All().Satisfy() and Satisfies()). None of the other changed assertion types (WaitsFor, collection, list, JSON, completesWithin, regex, mock assertions) have corresponding tests. The WaitsFor case in particular has unique semantics (the inner exception is an AssertionException) and would benefit from its own test. These are not blocking for merge but are worth follow-up issues.

AssertionResult<T>.Failed internal visibility — The internal overload AssertionResult<T>.Failed(string, Exception?) is now present (added in commit 225b65f), which resolves the parity gap. Worth noting: if this type ever becomes part of a formal extensibility surface (e.g. for third-party [GenerateAssertion] implementations outside the assembly), the internal visibility will prevent external consumers from using it. No action needed now, but worth revisiting if extensibility requirements change.

---

Summary

Item	Status
Core API (AssertionResult.Failed(string, Exception?))	Done
All catch sites propagated	Done
AssertionResult<T> parity	Done
Implicit conversion propagation	Correct
WaitsForAssertion double-wrapping decision + comment	Done
MatchIndexAssertion inconsistency	Fixed
Integration tests for exception threading	Added (two cases)
FailIf exception overload	Still missing — low priority
Broader test coverage (WaitsFor, collections, JSON, etc.)	Not added — follow-up recommended

The PR is in a mergeable state. The FailIf gap and broader test coverage are the only remaining actionable items, both appropriate for follow-up issues rather than blocking merge. Well done getting through a thorough iterative review cycle.