Skip to content

Reuse existing FormatCallSiteExpression helper for AreEqual/AreNotEqual#8316

Merged
Evangelink merged 1 commit into
mainfrom
dev/amauryleve/dedupe-binary-callsite-helper
May 18, 2026
Merged

Reuse existing FormatCallSiteExpression helper for AreEqual/AreNotEqual#8316
Evangelink merged 1 commit into
mainfrom
dev/amauryleve/dedupe-binary-callsite-helper

Conversation

@Evangelink
Copy link
Copy Markdown
Member

Follow-up to #8231 addressing #8231 (comment).

The new FormatBinaryCallSiteExpression helper (plus its LineBreakChars field) introduced in #8231 duplicated the existing 2-arg internal FormatCallSiteExpression overload and had subtly different semantics:

  • It dropped the entire call-site line when either expression was empty/whitespace, whereas the existing helper only drops it when both are empty and otherwise renders a <placeholder> for the empty side.
  • It carried its own LineBreakChars/IndexOfAny newline-detection in parallel with the existing IsMultiline/NormalizeCallSitePlaceholder infrastructure.

This PR:

  • Removes FormatBinaryCallSiteExpression and the LineBreakChars field from Assert.cs.
  • Routes Assert.AreEqual and Assert.AreNotEqual failure reporting through the existing FormatCallSiteExpression(string, string, string, string, string) overload, matching the call sites already used by Assert.AreSame, Assert.AreNotSame, Assert.Contains, Assert.AreEquivalent, Assert.StartsWith/EndsWith, Assert.MatchesRegex, etc.

Behavioral notes

For the typical case where both [CallerArgumentExpression] values are captured normally, the rendered call site is identical (e.g. Assert.AreEqual(expected, actual)).

The difference only shows up in the rare edge case where one side's caller-argument expression is empty (e.g. used through reflection / hand-built call) - the call site is now still rendered with <expected>/<actual> placeholders, consistent with the other binary assertions, instead of being suppressed.

Validation

  • .\build.cmd -c Release: succeeded.
  • dotnet test test\UnitTests\TestFramework.UnitTests\TestFramework.UnitTests.csproj -c Release --no-build: only the same 2 pre-existing AreAllDistinct_*WithComparer_HasDuplicate_ShouldFail failures observed on main remain (unrelated to this change).

Removes the duplicate FormatBinaryCallSiteExpression helper (and LineBreakChars field) added in #8231 and routes Assert.AreEqual / Assert.AreNotEqual through the existing 2-arg internal FormatCallSiteExpression overload. This aligns call-site formatting with Assert.AreSame / Assert.Contains / Assert.AreEquivalent etc.:

* When one of the two captured expressions is empty/whitespace, the call site is still rendered with a placeholder for the empty side (was: entire call site dropped).

* Multiline expressions are normalized through the shared NormalizeCallSitePlaceholder helper instead of a parallel LineBreakChars/IsMultiline implementation.

Addresses #8231 (comment).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 18, 2026 10:37
Copy link
Copy Markdown
Member Author

@Evangelink Evangelink 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 — PR #8316

21/21 dimensions clean — no findings.

# Dimension Verdict
1 Algorithmic Correctness ✅ LGTM
2 Threading & Concurrency ⏭️ N/A
3 Security & IPC ⏭️ N/A
4 Public API & Binary Compatibility ✅ LGTM
5 Performance & Allocations ✅ LGTM
6 Cross-TFM Compatibility ✅ LGTM
7 Resource & IDisposable Management ⏭️ N/A
8 Defensive Coding at Boundaries ✅ LGTM
9 Localization & Resources ⏭️ N/A
10 Test Isolation ⏭️ N/A
11 Assertion Quality ⏭️ N/A
12 Flakiness Patterns ⏭️ N/A
13 Test Completeness & Coverage ✅ LGTM
14 Data-Driven Test Coverage ⏭️ N/A
15 Code Structure & Simplification ✅ LGTM
16 Naming & Conventions ✅ LGTM
17 Documentation Accuracy ✅ LGTM
18 Analyzer & Code Fix Quality ⏭️ N/A
19 IPC Wire Compatibility ⏭️ N/A
20 Build Infrastructure & Dependencies ⏭️ N/A
21 Scope & PR Discipline ✅ LGTM

Summary

The change is a clean, well-reasoned deduplication:

  • Correctness: FormatCallSiteExpression(string,string,string,string,string) correctly handles the new semantics — both-empty → null; one-empty → placeholder; multiline → placeholder. Pre-bracketed literals like "<expected>" pass through NormalizeCallSitePlaceholder unchanged. ✅
  • Performance: Marginally better — IsMultiline uses two IndexOf(char) calls (zero-alloc) vs the old IndexOfAny(char[]) which allocated a char[] on pre-.NET 8 targets. ✅
  • Cross-TFM: IsMultiline already uses IndexOf(char) explicitly (not string.Contains(char)) for netstandard2.0/net462 compatibility. ✅
  • Test coverage: AssertTests.FormatCallSiteExpressionTests.cs covers the behavior-changing edge case (one-empty-one-non-empty → placeholder, not null). ✅
  • API compat: Both removed members were private static — no public surface affected. ✅

Generated by Expert Code Review (on open) for issue #8316 · ● 10.9M

@Evangelink Evangelink merged commit 089901b into main May 18, 2026
31 of 35 checks passed
@Evangelink Evangelink deleted the dev/amauryleve/dedupe-binary-callsite-helper branch May 18, 2026 13:04
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.

2 participants