Skip to content

Conversation

@thomhurst
Copy link
Owner

Fixes #3104

@claude
Copy link
Contributor

claude bot commented Sep 21, 2025

Claude finished @thomhurst's task —— View job


Code Review Complete ✅

Reviewing PR #3165: "fix: add WithMessageNotContaining assertion for exception messages"

Review Checklist:

  • Read repository guidelines (CLAUDE.md)
  • Analyze changed files and implementation
  • Evaluate code quality and best practices
  • Check for potential bugs or issues
  • Assess performance considerations
  • Review security concerns
  • Evaluate test coverage
  • Provide comprehensive feedback

Summary

This PR adds a WithMessageNotContaining assertion method for exception messages, which is the logical inverse of the existing WithMessageContaining method. The implementation follows TUnit's established patterns and coding standards.

✅ Strengths

Code Quality & Best Practices:

  • Consistent API Design: The new methods follow the exact same pattern as existing WithMessageContaining methods in ThrowsException.cs:53-65, providing both ordinal and custom StringComparison overloads
  • Modern C# Usage: Uses primary constructors, file-scoped namespaces, and expression-bodied members as per TUnit standards
  • Proper Naming: Method and class names follow established conventions and are descriptive
  • Code Reuse: Leverages existing extension methods like PrependAOrAn(), ShowNewLines(), and TruncateWithEllipsis(100)

Implementation Consistency:

  • Mirror Pattern: ThrowsWithMessageNotContainingAssertCondition.cs perfectly mirrors ThrowsWithMessageContainingAssertCondition.cs with only the logical inverse (lines 25-26)
  • Error Messages: Expectation message follows the established pattern: "to throw {ExceptionType} which message does not contain "{expected}""
  • CallerArgumentExpression: Properly uses [CallerArgumentExpression] attributes for better debugging experience

Test Coverage:

  • Comprehensive Testing: Tests cover all scenarios in ThrowInDelegateValueAssertionTests.cs:42-103:
    • ✅ Positive case: message doesn't contain text (passes)
    • ✅ Negative case: message contains text (fails with proper error message)
    • ✅ Case sensitivity with StringComparison.OrdinalIgnoreCase
    • ✅ Case sensitivity with StringComparison.Ordinal
  • Error Message Validation: Test on lines 68-70 verifies the assertion failure message contains the expected text

🔍 Technical Analysis

Performance:

  • Zero Performance Impact: Uses the same string comparison logic as existing methods
  • Efficient Logic: Uses !actualException.Message.Contains() instead of complex string operations
  • Minimal Allocations: Reuses existing extension methods for formatting

Security:

  • No Security Concerns: Simple string comparison logic with no external dependencies
  • Input Validation: Properly handles null exceptions and null expected values

Potential Issues:

  • Null Safety: Lines 23-26 in ThrowsWithMessageNotContainingAssertCondition.cs properly handle null exceptions
  • API Consistency: Both overloads match the existing WithMessageContaining pattern exactly

Public API Changes:

  • Snapshot Updates: All three target framework API snapshots (.NET 4.7, .NET 8.0, .NET 9.0) have been properly updated
  • Non-Breaking: Only adds new methods without modifying existing APIs

📋 Repository Compliance

TUnit Guidelines Adherence:

  • Code Standards: Follows modern C# patterns, proper casing, and expression-bodied members
  • Performance Focus: No performance regressions introduced
  • Testing: Comprehensive test coverage for the new functionality
  • API Snapshots: Properly accepted snapshot changes as required by CLAUDE.md guidelines

🎯 Verdict

APPROVED - This is a well-implemented feature that:

  • Adds useful functionality (inverse of existing WithMessageContaining)
  • Follows all established patterns and conventions
  • Includes comprehensive test coverage
  • Has zero performance impact
  • Maintains API consistency

The implementation is clean, follows TUnit's high standards, and provides a logical complement to the existing assertion methods.


@thomhurst thomhurst merged commit 6ee8bdb into main Sep 21, 2025
10 checks passed
@thomhurst thomhurst deleted the bug/3104 branch September 21, 2025 19:04
This was referenced Sep 22, 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.

add WithMessageNotContaining() to ThrowsException<>()

3 participants