Skip to content

Add WithInnerExceptions() for fluent AggregateException assertion chaining#5380

Merged
thomhurst merged 6 commits intomainfrom
feature/with-inner-exceptions
Apr 4, 2026
Merged

Add WithInnerExceptions() for fluent AggregateException assertion chaining#5380
thomhurst merged 6 commits intomainfrom
feature/with-inner-exceptions

Conversation

@thomhurst
Copy link
Copy Markdown
Owner

Summary

Closes #5345

  • Adds WithInnerExceptions() method to both ThrowsAssertion<TException> and ThrowsExactlyAssertion<TException>, enabling fluent assertion chaining on AggregateException.InnerExceptions
  • Uses delegate-based inline assertion pattern (consistent with existing Count(c => c.IsEqualTo(5)) pattern)
  • The delegate receives a CollectionAssertion<Exception>, allowing full collection assertion chaining (Count, All().Satisfy, Contains, etc.)

Usage

await Assert.That(action)
    .Throws<AggregateException>()
    .WithInnerExceptions(e => e.Count().IsEqualTo(3));

await Assert.That(action)
    .Throws<AggregateException>()
    .WithInnerExceptions(e => e
        .All().Satisfy(ex => ex.IsTypeOf<ArgumentException>()));

await Assert.That(action)
    .ThrowsExactly<AggregateException>()
    .WithInnerExceptions(e => e.Count().IsEqualTo(2));

Test plan

  • 6 tests covering Throws and ThrowsExactly entry points, count assertions, All().Satisfy, failure on wrong count, failure on mixed types, and failure on non-AggregateException
  • Existing WithInnerException tests pass (no regressions)
  • Public API snapshot updated

Copy link
Copy Markdown
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

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

Code Review: Add WithInnerExceptions() for fluent AggregateException assertion chaining

Overall this is a well-conceived addition that fills a real gap — there was previously no ergonomic way to assert on AggregateException.InnerExceptions in TUnit. The delegate-based inline pattern is consistent with the existing Count(c => c.IsEqualTo(5)) API, and the tests are comprehensive. A few issues worth addressing before merge:


Issue 1: WithInnerExceptionsAssertion<TException> is unnecessarily public (minor but has API surface implications)

WithInnerExceptionsAssertion<TException> is declared public, but it is only ever constructed internally (the constructor is internal). Existing parallel types like ExceptionInnerExceptionOfTypeAssertion are also public, so this is consistent — however, it registers as a new exported type in the public API snapshot, meaning it becomes a permanent commitment. If the type is only ever returned from the two WithInnerExceptions factory methods and never needs to be referenced by name in user code, consider whether it needs to be public at all, or whether it can be an implementation detail behind a sealed return type.


Issue 2: CheckAsync does not check metadata.Exception before accessing metadata.Value

Every other CheckAsync implementation in this codebase (e.g. ExceptionMessageContainsAssertion, ExceptionInnerExceptionOfTypeAssertion) begins with:

var evaluationException = metadata.Exception;
if (evaluationException != null)
    return Task.FromResult(AssertionResult.Failed($"threw {evaluationException.GetType().FullName}"));

WithInnerExceptionsAssertion.CheckAsync skips this guard entirely and goes straight to metadata.Value. In practice this may not surface a bug today because the throws pipeline puts the caught exception into Value via MapException, but it is inconsistent with the established pattern and will silently ignore unexpected evaluation errors. It should mirror the guard used in all sibling assertions.


Issue 3: Swallowing the inner assertion's failure message and losing context

catch (Exceptions.AssertionException ex)
{
    return AssertionResult.Failed($"inner exceptions assertion failed: {ex.Message}");
}

The ex.Message from AssertionException already contains the full formatted output (expectation, actual value, and the expression chain from the nested Assert.That(InnerExceptions)…). Wrapping it in "inner exceptions assertion failed: " causes it to be wrapped a second time by the outer CreateException() call in Assertion<T>.ExecuteCoreAsync, producing messages like:

