feat(assertions): focused diff messages for IsEqualTo/IsEquivalentTo (#5732)

[thomhurst]

Closes #5732

Summary

• Failure message now points to the first differing member path with both expected/actual values
• Falls back to existing full-object dump only when no comparer-based diff is available

Test plan

• Added tests covering object-equality diff and collection-equivalence diff
• Existing assertion tests pass

[codacy-production]

Not up to standards ⛔

🔴 Issues 1 medium

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

Results:
1 new issue

Category	Results
Performance	1 medium

View in Codacy

_{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]

Code Review: feat(assertions): focused diff messages for IsEqualTo/IsEquivalentTo

Summary

This PR delivers genuine user value — failure messages for complex object comparisons become actionable instead of opaque, which is a meaningful DX improvement. The core design (StructuralDiffHelper as a standalone depth-first traversal returning DiffResult, gated strictly behind failure paths) is clean and follows established patterns in the codebase. The AOT/trimming graceful-degrade approach is correct.

---

Issues Found

Medium Severity

1. Missing cache in ReflectionHelper.GetMembersToCompare

Every call allocates a new List<MemberInfo> via GetProperties + GetFields. StructuralDiffHelper.FindFirstDifference calls this for every recursion level, and CountMatchingTopLevelMembers calls it again. By contrast, the existing DeepEquals in EqualsAssertion.cs already uses static ConcurrentDictionary<Type, PropertyInfo[]> caches. The PR description claims "allocation-light traversal" but this contradicts that.

Why it matters: This pre-dates the PR but the new code amplifies the issue — every failure now triggers potentially many uncached reflection calls. A simple ConcurrentDictionary<Type, IReadOnlyList<MemberInfo>> in ReflectionHelper.GetMembersToCompare would benefit all callers (StructuralEqualityComparer, StructuralEquivalencyAssertion, and the new StructuralDiffHelper).

2. Double structural traversal on ordered collection failure

In CollectionEquivalencyChecker.cs, for a failing ordered item: StructuralEqualityComparer.Equals() already performs a full structural traversal to determine inequality, and then DescribeItemDifference calls StructuralDiffHelper.FindFirstDifference() which traverses again. For large objects this doubles reflection work on failure paths.

A longer-term fix would be to make StructuralEqualityComparer return diff information alongside the bool, but the immediate concern is worth calling out.

---

Low Severity

3. DiffResult.Reason is stored but never used

The Reason property is populated in some DiffResult.Found() calls (e.g., "one value is null", "actual ended early", "actual has extra item") but FormatDiff ignores it entirely. The context about why items differ (vs. where) is silently dropped. This should either be wired into the formatted message or removed — as-is it's dead code that misleads future maintainers.

4. Cycle detection only tracks actual side

In StructuralDiffHelper.cs, only actual objects are added to visited. If expected contains a reference cycle (e.g., expected.Next = expected), the traversal of expectedValue could loop infinitely. The existing StructuralEquivalencyAssertion.CompareObjects tracks both visitedActual and visitedExpected. Cyclic test expectations are unlikely in practice, but worth hardening.

5. bestScore = 0 threshold allows spurious "closest match" hints

In CollectionEquivalencyChecker.cs, bestScore starts at -1. CountMatchingTopLevelMembers returns 0 when no top-level members compare equal. Any candidate with score=0 > bestScore=-1 will be selected as the best candidate — meaning two completely unrelated objects of the same type will still get a "closest match" hint even if they share zero member values. A minimum threshold (bestScore >= 1) would prevent confusing hints for highly dissimilar objects.

6. DescribeClosestDiff includes already-matched items

Both CheckUnorderedEquivalenceLinear and CheckUnorderedEquivalenceDictionary pass the full original actualList to DescribeClosestDiff. Items already matched to previous expected entries are included in the search, meaning the "closest match" could point to an already-correctly-matched item. For the Linear path, passing remainingActual instead of actualList would be more accurate.

---

Minor / Informational

7. Feature gap: IgnoringType + IsEqualTo does not get focused diff

When _ignoredTypes.Count > 0, EqualsAssertion.cs takes the DeepEquals path, bypassing TryFormatObjectDiff. The feature simply doesn't work when IgnoringType() is chained. Not a bug, but worth documenting or adding a test to prevent future confusion.

8. Test relies on reflection ordering stability

FocusedDiffMessageTests.cs asserts Contains("differs at member FirstName") for a record — relying on FirstName being the first property visited. GetProperties() order is technically unspecified by the runtime contract, though it's reliable for positional records in practice.

---

Recommendation

The most important fix before merging: Add caching in ReflectionHelper.GetMembersToCompare (issue #1) and either wire DiffResult.Reason into the output or remove it (issue #3). The bestScore threshold (issue #5) is a quick guard that prevents user-facing confusing output.

The overall design is sound and the test coverage is reasonable. These are refinements, not blockers — but #1 and #3 are worth addressing given the "allocation-light" framing in the PR description.

[claude]

Code Review: feat(assertions): focused diff messages for IsEqualTo/IsEquivalentTo (#5732)

Overview

This PR improves failure messages for IsEqualTo and IsEquivalentTo assertions by surfacing a focused "differs at member X: expected Y but found Z" hint instead of dumping the full serialized object. The core design is sound: StructuralDiffHelper is a standalone depth-first traversal returning a DiffResult value type, gated strictly behind the failure path. The AOT graceful-degrade approach ([UnconditionalSuppressMessage] with best-effort semantics) is appropriate.

This is an updated review — several issues raised in the prior review have been addressed. I'll call out what was fixed and what still needs attention.

---

Fixed Since Prior Review

1. ReflectionHelper.GetMembersToCompare caching — FIXED
The ConcurrentDictionary<Type, MemberInfo[]> cache has been added and the return type changed to MemberInfo[]. Both existing callers (StructuralEqualityComparer, StructuralEquivalencyAssertion) and the new StructuralDiffHelper now benefit from cached reflection.

2. DiffResult.Reason was dead code — FIXED
FormatDiff now includes the reason in the output via — {diff.Reason} (lines 187–189). The stored reasons ("one value is null", "actual ended early", "actual has extra item") are now surfaced to users.

3. Cycle detection only tracking actual — FIXED
FindFirstDifference now correctly tracks both visitedActual and visitedExpected, matching the pattern in StructuralEquivalencyAssertion.

4. bestScore allowing zero-similarity "closest match" — FIXED
The guard if (bestScore < 1) return string.Empty; prevents confusing closest-match hints for completely dissimilar objects.

5. DescribeClosestDiff including already-matched items in linear path — FIXED
CheckUnorderedEquivalenceLinear correctly passes remainingActual (not actualList) to DescribeClosestDiff.

---

Remaining Issues

Medium

6. DescribeClosestDiff in CheckUnorderedEquivalenceDictionary still passes the full actualList

In CheckUnorderedEquivalenceDictionary, DescribeClosestDiff(expectedItem, actualList) is called with the original unfiltered list. Unlike the linear path which maintains remainingActual, the dictionary path has no equivalent concept of "remaining" items — items are tracked only as frequency counts. This means the closest-match hint can point to an item that was already consumed by a previous expected entry.

The practical impact: in a collection like [A, A'] expected [A, B], when B is not found, the "closest match" search scans all of actualList including A (already consumed for the first expected item). If A happens to have a higher similarity score to B than A' does, the hint will mislead the user.

A pragmatic fix is to build a List<TItem> remainingForDiff that mirrors the frequency-map consumption:

// After building actualCounts, build remainingForDiff in parallel, then
// remove from it as items are matched (using the same comparer logic).

Alternatively, document this limitation as a known approximation, since the dictionary path is the O(n) fast path for the default comparer. The linear O(n²) path is already correct.

---

Low

7. Double structural traversal on ordered collection failure

In CheckOrderedEquivalence, comparer.Equals(actualItem, expectedItem) is called first. When comparer is StructuralEqualityComparer<TItem>.Instance (the default for IsEquivalentTo), this performs a full depth-first structural walk. When that returns false, DescribeItemDifference calls StructuralDiffHelper.FindFirstDifference which performs a second full walk on the same pair.

For large or deeply-nested objects, this doubles the reflection work on every mismatched item. A longer-term improvement would be to have StructuralEqualityComparer optionally return diff info, but even in the short term it's worth noting with a comment so future maintainers don't assume it's accidental.

---

8. DiffResult name collision with JsonDiffHelper.DiffResult

TUnit.Assertions.Conditions.Helpers.StructuralDiffHelper.DiffResult and TUnit.Assertions.Conditions.Json.JsonDiffHelper.DiffResult are both named DiffResult. They live in different namespaces so there's no compile error today, but any file importing both namespaces will require explicit qualification. Consider StructuralDiffResult or MemberDiffResult for the new type before more usages cement the name.

---

9. [DynamicallyAccessedMembers] annotation on GetMembersToCompare is bypassed by the cache delegate

The [DynamicallyAccessedMembers] annotation on the type parameter is a flow annotation telling the trimmer that PublicProperties and PublicFields are preserved for that type. However, BuildMembersToCompare receives the type as a raw Type without that annotation — the IL2070 suppression acknowledges this. For callers that use actual.GetType() at runtime (i.e., StructuralDiffHelper), the trimmer cannot statically prove members are preserved.

The [RequiresUnreferencedCode] on FindFirstDifference and the call-site suppressions handle this correctly — the entire feature degrades gracefully under AOT/trimming. Just be aware the annotation on GetMembersToCompare provides a false sense of safety for GetType() call sites; the suppression on BuildMembersToCompare is doing the actual work.

---

10. Test: IsEqualTo_OnRecord_Failure_Includes_Differing_Property relies on reflection ordering

The test asserts Contains("differs at member FirstName") for a positional record EmployeeInfo(string FirstName, string LastName, int Age). GetProperties() order is not contractually guaranteed by the runtime, though stable in practice for compiler-generated positional record types. A more robust assertion would check for the presence of any member path (Contains("differs at member")) rather than a specific one, or document the assumption explicitly.

---

Informational / Not Blocking

11. TryFormatObjectDiff is skipped when IgnoringType is active

When _ignoredTypes.Count > 0, EqualsAssertion.CheckAsync takes the DeepEquals path and never reaches TryFormatObjectDiff. The focused diff feature silently does not activate for .IgnoringType<T>().IsEqualTo(...) chains. The existing DeepEquals already produces per-member messages, so the regression is minimal, but a comment would prevent future confusion.

---

Summary

The PR is functionally correct and all prior medium-severity issues have been addressed — good progress. Two items worth fixing before merge:

• Issue #6 (Medium): DescribeClosestDiff in the dictionary unordered-equivalence path passes the full actualList rather than the unconsumed-items set, which can produce misleading "closest match" hints.
• Issue #8 (Low): DiffResult name collision is a low-risk maintainability concern worth resolving now before more usages appear.

Issues #7, #9, #10, and #11 are informational and do not block merging.

[claude]

Code Review: feat(assertions): focused diff messages for IsEqualTo/IsEquivalentTo (#5732)

This is a follow-up review after the prior two rounds. The code has improved significantly across iterations.

---

Fixed Since Prior Review

Issue #6 (Medium) — DescribeClosestDiff in dictionary path: FIXED
ExpandRemaining correctly materialises only the unconsumed items from the frequency map (items with count > 0) before calling DescribeClosestDiff, so the hint no longer points at items already matched by earlier expected entries.

Issue #8 (Low) — DiffResult naming collision: FIXED
The new type is now StructuralDiffResult, avoiding the ambiguity with JsonDiffHelper.DiffResult.

All issues from prior reviews are now addressed.

---

Remaining Observations

These are not blockers but worth knowing before merging:

1. DescribeClosestDiff traverses each candidate twice

In CollectionEquivalencyChecker.DescribeClosestDiff, per candidate the code calls:

1. StructuralDiffHelper.FindFirstDifference(candidate, expected) — full depth-first walk
2. StructuralDiffHelper.CountMatchingTopLevelMembers(candidate, expected) — another full walk

For N candidates with M members each, this is 2·N·M reflection reads on the failure path. Since both the diff path and the score path read the same top-level members, they can be merged into a single pass. One approach:

// Combine into one pass: count matches AND capture the first diff simultaneously.
// Returns (matchCount, firstDiff).
private static (int Score, StructuralDiffResult Diff) ScoreAndDiff(object candidate, object expected) { ... }

The impact is purely on failure diagnostics so it is not a hot-path concern, but for collections of large objects with many candidates it avoids doubling the reflection work.

2. ExpandRemaining capacity hint is pessimistic for deduplicated collections

var remaining = new List<TItem>(counts.Count + nullCount);

counts.Count is the number of unique keys, not the sum of their frequencies. For a collection like [A, A, A] where A still has count 3, the capacity is initialised to 1 rather than 3, causing List<T> to resize internally. The correct hint is:

var capacity = nullCount;
foreach (var pair in counts)
    capacity += pair.Value;
var remaining = new List<TItem>(capacity);

This is cosmetic (no correctness issue) but aligns with the allocation-efficiency framing elsewhere.

3. Test relies on GetProperties() ordering for records

IsEqualTo_OnRecord_Failure_Includes_Differing_Property asserts Contains("differs at member FirstName"), implying that FirstName will be the first property visited. For positional records the C# compiler emits properties in declaration order and GetProperties() returns them in that order in practice — but the runtime contract does not guarantee it. The test is stable today but fragile in principle. A weaker assertion like Contains("differs at member") would be more durable.

4. IgnoringType + IsEqualTo silently skips the focused diff

When .IgnoringType<T>() is chained, EqualsAssertion.CheckAsync takes the DeepEquals path and never reaches TryFormatObjectDiff. The feature simply does not activate in that combination. DeepEquals already emits per-member messages, so there is no regression, but a short comment at the branch would prevent future confusion.

---

Overall Assessment

The core design is correct and clean: StructuralDiffHelper is a well-bounded helper, gated strictly behind failure paths, with proper cycle detection on both sides, caching in ReflectionHelper, graceful AOT degradation via [UnconditionalSuppressMessage], and a meaningful score threshold on closest-match hints. The ExpandRemaining fix for the dictionary path closes the last medium-severity issue from prior reviews.

The four points above are non-blocking refinements. Approving with those noted.

[claude]

Review — iteration 4 (current HEAD)

Status: All prior issues resolved ✓

Every item flagged in the three previous rounds has been addressed in the current state. Summary of confirmed fixes:

Prior issue	Status
Missing ReflectionHelper member cache	Fixed — ConcurrentDictionary<Type, MemberInfo[]>
DiffResult.Reason dead code	Fixed — FormatDiff appends — {reason}
Cycle detection single-sided	Fixed — both visitedActual + visitedExpected tracked
bestScore = -1 zero-similarity loophole	Fixed — if (bestScore < 1) return string.Empty
DescribeClosestDiff linear path using full list	Fixed — passes remainingActual
DescribeClosestDiff dictionary path using consumed items	Fixed — ExpandRemaining materialises only unconsumed frequency-map items
DiffResult name collision with JsonDiffHelper	Fixed — renamed StructuralDiffResult
ExpandRemaining pessimistic capacity	Fixed — sums pair.Value (frequencies), not counts.Count (unique keys)
Test asserting specific FirstName member order	Fixed — test now asserts only differing values, not member name
IgnoringType bypass unexplained	Fixed — comment in EqualsAssertion.CheckAsync explains skip
Double traversal per candidate	Fixed — ScoreAndDiff single-pass walk

---

Remaining observations (non-blocking, pre-existing scope)

1. Dual reflection caching across the two comparison paths

EqualsAssertion owns two static caches (PropertyCache: ConcurrentDictionary<Type, PropertyInfo[]> and FieldCache: ConcurrentDictionary<Type, FieldInfo[]>) used by DeepEquals. ReflectionHelper._membersCache stores a MemberInfo[] that merges both. For any type exercised by both paths (e.g. one test uses IgnoringType, another doesn't), the same type's member info lives in both caches simultaneously.

This is pre-existing — DeepEquals predates the PR — so it's not a regression, but it's a natural consolidation opportunity. Pointing it out so the next refactor can collapse PropertyCache / FieldCache into ReflectionHelper.

2. Primitive detection divergence between the two paths

DeepEquals guards with type.IsValueType || type == typeof(string), while StructuralDiffHelper guards with TypeHelper.IsPrimitiveOrWellKnownType. Types like Guid, DateTimeOffset, Uri, or decimal are value types, so both paths treat them the same — but immutable reference types registered in TypeHelper (if any) would only be short-circuited by StructuralDiffHelper. Not a practical bug today, but it means the two paths have a semantic drift that could manifest as confusing if TypeHelper is extended.

3. Message format inconsistency

DeepEquals (activated by .IgnoringType()) emits "property X differs: Y vs Z" while StructuralDiffHelper emits "differs at member X: expected Y but found Z". A user toggling between .IsEqualTo(x) and .IsEqualTo(x).IgnoringType<T>() sees two different message shapes for conceptually the same failure. Not a blocker, but worth a follow-up issue to align the two formats.

---

Overall assessment

The core design is sound: best-effort, AOT-graceful-degrade, well-guarded against cycles, with a clean single-pass scoring path for closest-match hints. The feature adds meaningful value to failure messages without touching the success path. The three observations above are all pre-existing architectural debt that this PR exposed rather than introduced.

Approving. The observations above are good candidates for a follow-up cleanup issue.