-
-
Notifications
You must be signed in to change notification settings - Fork 108
feat: add line-based diff output for multiline string comparisons #4512
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
SummaryAdds line-based diff output for multiline string comparisons with context lines, unified diff-style formatting, and configurable limits. Critical IssuesNone found ✅ Suggestions1. Public API Snapshot Testing (Minor)Since dotnet test TUnit.PublicAPIIf snapshots changed, commit the 2. Performance Considerations (Optional)The TUnit.Assertions/Conditions/Helpers/LineDiffHelper.cs:132-134:
TUnit.Assertions/Conditions/Helpers/LineDiffHelper.cs:143-160:
These are not blocking since assertion failure paths don't need micro-optimization, but worth considering for exceptionally large strings. 3. String.Concat vs StringBuilder (Very Minor)TUnit.Assertions/Conditions/Helpers/LineDiffHelper.cs:330: return line.Substring(0, maxLength) + "…";This is fine as-is, but for consistency with TUnit's performance-first principle, could use: return string.Concat(line.AsSpan(0, maxLength), "…");(Though this only applies if targeting NET8_0_OR_GREATER, and the current code already has good NET8 optimizations elsewhere.) Previous Review StatusNo previous comments found. Verdict✅ APPROVE - No critical issues This is a nice enhancement that significantly improves the UX for multiline string assertion failures. The implementation is well-tested (23 LineDiffHelper tests + 10 StringDifference integration tests) and the automatic fallback to line-based diff is a good design choice. Just verify the public API snapshots pass. |
When comparing strings containing newlines, assertions now show a unified-diff-style output with line numbers and context instead of the character-based pointer approach. Closes #4511
eb6d049 to
57156c3
Compare
SummaryAdds line-based diff output for multiline string comparisons, automatically switching from character-pointer to unified-diff-style output when newlines are detected. Critical IssuesNone found ✅ Suggestions1. Test Count DiscrepancyTUnit.Assertions.Tests/Helpers/StringDifferenceTests.cs: The PR description mentions "23 LineDiffHelper unit tests" but I only count 6 new integration tests in this file:
These are integration tests through 2. Allocation Optimization (Optional)TUnit.Assertions/Conditions/Helpers/LineDiffHelper.cs:332: The return line.Substring(0, maxLength) + "…";For consistency with TUnit's existing code (see #if NET8_0_OR_GREATER
return string.Concat(line.AsSpan(0, maxLength), "…");
#else
return line.Substring(0, maxLength) + "…";
#endifThis is very minor since this code is only in the assertion failure path (not a hot path). Previous Review StatusThe previous review by thomhurst mentioned public API snapshot testing, but since Verdict✅ APPROVE - No critical issues The implementation follows TUnit principles: modern C# features, proper StringComparison handling, and good UX improvements. The automatic detection of multiline strings is well-designed. The integration tests adequately cover the feature's behavior through the public API. |
Summary
LineDiffHelperclass that generates line-based diffs for multiline string comparisonsStringEqualsAssertionto automatically use line diff when strings contain newlinesBefore:
After:
Features
Closes #4511
Test plan