Expected to have inner exceptions satisfying assertion
but inner exceptions assertion failed: Expected to have count equal to 3
but received 2

at Assert.That(action).Throws<AggregateException>().WithInnerExceptions(exceptions => exceptions.Count().IsEqualTo(3))

The prefix "inner exceptions assertion failed: " adds noise and is redundant. The raw ex.Message (or even just a trimmed form of it) would read more naturally. Compare how CollectionCountWithInlineAssertionAssertion handles this — it discards the inner exception entirely and emits just "count was {_actualCount}", which is cleaner but loses specifics. For exception assertions the full nested message is valuable, but the prefix wrapping should be removed or the message should be passed through unchanged.


Issue 4: The expression parameter appended to ExpressionBuilder before the assertion runs

In both ThrowsAssertion<TException>.WithInnerExceptions and ThrowsExactlyAssertion<TException>.WithInnerExceptions:

Context.ExpressionBuilder.Append($".WithInnerExceptions({expression})");
return new WithInnerExceptionsAssertion<TException>(Context, innerExceptionsAssertion);

The [CallerArgumentExpression] expression captures the source-text of the delegate (e.g. "exceptions => exceptions.Count().IsEqualTo(3)"). This is then appended to the shared ExpressionBuilder before the inner CollectionAssertion<Exception> is constructed. The inner CollectionAssertion also calls CreateExpressionBuilder with "InnerExceptions", creating a new independent StringBuilder. This means the outer expression string and the inner expression string live in separate builders and do not compose — the outer builder shows .WithInnerExceptions(exceptions => expressions.Count().IsEqualTo(3)) but the inner assertion's own expression shows Assert.That(InnerExceptions).Count().... This is unavoidable given the current architecture (it mirrors how Count(c => c.IsEqualTo(5)) works), but it may surprise users who read the assertion failure output. Worth at least being aware of as a future improvement.


Issue 5: Method duplication between ThrowsAssertion and ThrowsExactlyAssertion

Both ThrowsAssertion<TException> and ThrowsExactlyAssertion<TException> now have identical WithInnerExceptions implementations (same body, same signature). This mirrors the pattern for WithMessageContaining, WithMessage, etc. — but given that BaseThrowsAssertion<TException, TSelf> already exists precisely to hold common behaviour via the self-referencing CRTP pattern (TSelf), WithInnerExceptions is a strong candidate to be promoted to the base class. Currently BaseThrowsAssertion has no public fluent methods defined on it, but adding WithInnerExceptions there would eliminate the duplication and be consistent with the stated purpose of the class. If the reason this wasn't done is that other With* methods also live in the concrete classes, that is a pre-existing design issue worth noting for future consolidation.


Minor observation: null-delegate return path

var resultingAssertion = _innerExceptionsAssertion(collectionSource);

if (resultingAssertion != null)
{
    ...
}

return AssertionResult.Passed;

If the user provides a delegate that returns null (i.e. e => null), the assertion trivially passes regardless of the AggregateException content. This is consistent with how Count(c => null) behaves and is intentional per the existing pattern. It could be worth a short comment to make the intent explicit since it's counterintuitive.


Test coverage

The 6 tests are well-chosen and cover the key paths: success with Throws, success with ThrowsExactly, count failure, type mismatch failure, and non-AggregateException failure. One gap worth filling: a test for the case where the exception is an AggregateException but is accessed via a broader type generic parameter (e.g. .Throws<Exception>().WithInnerExceptions(...)), to confirm the is not AggregateException guard in CheckAsync behaves as expected in that polymorphic scenario.


Summary: The feature is correct and useful. The main actionable issues are (2) the missing metadata.Exception guard for evaluation errors and (3) the error-message double-wrapping. Issue (5) is a nice-to-have cleanup. The rest are observations for future polish.

@codacy-production
Copy link
Copy Markdown

