Skip to content

Conversation

@thomhurst
Copy link
Owner

Fixes #4085

Copilot AI review requested due to automatic review settings December 22, 2025 13:23
@claude
Copy link
Contributor

claude bot commented Dec 22, 2025

Code Review

Thank you for this PR! The implementation looks solid and addresses issue #4085 well.

Strengths

  1. Clear Improvement: Moving the because message inline with the expectation significantly improves readability
  2. Backward Compatible: Does not break existing behavior, only improves message formatting
  3. Good Test Coverage: New test validates the exact message format with string literal comparison
  4. Consistent Logic: The becausePrefix handling properly avoids double because prefixes
  5. Proper Integration: Integrates well with AndAssertion and OrAssertion

Observations

  1. Code Quality: Follows modern C# style with proper string handling
  2. Minimal Changes: Focused refactoring - extracting expectation to a variable before composing
  3. Test Consistency: All existing tests remain valid, plus new test validates exact format

Potential Considerations

  1. Edge Case: The Because() method trims whitespace but does not validate for empty strings. If someone passes an empty string, you would get awkward formatting. Consider adding validation.

  2. Second Because Message: In AndAssertion and OrAssertion, only the first assertion's because message is included. This seems intentional per existing tests, but worth documenting.

  3. String Allocation: The change adds one string concatenation in the failure path. Since this only happens on assertion failures, performance impact is negligible.

Recommendations

  1. Consider adding a test for empty/whitespace because messages
  2. The existing test Apply_Because_Reasons_Only_On_Previous_Assertions already validates And chaining works

Pre-Commit Checklist

All items from CLAUDE.md checklist satisfied: tests added, modern C# style, no breaking changes, minimal focused change, no performance concerns, no dual-mode needed, no snapshot updates needed.

Final Verdict

LGTM with minor suggestion

The implementation is clean, well-tested, and solves the user problem effectively. The only enhancement would be validation for empty/whitespace because messages, but that is a minor quality improvement, not a blocker.

Great work on improving the developer experience!

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR enhances assertion exception messages by moving the "because" reason inline with the expectation statement, rather than appending it as a separate line. This change improves readability by keeping the reason directly adjacent to what's being asserted.

Key Changes:

  • Modified the exception message format to append the "because" reason inline with the expectation (using a comma separator)
  • Added a comprehensive test to verify the new inline format works correctly

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
TUnit.Assertions/Core/Assertion.cs Refactored CreateException method to build the expectation with inline "because" message before constructing the full error message
TUnit.Assertions.Tests/AssertConditions/BecauseTests.cs Added new test Because_Message_Appears_Inline_With_Expectation to verify the new format produces the expected inline message structure

The changes look good overall! The implementation is clean and consistent with the existing patterns in AndAssertion and OrAssertion classes which already format "because" messages inline. The test coverage is adequate, and the code follows proper style guidelines.

@thomhurst thomhurst merged commit 19c3e44 into main Dec 22, 2025
19 checks passed
@thomhurst thomhurst deleted the feature/4085 branch December 22, 2025 14:23
@claude claude bot mentioned this pull request Dec 22, 2025
1 task
This was referenced Dec 23, 2025
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.

[Feature]: Add reason string to end of error message

2 participants