fix: expand nested And/Or expectations in failure messages (#5573)#5577
fix: expand nested And/Or expectations in failure messages (#5573)#5577
Conversation
Chaining 3+ `.And` (or `.Or`) produced "Expected both conditions and
to be X" because the nested combinator's GetExpectation() returned a
placeholder ("both conditions" / "either condition") instead of
expanding to its own children.
Override GetExpectation() on AndAssertion/OrAssertion to return the
combined expansion, and indent continuation lines with a tab so chained
clauses are visually grouped under the leading "Expected".
There was a problem hiding this comment.
Code Review
The fix is clean, minimal, and solves the root cause correctly. Here's my analysis:
What the fix does
The previous GetExpectation() on AndAssertion/OrAssertion returned a dead-end placeholder string ("both conditions" / "either condition"). When a parent combinator called GetExpectation() on a nested one, it got that placeholder instead of the real sub-chain expansion. The fix delegates GetExpectation() to BuildCombinedExpectation(), which recursively expands the full chain by calling InternalGetExpectation() on its children — causing the correct flattening at any depth.
Strengths
- Minimal footprint: Two one-line changes in the core logic cover the fix entirely.
- Self-healing recursion: Because
BuildCombinedExpectation()delegates to_first.InternalGetExpectation(), which itself callsGetExpectation()on any nested combinator, an arbitrarily deep chain automatically flattens without any loop or explicit traversal. - Good test coverage: The new tests verify both 3-way and 4-way chains, and the
DoesNotContain("both conditions")assertion directly guards the regression.
Minor observation: O(N²) string work for deep chains
Each level of nesting calls BuildCombinedExpectation(), which re-expands all children via InternalGetExpectation(). For a chain of depth N, this is O(N²) string allocations. For realistic assertion chains (2–10 links) this is completely negligible — just worth knowing if someone ever chains 1000 assertions together and wonders why the failure message is slow to build.
Style note: tab vs. space for indentation
Using \t is a reasonable choice and produces clean visual grouping. One thing to watch: if downstream consumers parse or trim failure messages, a literal tab character can be surprising. Existing tests have already been updated to match the new format, which is the right approach.
Verdict
Overall this is a well-scoped, correct fix with good test coverage. No changes needed. Approving.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
TIP This summary will be updated as you push new changes. Give us feedback
Updated [TUnit](https://github.com/thomhurst/TUnit) from 1.33.0 to 1.35.2. <details> <summary>Release notes</summary> _Sourced from [TUnit's releases](https://github.com/thomhurst/TUnit/releases)._ ## 1.35.2 <!-- Release notes generated using configuration in .github/release.yml at v1.35.2 --> ## What's Changed ### Other Changes * fix: restore SourceLink and deterministic builds in published packages by @thomhurst in thomhurst/TUnit#5579 ### Dependencies * chore(deps): update tunit to 1.35.0 by @thomhurst in thomhurst/TUnit#5578 **Full Changelog**: thomhurst/TUnit@v1.35.0...v1.35.2 ## 1.35.0 <!-- Release notes generated using configuration in .github/release.yml at v1.35.0 --> ## What's Changed ### Other Changes * fix: support open generic transitive auto-mocks by @thomhurst in thomhurst/TUnit#5568 * refactor: separate test and lifecycle tracing by @thomhurst in thomhurst/TUnit#5572 * fix: expand nested And/Or expectations in failure messages (#5573) by @thomhurst in thomhurst/TUnit#5577 ### Dependencies * chore(deps): update tunit to 1.34.5 by @thomhurst in thomhurst/TUnit#5566 * chore(deps): bump follow-redirects from 1.15.11 to 1.16.0 in /docs by @dependabot[bot] in thomhurst/TUnit#5538 * chore(deps): update verify to 31.16.0 by @thomhurst in thomhurst/TUnit#5570 * chore(deps): update verify to 31.16.1 by @thomhurst in thomhurst/TUnit#5574 * chore(deps): update gittools/actions action to v4 by @thomhurst in thomhurst/TUnit#5575 **Full Changelog**: thomhurst/TUnit@v1.34.5...v1.35.0 ## 1.34.5 <!-- Release notes generated using configuration in .github/release.yml at v1.34.5 --> ## What's Changed ### Other Changes * fix: cap test output at 1M chars to prevent OOM by @thomhurst in thomhurst/TUnit#5561 * fix: handle explicit interface impl with different return types in mock generator by @thomhurst in thomhurst/TUnit#5564 * fix: include XML documentation files in NuGet packages by @thomhurst in thomhurst/TUnit#5565 ### Dependencies * chore(deps): update tunit to 1.34.0 by @thomhurst in thomhurst/TUnit#5562 **Full Changelog**: thomhurst/TUnit@v1.34.0...v1.34.5 ## 1.34.0 <!-- Release notes generated using configuration in .github/release.yml at v1.34.0 --> ## What's Changed ### Other Changes * refactor: move CorrelatedTUnitLogger to TUnit.Logging.Microsoft and auto-inject handlers by @thomhurst in thomhurst/TUnit#5532 * feat: add Dev Drive setup for Windows in CI workflow by @thomhurst in thomhurst/TUnit#5544 * fix: start session activity before discovery so discovery spans parent correctly by @thomhurst in thomhurst/TUnit#5534 * feat: cross-process test log correlation via OTLP receiver by @thomhurst in thomhurst/TUnit#5533 * refactor: use natural OTEL trace propagation instead of synthetic TraceIds by @thomhurst in thomhurst/TUnit#5557 * fix: route ITestOutput writes through synchronized ConcurrentStringWriter by @thomhurst in thomhurst/TUnit#5558 ### Dependencies * chore(deps): update tunit to 1.33.0 by @thomhurst in thomhurst/TUnit#5527 * chore(deps): update dependency dompurify to v3.4.0 by @thomhurst in thomhurst/TUnit#5537 * chore(deps): update dependency docusaurus-plugin-llms to ^0.3.1 by @thomhurst in thomhurst/TUnit#5541 * chore(deps): update dependency microsoft.sourcelink.github to 10.0.202 by @thomhurst in thomhurst/TUnit#5543 * chore(deps): update dependency microsoft.entityframeworkcore to 10.0.6 by @thomhurst in thomhurst/TUnit#5542 * chore(deps): update dependency microsoft.templateengine.authoring.templateverifier to 10.0.202 by @thomhurst in thomhurst/TUnit#5546 * chore(deps): update dependency microsoft.templateengine.authoring.cli to v10.0.202 by @thomhurst in thomhurst/TUnit#5545 * chore(deps): update dependency system.commandline to 2.0.6 by @thomhurst in thomhurst/TUnit#5547 * chore(deps): update microsoft.aspnetcore to 10.0.6 by @thomhurst in thomhurst/TUnit#5548 * chore(deps): update dependency nuget.protocol to 7.3.1 by @thomhurst in thomhurst/TUnit#5549 * chore(deps): update microsoft.extensions to 10.0.6 by @thomhurst in thomhurst/TUnit#5550 * chore(deps): update dependency dotnet-sdk to v10.0.202 by @thomhurst in thomhurst/TUnit#5551 * chore(deps): update opentelemetry by @thomhurst in thomhurst/TUnit#5552 * chore(deps): update microsoft.extensions to 10.5.0 by @thomhurst in thomhurst/TUnit#5554 **Full Changelog**: thomhurst/TUnit@v1.33.0...v1.34.0 Commits viewable in [compare view](thomhurst/TUnit@v1.33.0...v1.35.2). </details> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Summary
.And/.Orproduced confusingExpected both conditions and to be Xmessages because the nested combinator returned a placeholder (both conditions/either condition) when its enclosing combinator asked for its expectation.GetExpectation()onAndAssertion/OrAssertionto return the combined expansion, so nested chains flatten into a readable multi-line expectation.Expected.Before
After
Test plan
Three_Way_And_Chain_Error_Message_Expands_All_Expectationsreproduces the issue and now passes.Four_Way_And_Chain_Error_Message_Expands_All_Expectationscovers deeper nesting.BecauseTests,Throws.ExactlyTests, andAssertMultipleTestsupdated to match the new tab-indented format.TUnit.Assertions.Testssuite: 1974/1974 passing.