codacy-production bot commented Apr 4, 2026

Not up to standards ⛔

🔴 Issues 1 minor

Alerts:
⚠ 1 issue (≤ 0 issues of at least minor severity)

Results:
1 new issue

Category Results
CodeStyle 1 minor

View in Codacy

🟢 Metrics 15 complexity

Metric Results
Complexity 15

View in Codacy

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

Copy link
Copy Markdown
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

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

Follow-up Review

No new commits have been pushed since my previous review. The previous review identified several actionable issues that remain unaddressed:

Still Outstanding

Issue 2 (Actionable) — Missing metadata.Exception guard in CheckAsync

The current implementation goes straight to metadata.Value without checking metadata.Exception first. All sibling assertions guard against evaluation-time errors before accessing the value. This is a consistency/correctness issue that should be fixed before merge.

Issue 3 (Actionable) — Bare catch swallows the failure message entirely

catch
{
    return AssertionResult.Failed("inner exceptions assertion failed");
}

The failure message drops all context from the inner assertion. At minimum this should be catch (Exception ex) and include ex.Message so users know why their inner exception assertion failed (e.g. "Expected count to equal 3 but received 2"). The bare catch also silently catches unexpected exceptions like NullReferenceException.

Issue 5 (Nice-to-have) — Identical method body duplicated across ThrowsAssertion and ThrowsExactlyAssertion

Both concrete classes have byte-for-byte identical WithInnerExceptions implementations. BaseThrowsAssertion exists specifically to hold shared behaviour — this method should be promoted there.


The feature itself is well-designed and the test coverage is solid. The two actionable items above (guard + catch) are quick fixes that would unblock merge.

Copy link
Copy Markdown
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

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

Code Review: Add WithInnerExceptions() for AggregateException chaining

Good feature addition with clean API design. The delegate-based pattern fits well with the existing Count(c => c.IsEqualTo(5)) style. However, there are a few issues to address — one of which is a correctness bug.


🐛 Critical: Assert.Multiple (AssertionScope) interaction

In WithInnerExceptionsAssertion.CheckAsync:

try
{
    await resultingAssertion.AssertAsync();
}
catch
{
    return AssertionResult.Failed("inner exceptions did not satisfy assertion");
}

When called inside Assert.Multiple, AssertAsync() does not throw on failure — it accumulates the failure in the current AssertionScope and returns normally. This means:

  • The catch block is never reached
  • CheckAsync returns AssertionResult.Passed
  • The inner assertion failure leaks into the outer scope silently
  • The outer WithInnerExceptions assertion is reported as passed even though the inner assertion failed

There's no simple catch-based fix here. The best approach is to capture the inner assertion scope in isolation — run the delegate assertions without an active scope, collect any failures, and use those to determine the result:

// Run inner assertions in an isolated scope to capture failures
List<AssertionException> capturedFailures;
using (var innerScope = new AssertionScope())
{
    await (resultingAssertion.AssertAsync());
    capturedFailures = innerScope.GetExceptions(); // hypothetical API
}

if (capturedFailures.Count > 0)
{
    return AssertionResult.Failed(capturedFailures[0].Message);
}

The exact API depends on what AssertionScope exposes — but the principle is to isolate the inner assertion's execution from any outer scope.


🐛 Information loss on failure

Even outside Assert.Multiple, the catch block discards the specific failure message:

catch
{
    return AssertionResult.Failed("inner exceptions did not satisfy assertion"); // original detail lost!
}

A user who writes .WithInnerExceptions(e => e.Count().IsEqualTo(5)) when there are only 2 inner exceptions will see:

❌ Expected: to have inner exceptions satisfying assertion
But: inner exceptions did not satisfy assertion

Instead of the informative:

❌ Expected: Count to be equal to 5
But: received 2

At minimum, the exception message should be re-used:

catch (Exception ex)
{
    return AssertionResult.Failed(ex.Message);
}

