refactor: fix xUnit true/false assertion conversion logic when supplying optional message parameter

[thomhurst]

Fixes #3857

[claude]

Code Review ✓

Thank you for this fix! This PR correctly addresses issue #3857 by properly handling the optional message parameter in xUnit's Assert.True and Assert.False conversions.

✅ Positive Observations

1. Correct Fix Implementation
The fix correctly handles the two-argument case by:

• Extracting the condition as the first argument
• Converting the message parameter to TUnit's .Because() method
• Maintaining backward compatibility for single-argument cases

2. Excellent Test Coverage
The PR adds comprehensive test cases covering:

• True/False with message → .Because()
• True/False without message → no .Because()
• All other xUnit assertion conversions (great regression test suite!)

3. Clean Refactoring
Good use of helper methods (ConvertTrueAssertion, ConvertFalseAssertion) to eliminate code duplication.

4. Dual-Path Coverage
Both conversion paths are updated:

• Main switch expression (lines 161, 163)
• Simplified switch in All handler (lines 535-542)

📋 Minor Observations

1. Parameter Naming Inconsistency
In the new helper methods, the parameter is called condition (line 304, 324), but it's actually expected when passed from the caller (line 161, 163). This works but might be slightly confusing:

// Line 161: 'expected' is actually the condition, not expected value
"True" => ConvertTrueAssertion(argumentListArguments, expected, isInSatisfy, parameterName),

However, looking at the context, expected is actually assigned from argumentListArguments[0] (the first argument), so this is semantically correct for True/False assertions. No change needed.

2. Consistent Pattern Application
The fix correctly mirrors the pattern used elsewhere in the codebase (e.g., IsNotEqualTo with .Within() on line 213). Well done!

3. Code Style Compliance
✅ Modern C# syntax used
✅ Proper indentation and formatting
✅ Clear variable names
✅ Braces used consistently

🔍 Security & Performance

Security: ✅ No security concerns. String interpolation is used safely within Roslyn syntax parsing.

Performance: ✅ No performance concerns. The helper methods are simple and don't add allocations in hot paths.

✅ Pre-Commit Checklist Verification

Based on CLAUDE.md requirements:

• ✅ Code follows style guide (modern C# syntax, proper naming)
• ✅ Comprehensive test coverage added
• ✅ Analyzer code fix properly implemented
• ✅ No breaking changes
• ⚠️ Snapshot tests: This change modifies source generator output indirectly (analyzer fixes). You may need to verify if TUnit.PublicAPI tests need snapshot updates.

🎯 Recommendation

LGTM! This is a solid fix with excellent test coverage.

Action Items:

1. ✅ Ensure dotnet test passes (especially TUnit.Assertions.Analyzers.CodeFixers.Tests)
2. ⚠️ Check if any .received.txt files were generated and need to be accepted as .verified.txt

Great work fixing this bug! The refactoring to extract helper methods makes the code more maintainable.

[Copilot]

Pull request overview

This PR fixes the xUnit to TUnit assertion conversion for Assert.True and Assert.False when an optional message parameter is supplied. The fix extracts the inline True/False conversion logic into dedicated helper methods that properly handle the optional message by appending .Because(message) to the assertion chain.

Key Changes

• Extracted True/False assertion conversion logic into dedicated helper methods (ConvertTrueAssertion and ConvertFalseAssertion)
• Added support for converting the optional message parameter to .Because() calls
• Updated pattern matching in ConvertLambdaBodyToTUnitAssertions to handle True/False with messages
• Cleaned up unused dead code for non-existent xUnit methods (IsAssignableTo, IsNotAssignableTo, IsNotAssignableFrom)

Reviewed changes

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

File	Description
TUnit.Assertions.Analyzers.CodeFixers/XUnitAssertionCodeFixProvider.cs	Added ConvertTrueAssertion and ConvertFalseAssertion helper methods; updated True/False case handlers in switch expressions to use new helpers; added pattern matching cases for True/False with messages in lambda body conversion; removed dead code for non-existent xUnit methods
TUnit.Assertions.Analyzers.CodeFixers.Tests/XUnitAssertionCodeFixProviderTests.cs	Added comprehensive test coverage for True/False assertions with and without messages, plus additional tests for other assertion types (Null, NotNull, Same, NotSame, Contains, etc.)

[Copilot]

The parameter name condition could be more descriptive. Consider renaming it to conditionArg or conditionArgument to clearly indicate it's an ArgumentSyntax node, not the condition value itself. This improves code readability and consistency with the args parameter.

[Copilot]

The parameter name condition could be more descriptive. Consider renaming it to conditionArg or conditionArgument to clearly indicate it's an ArgumentSyntax node, not the condition value itself. This improves code readability and consistency with the args parameter.

[Copilot]

The ConvertTrueAssertion and ConvertFalseAssertion methods are nearly identical, differing only in the assertion method name (.IsTrue() vs .IsFalse()). Consider refactoring to a single method like ConvertBooleanAssertion(args, condition, isInSatisfy, parameterName, assertionMethod) where assertionMethod is either "IsTrue" or "IsFalse". This reduces code duplication and makes the code more maintainable.

[Copilot]

While the new tests cover Assert.True/Assert.False with messages in simple scenarios, there's no test coverage for these assertions with messages when used inside Assert.All lambda bodies (which uses the ConvertLambdaBodyToTUnitAssertions code path at lines 535-542). Consider adding a test like the existing Xunit_All_Converts_To_AssertMultiple_WithForeach test but with Xunit.Assert.True(condition, "message") inside the lambda to ensure both code paths work correctly.