Migrate string AreEqual overloads to RFC 012 messages#8331
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR migrates the string-specific Assert.AreEqual / Assert.AreNotEqual failure paths in the MSTest framework to RFC 012 structured assertion messages, aligning string diff summary behavior with the generic Assert.AreEqual path and updating unit tests and localized resources accordingly.
Changes:
- Updated
Assert.AreEqual(string)/Assert.AreNotEqual(string)to emit RFC 012 structured messages (summary + evidence + call-site expression), including culture/ignore-case evidence when applicable. - Centralized string diff-summary formatting via shared helpers (and removed the older string preview helper logic).
- Refreshed string assertion unit tests and updated
FrameworkMessages.resxplus corresponding XLF resources.
Show a summary per file
| File | Description |
|---|---|
| test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.AreEqualTests.cs | Updates assertions to match new structured string messages; adds coverage for ignore-case+culture diff behavior and AreNotEqual structured output. |
| src/TestFramework/TestFramework/Assertions/Assert.AreEqual.String.cs | Implements structured message reporting for string AreEqual/AreNotEqual, including comparison-aware diff index computation and evidence formatting. |
| src/TestFramework/TestFramework/Assertions/Assert.AreEqual.InterpolatedStringHandlers.cs | Updates interpolated string handlers to route through the new structured-message failure paths for string overloads and to pass caller argument expressions. |
| src/TestFramework/TestFramework/Assertions/Assert.AreEqual.cs | Reuses the new diff-summary formatting for the generic AreEqual string path and removes the older formatted string-diff message implementation. |
| src/TestFramework/TestFramework/Resources/FrameworkMessages.resx | Updates string diff summary templates and adds new structured-message summary resources for string-specific paths. |
| src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.*.xlf | Regenerates localized resources for the updated/new messages. |
Copilot's findings
- Files reviewed: 18/18 changed files
- Comments generated: 1
Evangelink
left a comment
There was a problem hiding this comment.
Review Summary — PR #8331: Migrate string AreEqual overloads to RFC 012 messages
| # | Dimension | Verdict |
|---|---|---|
| 1 | Algorithmic Correctness | |
| 5 | Performance & Allocations | |
| 9 | Localization & Resources | ✅ LGTM |
| 11 | Assertion Quality | |
| 13 | Test Completeness & Coverage |
✅ 16/21 dimensions clean.
Findings
-
Performance —
ToString()in culture-awareFindFirstStringDifference—expected[i].ToString()allocates a heap string per character comparison; usestring.Compare(expected, i, actual, j, 1, ignoreCase, culture)instead (no allocations). -
Performance —
SubstringinTryAdvanceMatchingWindow— up to 8Substringallocations per mismatching position; replace with the index+count overload ofstring.Compare. -
Correctness — Surrogate pairs —
expected[i].ToString()on a lone surrogate (half of an emoji/supplementary char) produces an ill-formed string; the reported diff index can point into the middle of a surrogate pair rather than its start. Consider snapping the returned index back withchar.IsHighSurrogate. -
Correctness — Hardcoded
"1 location(s)"— BothAreEqualStringDiffLengthBothMsgandAreEqualStringDiffLengthDifferentMsgsay"differ at 1 location(s)"regardless of how many positions actually differ. The code only computes the first difference index; the count is always 1 or could be many. Consider removing the count claim or computing it, e.g."Strings have the same length ({0}). First difference at index {1}.". -
Test Completeness —
AreEqualStringSpecificWithNullExpectedUsesStructuredMessagecovers(null, "string", false)but not the symmetric("string", null, false)path. A nullactualthrough the string-specific overload is untested for the new RFC 012 format. -
Assertion Quality —
AreEqual_WithTurkishCultureAndDoesNotIgnoreCase_Throwsasserts only.Should().Throw<Exception>()with no message verification. For an RFC 012 migration PR this leaves the new structured output for the Turkish-culture path entirely unchecked.
Notes
- XLF files appear correctly auto-generated (states
new/needs-review-translation), not hand-edited ✅ PublicAPI.Unshipped.txtis not affected because the modified constructors pre-existed in both handler structs ✅- No threading, IPC, security, or cross-TFM concerns identified ✅
Generated by Expert Code Review (on open) for issue #8331 · ● 34.6M
Summary
Assert.AreEqual/Assert.AreNotEqualfailure paths to RFC 012 structured assertion messagesAssert.AreEqualpath and update string interpolated handlersValidation
dotnet build src\TestFramework\TestFramework\TestFramework.csproj -c Release --nologo -v:minimaldotnet msbuild test\UnitTests\TestFramework.UnitTests\TestFramework.UnitTests.csproj /restore /t:Build /p:Configuration=Release /p:TargetFramework=net9.0 /m:1 /v:m /nologoartifacts\bin\TestFramework.UnitTests\Release\net9.0\Microsoft.VisualStudio.TestPlatform.TestFramework.UnitTests.exe --no-progress --output Normal --treenode-filter "/*/*/AssertTests/*AreEqual*"artifacts\bin\TestFramework.UnitTests\Release\net9.0\Microsoft.VisualStudio.TestPlatform.TestFramework.UnitTests.exe --no-progress --output Normal --treenode-filter "/*/*/AssertTests/*AreNotEqual*"