⚠️ Missing compile-time type safety

WithInnerExceptions is callable on any ThrowsAssertion\<TException\>, not just when TException : AggregateException. The runtime check is correct, but a compile-time guard is far better UX:

// Catches misuse at compile time with no runtime overhead
public static WithInnerExceptionsAssertion<TException> WithInnerExceptions<TException>(
    this ThrowsAssertion<TException> assertion,
    Func<CollectionAssertion<Exception>, Assertion<IEnumerable<Exception>>?> innerExceptionsAssertion,
    [CallerArgumentExpression(nameof(innerExceptionsAssertion))] string? expression = null)
    where TException : AggregateException  // ← compile error if TException isn't AggregateException

Extension methods can have narrower type constraints than the class they extend. Moving both WithInnerExceptions overloads to a static extensions class with where TException : AggregateException would eliminate the runtime null/type check in CheckAsync entirely. The test WithInnerExceptions_Fails_When_Not_AggregateException becomes redundant (which is the right outcome — the compiler prevents it).


♻️ Code duplication in ThrowsAssertion.cs

The WithInnerExceptions method body is identical in both ThrowsAssertion\<TException\> and ThrowsExactlyAssertion\<TException\>. Both extend BaseThrowsAssertion\<TException, TSelf\>, so the method belongs on the base class:

// In BaseThrowsAssertion<TException, TSelf>
public WithInnerExceptionsAssertion<TException> WithInnerExceptions(
    Func<CollectionAssertion<Exception>, Assertion<IEnumerable<Exception>>?> innerExceptionsAssertion,
    [CallerArgumentExpression(nameof(innerExceptionsAssertion))] string? expression = null)
{
    Context.ExpressionBuilder.Append($".WithInnerExceptions({expression})");
    return new WithInnerExceptionsAssertion<TException>(Context, innerExceptionsAssertion);
}

(Or alternatively, move to extension methods as described above, which solves both issues at once.)


📋 Minor: tests only assert that failures throw, not what they say

The failure tests use:

await Assert.That(sut).ThrowsException();

It would be more robust to also assert on the failure message to prevent regression on the information-loss issue:

await Assert.That(sut).ThrowsException()
    .WithMessage("*5*").Or.WithMessageContaining("expected");

Summary

Issue Severity
AssertionScope interaction — inner failures silently pass 🔴 Bug
Failure message discarded in catch block 🔴 Bug
No compile-time guard for non-AggregateException 🟡 Design
Method duplication in ThrowsAssertion.cs 🟡 Design
Failure tests don't verify message content 🟢 Minor

The feature direction is right and the API surface is clean — these issues are in the implementation details of WithInnerExceptionsAssertion.

Move WithInnerExceptions from instance methods on ThrowsAssertion<T> and
ThrowsExactlyAssertion<T> to extension methods constrained to
AggregateException. This prevents misuse at compile time rather than
runtime. Also simplify WithInnerExceptionsAssertion to a non-generic
class since TException is always AggregateException.
Copy link
Copy Markdown
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

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

Code Review

This PR adds a useful WithInnerExceptions() method for fluent AggregateException assertion chaining. The feature is well-scoped and the tests cover the core scenarios. I have several design and correctness concerns worth addressing before merge.


Critical: Error Message Swallows Actual Failure Details

TUnit.Assertions/Conditions/WithInnerExceptionsAssertion.cs, lines 43–50

try
{
    await resultingAssertion.AssertAsync();
}
catch
{
    return AssertionResult.Failed("inner exceptions did not satisfy assertion");
}

This silently discards the actual assertion failure message produced by the inner AssertAsync() call. When the collection assertion fails, it will have already generated a detailed, specific error message (e.g., "Expected count to be 5 but was 2"). By swallowing the exception and replacing it with a generic string, you lose that signal entirely.

Compare with how CollectionCountWithInlineAssertionAssertion handles this:

catch
{
    return AssertionResult.Failed($"count was {_actualCount}");
}

Even that class preserves the actual value. For inner exceptions, the failure message gives no information about which inner exception failed, what was expected, or what was found.

A better approach would be to propagate the original exception message:

catch (Exception ex)
{
    return AssertionResult.Failed(ex.Message);
}

Or, if the inner assertion already throws an AssertionException with a full formatted message, re-throw it directly rather than returning Failed(...). This is the approach that MappedSatisfiesAssertion uses — it calls AssertAsync() and lets the exception propagate, avoiding the try/catch pattern altogether.


Design: Duplicate Extension Method Overloads vs. Instance Methods

TUnit.Assertions/Extensions/AssertionExtensions.cs, lines 1166–1193

The two WithInnerExceptions overloads are nearly identical — one for ThrowsAssertion<AggregateException> and one for ThrowsExactlyAssertion<AggregateException>. This duplication will need to be maintained in parallel whenever the signature changes.

Looking at the existing pattern in ThrowsAssertion.cs, the other With* methods (WithInnerException<T>(), WithMessage(), WithMessageContaining(), etc.) are defined as instance methods on the assertion class itself, not as extension methods. Extension methods were only added for backward compatibility during refactoring. The new WithInnerExceptions follows neither the instance method pattern (which avoids the duplication problem) nor the backward-compatibility extension-method pattern — it's using extension methods as the primary API for a new feature.

If both ThrowsAssertion<AggregateException> and ThrowsExactlyAssertion<AggregateException> need this, the cleanest approach would be to add WithInnerExceptions() as an instance method to the shared base BaseThrowsAssertion<AggregateException, TSelf> (constrained via the where TException : AggregateException bound), or alternatively to add it directly to both concrete classes following the same style as WithInnerException<T>().


Design: WithInnerExceptionsAssertion Cannot Chain .And / .Or

TUnit.Assertions/Conditions/WithInnerExceptionsAssertion.cs

WithInnerExceptionsAssertion extends Assertion<AggregateException> directly, not CollectionAssertionBase or any source type. This means after WithInnerExceptions(...), the user cannot write .And. to chain further assertions about the AggregateException (e.g., its message). The base Assertion<TValue> does provide .And / .Or, but they return AndContinuation<AggregateException> / OrContinuation<AggregateException>, which don't expose collection-style methods.

The existing WithInnerException() method on ThrowsAssertion<TException> returns ThrowsAssertion<Exception>, making it fully chainable. This new assertion returns a dead-end. For a terminal assertion that's intentional, it should at minimum be documented as such, and consider whether returning ThrowsAssertion<AggregateException> (by continuing on the parent context) would be more consistent.


Design: Nullable Delegate Allows Silent No-ops

TUnit.Assertions/Conditions/WithInnerExceptionsAssertion.cs, line 13

private readonly Func<CollectionAssertion<Exception>, Assertion<IEnumerable<Exception>>?> _innerExceptionsAssertion;

The delegate return type is nullable (Assertion<IEnumerable<Exception>>?), and lines 41–51 explicitly handle a null return by doing nothing and returning Passed. This mirrors the CollectionCountWithInlineAssertionAssertion pattern, but there it makes sense — a user calling Count(c => c.IsEqualTo(5)) where the lambda might return null is an established pattern for item filtering.

For WithInnerExceptions, a null-returning delegate would mean calling WithInnerExceptions(_ => null), which silently asserts nothing. This is a footgun: the method exists solely to assert, so a null result should either be a compile-time error (change to non-nullable return) or at minimum throw an ArgumentException at construction time. The [CallerArgumentExpression] on expression suggests the intent is always to produce an assertion.


Test Coverage Gap: Non-AggregateException Path

The test WithInnerExceptions_Count_Fails and WithInnerExceptions_AllSatisfy_Fails_When_Mixed_Types both test inner assertion failure, but there's no test for the case where the action does not throw (the aggregateException == null path at line 33) or where a non-AggregateException is thrown (the evaluationException != null path at lines 26–28). Given that the method is only available on ThrowsAssertion<AggregateException>, the non-AggregateException case can't be reached via normal usage, but the null path can. Adding a test for the "action does not throw" failure case would make the safety net complete.


Minor: GetExpectation() Is Generic

protected override string GetExpectation() => "to have inner exceptions satisfying assertion";

This shows up verbatim in the failure message (Expected to have inner exceptions satisfying assertion). Compare with GetExpectation() on CollectionCountWithInlineAssertionAssertion:

protected override string GetExpectation() => "to satisfy count assertion";

Both are vague, but WithInnerExceptionsAssertion's is slightly odd phrasing. More importantly, since the actual assertion failure is swallowed (see the first point), this generic string is all the user sees. If the failure message propagation issue is fixed, this becomes less critical.


Minor: CollectionAssertion<Exception> Context Is a New Root

WithInnerExceptionsAssertion.cs, line 38

var collectionSource = new CollectionAssertion<Exception>(aggregateException.InnerExceptions, "InnerExceptions");

This creates a fresh CollectionAssertion with "InnerExceptions" as its expression root, so error messages from the inner assertion will say Assert.That(InnerExceptions)... rather than being contextualised within the parent expression. This is a disconnect in the diagnostic output — a user would see the outer assertion's expression separately from the inner collection assertion's expression. This isn't necessarily wrong (it's consistent with how WithInnerException() works by creating new contexts), but it's worth noting since it creates a disjointed error message experience.


Summary

The most impactful issue to fix before merge is the swallowing of failure messages — users will be unable to diagnose test failures without actionable details in the error output. The nullable delegate allowing silent no-ops is a secondary concern that could be addressed with a non-nullable return type constraint or a guard in the constructor.

Copy link
Copy Markdown
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

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

Follow-up Review

Looking at the current diff and the commit history, this PR has evolved well across four commits. Here is what I can see has been addressed and what remains.


What Has Been Addressed

Compile-time type safety — The move to extension methods constrained to ThrowsAssertion<AggregateException> and ThrowsExactlyAssertion<AggregateException> is the right call. It eliminates the runtime is not AggregateException check entirely and makes misuse a compiler error. This is the most impactful design improvement from earlier feedback.

metadata.Exception guard — The CheckAsync implementation now correctly checks metadata.Exception before accessing metadata.Value, consistent with sibling assertions.

WithInnerExceptionsAssertion simplified to non-generic — Since TException is always AggregateException after the compile-time constraint, this simplification is correct and clean.

.gitattributes improvements — The expansion is sensible and follows common .NET repo conventions.


Remaining Issue: Failure Message Still Discarded

The catch block in WithInnerExceptionsAssertion.CheckAsync still reads:

catch
{
    return AssertionResult.Failed("inner exceptions did not satisfy assertion");
}

This was raised in every previous review and was not fixed by the commit titled "Address PR review: add metadata.Exception guard and fix error message". The bare catch swallows the actual failure detail from the inner collection assertion. A user who writes:

.WithInnerExceptions(e => e.Count().IsEqualTo(5))

when there are only 2 inner exceptions will see:

Expected: to have inner exceptions satisfying assertion
But: inner exceptions did not satisfy assertion

instead of:

Expected: Count to be equal to 5
But: received 2

The fix is straightforward — at minimum propagate ex.Message:

catch (Exception ex)
{
    return AssertionResult.Failed(ex.Message);
}

Or, more aligned with how MappedSatisfiesAssertion works, let the exception from AssertAsync() propagate without catching it at all, since the calling infrastructure handles AssertionException correctly. The try/catch was only ever needed to convert from a thrown exception to an AssertionResult.Failed(...) return — but if the inner assertion already throws with a fully formatted message, letting it propagate is strictly better for the user.

Note: the bare catch (not catch (Exception)) also silently swallows unexpected exceptions like NullReferenceException or ObjectDisposedException, which would make test failures extremely difficult to diagnose.


Minor: Test Failure Cases Don't Assert on Message Content

The failure tests use:

await Assert.That(sut).ThrowsException();

This verifies that something throws, but gives no protection against the information-loss issue above regressing silently. Consider adding at least one message assertion to a failure test:

await Assert.That(sut).ThrowsException()
    .WithMessageContaining("5"); // or "IsEqualTo" / "count"

This would have caught the bare catch issue automatically.


Minor: Method Duplication Still Present

The two WithInnerExceptions extension method overloads in AssertionExtensions.cs have identical bodies — only the this parameter type differs (ThrowsAssertion<AggregateException> vs ThrowsExactlyAssertion<AggregateException>). This is intentional given that both concrete types need to be supported, and the alternative (moving to the base class) would require exposing the return type through the CRTP chain. Acceptable as-is, though a shared private helper extracting the body would reduce maintenance surface:

private static WithInnerExceptionsAssertion WithInnerExceptionsCore(
    AssertionContext<AggregateException> context,
    Func<CollectionAssertion<Exception>, Assertion<IEnumerable<Exception>>?> innerExceptionsAssertion,
    string? expression)
{
    context.ExpressionBuilder.Append($".WithInnerExceptions({expression})");
    return new WithInnerExceptionsAssertion(context, innerExceptionsAssertion);
}

Summary

Issue Status
Compile-time AggregateException constraint Fixed
metadata.Exception guard Fixed
Non-generic WithInnerExceptionsAssertion Fixed
Bare catch swallows failure details Still outstanding
Test failures don't verify message content Still outstanding (minor)
Method body duplication Acceptable as-is

The single blocker before merge is the bare catch losing all failure detail. Everything else is in good shape.

Copy link
Copy Markdown
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

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

Follow-up Review (Round 3)

Previous issues 2 and 5 have been addressed in commit 5e7ddb37. One issue remains unresolved, and there is a new observation about the .gitattributes change.


Still Outstanding: Bare catch still swallows the failure detail (Issue 3)

WithInnerExceptionsAssertion.CheckAsync currently reads:

catch
{
    return AssertionResult.Failed("inner exceptions did not satisfy assertion");
}

This is still a bare catch with no exception variable, so the user gets no information about why the inner assertion failed. The directly analogous MappedSatisfiesAssertion (added in this same codebase) does:

catch (Exception ex)
{
    return AssertionResult.Failed($"mapped value did not satisfy assertions: {ex.Message}");
}

This is the established pattern: catch Exception, capture it in ex, and include ex.Message in the failure string. Without this, a failure like "expected 3 inner exceptions but got 2" is reported as simply "inner exceptions did not satisfy assertion" — the user has to re-read the assertion code to know what went wrong. The fix is a one-line change:

catch (Exception ex)
{
    return AssertionResult.Failed($"inner exceptions did not satisfy assertion: {ex.Message}");
}

New Observation: .gitattributes change is unrelated to the feature

The PR includes a substantial expansion of .gitattributes (adding line-ending rules for .cs, .json, .sln, .ps1, binary files, etc.) that has no connection to the WithInnerExceptions feature. Mixing infrastructure/tooling changes with a feature commit makes the commit history harder to bisect and complicates reviewability. This change belongs in a separate PR so it can be evaluated on its own merits (e.g. verifying that adding working-tree-encoding=UTF-8 was intentionally dropped for the Verify snapshot files, and that the new line-ending rules won't create noise for contributors on Windows). The feature changes themselves are clean and ready.


Summary

The feature is well-designed, the metadata.Exception guard is now correct, and the type constraint via extension methods is a clean approach. The one remaining fix needed before merge is catching Exception ex in WithInnerExceptionsAssertion.CheckAsync and surfacing ex.Message in the failure string, consistent with MappedSatisfiesAssertion.

…h block

Change bare `catch` to `catch (Exception ex)` and include ex.Message
in the failure string, consistent with MappedSatisfiesAssertion and
other sibling assertion patterns in the codebase.
Copy link
Copy Markdown
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

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

Code Review: Add WithInnerExceptions() for fluent AggregateException assertion chaining

Overall this is a useful addition and the implementation is straightforward. A few architectural observations worth considering:

Inconsistency: instance methods vs. extension methods

The other fluent methods on ThrowsAssertion<TException> and ThrowsExactlyAssertion<TException> are instance methods on the class itself (e.g., WithInnerException(), WithMessage(), WithMessageContaining(), etc.). The new WithInnerExceptions() is implemented as a static extension method in AssertionExtensions.cs instead.

This creates a split API surface: some chaining methods live on the class and some on the extension class. Since the type is constrained to AggregateException (these overloads only target ThrowsAssertion<AggregateException> and ThrowsExactlyAssertion<TException>), they can't be a plain instance method on the generic class. A cleaner approach would be to follow the same pattern as WithInnerException() – which is an instance method – and either:

  1. Add a specialised assertion class (e.g., ThrowsAggregateAssertion) returned from Throws<AggregateException>() that has WithInnerExceptions() as an instance method, or
  2. Explicitly document why this method diverges from the instance-method pattern used by all other chaining methods.

The current extension method approach means WithInnerExceptions is not discoverable via IntelliSense after .Throws<AggregateException>(). in the same way as WithMessage or WithInnerException<T>.

Nullable delegate return type may silently swallow assertions

The delegate signature is:

Func<CollectionAssertion<Exception>, Assertion<IEnumerable<Exception>>?>

The nullable return allows the user to return null from the lambda, which is silently treated as a pass:

if (resultingAssertion != null)
{
    ...
}
return AssertionResult.Passed;

This is a footgun: a user who accidentally writes a void-style lambda or forgets to return a value will get a green test that checks nothing. The delegate should be non-nullable, or the null path should at minimum fail with AssertionResult.Failed("assertion delegate returned null").

Error message double-wrapping loses inner assertion context

When the collection assertion fails, the message is wrapped:

return AssertionResult.Failed($"inner exceptions did not satisfy assertion: {ex.Message}");

Since ex.Message already contains a fully-formed TUnit failure message, the output will look like:

inner exceptions did not satisfy assertion: Expected collection Count to equal 5 but was 2

Consider propagating ex.Message directly (i.e., return AssertionResult.Failed(ex.Message)) to avoid the redundant prefix, or restructuring to not use exception-catching at all and instead inspect AssertionResult directly if the collection assertion API exposes that.

Unreachable null-guard

The WithInnerExceptions extension methods are typed to ThrowsAssertion<AggregateException>, so by the time CheckAsync runs, the value is guaranteed to be an AggregateException (or metadata.Exception will be non-null). The null-guard if (aggregateException == null) is therefore unreachable dead code. Either remove it or add a comment explaining under what invariant it would ever trigger.

Minor: GetExpectation() should include the expression

protected override string GetExpectation() => "to have inner exceptions satisfying assertion";

This is always the same string regardless of what the delegate actually asserts. The expression captured via [CallerArgumentExpression] is already being appended to the ExpressionBuilder in the extension methods but is not passed into the assertion class. Storing it and using it in GetExpectation() would produce far more informative failure messages:

protected override string GetExpectation() => $"to have inner exceptions where {_expression}";

@thomhurst thomhurst disabled auto-merge April 4, 2026 19:42
@thomhurst thomhurst merged commit e4c164f into main Apr 4, 2026
13 of 15 checks passed
@thomhurst thomhurst deleted the feature/with-inner-exceptions branch April 4, 2026 19:42
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.

[Feature]: Expose AggregateException.InnerExceptions for fluent assertion chaining

1 participant