fix: multiple migration code fixer issues

[thomhurst]

Summary

Fixes multiple issues in the xUnit and NUnit migration code fixers:

• Fixes xUnit Migration: Assert.Same converted to non-existent IsSameReference method #4333 - xUnit Assert.Same now converts to IsSameReferenceAs (not IsSameReference)
• Fixes xUnit Migration: Assert.Throws converted to undefined 'awaitAssert.ThrowsAsync' #4331 - xUnit Assert.Throws now has proper await keyword spacing (was generating awaitAssert.ThrowsAsync)
• Fixes xUnit Migration: Code fixer does not add 'using System.Threading.Tasks;' directive #4330 - System.Threading.Tasks using is now added for async methods even when ShouldAddTUnitUsings() is false
• Fixes NUnit Migration: [Ignore] attribute not converted to [Skip] #4338 - NUnit [Ignore] attribute now converts to [Skip]
• Fixes NUnit Migration: [Theory] and [Description] attributes not converted #4343 - NUnit [Theory] and [Description] attributes now handled
• Fixes NUnit Migration: Methods with ref/out parameters incorrectly converted to async #4341 - Methods with ref/out/in parameters are no longer incorrectly converted to async

Test plan

• All xUnit migration tests pass (40 total, 39 succeeded, 1 skipped)
• All NUnit migration tests pass (65 total)
• All MSTest migration tests pass (23 total)

🤖 Generated with Claude Code

[thomhurst]

Summary

Fixes multiple migration code fixer issues including Assert.Same conversion, await keyword spacing, System.Threading.Tasks using, NUnit Ignore/Theory/Description attributes, and ref/out parameter handling.

Critical Issues

None found ✅

Suggestions

1. Missing Test Coverage for ref/out Parameter Fix

The fix in AsyncMethodSignatureRewriter.cs:25-31 prevents async conversion for methods with ref/out/in parameters (fixing #4341), but there's no test case verifying this behavior. Consider adding a test to prevent regression:

[Test]
public async Task Method_With_Ref_Parameter_Not_Converted_To_Async()
{
    await CodeFixer.VerifyCodeFixAsync(
        """
        {|#0:using NUnit.Framework;
        
        public class MyClass
        {
            private static void HandleRealized(object sender, ref bool realized)
            {
                Assert.AreSame(sender, sender);
                realized = true;
            }
        }|}
        """,
        Verifier.Diagnostic(Rules.NUnitMigration).WithLocation(0),
        """
        using TUnit.Core;
        using TUnit.Assertions;
        using static TUnit.Assertions.Assert;
        using TUnit.Assertions.Extensions;
        
        public class MyClass
        {
            // Method should remain non-async due to ref parameter
            private static void HandleRealized(object sender, ref bool realized)
            {
                Assert.That(sender).IsSameReferenceAs(sender).Wait();
                realized = true;
            }
        }
        """,
        ConfigureNUnitTest
    );
}

2. Consider Extracting AddSystemThreadingTasksUsing Logic

In MigrationHelpers.cs:180-209, the refactored code now has AddSystemThreadingTasksUsing as a separate method, but it's only called from two places. The logic for detecting async code is duplicated. Consider whether the detection logic could be shared or if the current separation is optimal.

3. Verify [Theory] and [Description] Handling

The PR adds Theory and Description to the NUnit framework attributes list (lines 25 and 33 in MigrationHelpers.cs), but:

• Theory → Test conversion seems correct
• Description → null\! (removal) - verify there's no TUnit equivalent or if it should map to something like [Property]

The PR description mentions these were fixed, but it would be good to see test coverage for these attribute conversions.

Verdict

✅ APPROVE - No critical issues

The changes correctly address the reported bugs. The code is well-structured and follows TUnit patterns. The main suggestion is adding test coverage for the ref/out parameter fix to prevent future regressions.

[thomhurst]

Summary

This PR fixes multiple migration code fixer issues including await keyword spacing, System.Threading.Tasks using directive addition, NUnit Ignore/Theory/Description attribute handling, and prevention of async conversion for methods with ref/out/in parameters.

Critical Issues

None found ✅

All changes are analyzer/code fixer improvements that don't touch core engine code, so TUnit's critical rules (dual-mode, AOT, VSTest, performance) don't apply here.

Suggestions

1. Missing Test Coverage for ref/out/in Parameter Fix (Important)

The fix in AsyncMethodSignatureRewriter.cs:25-31 prevents async conversion for methods with ref/out/in parameters (fixing #4341), but there's no test coverage for this critical fix. This is a regression risk since it prevents compilation errors.

Recommendation: Add a test case to verify methods with ref/out/in parameters are not converted to async:

[Test]
public async Task NUnit_Method_With_Ref_Parameter_Not_Converted_To_Async()
{
    await CodeFixer.VerifyCodeFixAsync(
        """
        using NUnit.Framework;
        
        {|#0:public class MyClass|}
        {
            private static void HandleRealized(object sender, ref bool realized)
            {
                Assert.AreSame(sender, sender);
                realized = true;
            }
        }
        """,
        Verifier.Diagnostic(Rules.NUnitMigration).WithLocation(0),
        """
        using TUnit.Core;
        using TUnit.Assertions;
        using static TUnit.Assertions.Assert;
        using TUnit.Assertions.Extensions;
        
        public class MyClass
        {
            // Should remain non-async due to ref parameter
            private static void HandleRealized(object sender, ref bool realized)
            {
                Assert.That(sender).IsSameReferenceAs(sender).Wait();
                realized = true;
            }
        }
        """,
        ConfigureNUnitTest
    );
}

2. Missing Test Coverage for await Keyword Spacing Fix

The fix in XUnitMigrationCodeFixProvider.cs (lines 774-776, 821-823, etc.) adds proper spacing to await keywords, but there's no specific test validating the generated code has correct await spacing (not awaitAssert.ThrowsAsync).

Recommendation: Consider adding a test that explicitly validates the spacing in generated code for Assert.Throws conversions.

3. [Description] Attribute Handling

The PR maps NUnit's [Description] → null\! (removal) with the comment "no direct TUnit equivalent" in MigrationHelpers.cs:33.

While removal is a valid choice, verify whether:

• Users should be warned about lost metadata (Description contains useful documentation)
• It should map to [Property("Description", "...")] instead (preserving metadata)

The test coverage shows Description being converted to [Property] in some tests (e.g., line 1230 in NUnitMigrationAnalyzerTests.cs), which seems to contradict the null\! mapping. This may need clarification.

4. Code Quality Note

In MigrationHelpers.cs:202, the new AddSystemThreadingTasksUsing method duplicates async detection logic that already exists in AddTUnitUsings. The refactoring is clean, but consider whether early returns are optimal vs. the current structure.

Previous Review Status

Previous review by thomhurst noted similar concerns about test coverage for the ref/out parameter fix. This remains unaddressed.

Verdict

⚠️ REQUEST CHANGES - Missing test coverage for critical bug fix (#4341)

While the code changes are correct and fix real issues, the lack of test coverage for the ref/out/in parameter fix is concerning because:

1. It fixes a compilation error (CS1988) - a critical issue
2. Without tests, future refactoring could reintroduce the bug
3. The fix is non-obvious enough that it warrants explicit test coverage

The other fixes (await spacing, System.Threading.Tasks using, NUnit attributes) are well-covered by updated snapshot tests.

[thomhurst]

Summary

Fixes multiple migration code fixer issues including reference assertion naming, await keyword spacing, using directive handling for async methods, and handling of methods with ref/out/in parameters.

Critical Issues

None found ✅

Suggestions

Pre-existing bug in MSTest migrator (not blocking this PR)

The MSTest migrator uses incorrect assertion names IsSameReference and IsNotSameReference (without "As") at lines 250, 252, 254, and 256 of MSTestMigrationCodeFixProvider.cs. The correct TUnit assertion names are IsSameReferenceAs and IsNotSameReferenceAs (as used in xUnit and NUnit migrators). This is a pre-existing issue outside the scope of this PR, but should be addressed in a follow-up.

Reference: TUnit.Assertions/Conditions/ReferenceEqualityAssertions.cs:10 shows the correct attribute is [AssertionExtension("IsSameReferenceAs")].

Verdict

✅ APPROVE - No critical issues

This PR successfully addresses all the issues mentioned:

• ✅ xUnit Assert.Same correctly converts to IsSameReferenceAs
• ✅ xUnit Assert.Throws now has proper await keyword spacing
• ✅ System.Threading.Tasks using is added for async methods even when TUnit usings are not added
• ✅ NUnit [Ignore] converts to [Skip]
• ✅ NUnit [Theory] and [Description] attributes are handled
• ✅ Methods with ref/out/in parameters use .Wait() instead of await (cannot be async)

The implementation is well-tested with comprehensive test coverage. The code is clean and follows TUnit patterns. No TUnit rule violations detected.

[thomhurst]

Summary

Fixes multiple issues in xUnit and NUnit migration code fixers, including correct assertion conversions, async keyword spacing, proper handling of ref/out/in parameters, and System.Threading.Tasks using directive management.

Critical Issues

None found ✅

Suggestions

1. Duplicate await keyword creation in XUnitMigrationCodeFixProvider.cs

The XUnit migration code fixes duplicate the await keyword creation logic in multiple places instead of using the inherited WrapAssertionForAsync method. This inconsistency could lead to maintenance issues.

Locations:

• ConvertThrowsAny (XUnitMigrationCodeFixProvider.cs:911-913)
• ConvertIsNotType (XUnitMigrationCodeFixProvider.cs:958-960)
• ConvertThrows (XUnitMigrationCodeFixProvider.cs:992-994)
• ConvertThrowsAsync (XUnitMigrationCodeFixProvider.cs:1027-1029)
• ConvertIsType (XUnitMigrationCodeFixProvider.cs:1068-1070)
• ConvertIsAssignableFrom (XUnitMigrationCodeFixProvider.cs:1109-1111)

Why this matters: XUnit tests won't have ref/out/in parameters in practice (xUnit doesn't support them), so this works. However, the inconsistency with the NUnit code (which properly uses WrapAssertionForAsync) creates technical debt.

Suggestion: Consider using WrapAssertionForAsync consistently across both providers for better maintainability, even though the current implementation is functionally correct for xUnit tests.

2. Inconsistent variable naming

In ConvertThrowsAsync, the await keyword variable is named awaitKeyword2 (line 1027):

var awaitKeyword2 = SyntaxFactory.Token(SyntaxKind.AwaitKeyword)

This appears to be a copy-paste artifact and could simply be awaitKeyword.

Verdict

✅ APPROVE - No critical issues. The PR correctly fixes all the reported bugs. The suggestions are minor maintainability improvements that don't affect correctness.

[thomhurst]

Summary

Fixes multiple migration code fixer issues: Assert.Same conversion, await keyword spacing, System.Threading.Tasks using directive handling, NUnit attribute mappings, and proper handling of methods with ref/out/in parameters.

Critical Issues

None found - No TUnit rules apply (analyzer-only changes)

Suggestions

1. Missing test coverage for NUnit Theory and Description attributes (issue 4343)
2. Code inconsistency: XUnit migrator manually creates await expressions while NUnit migrator uses inherited WrapAssertionForAsync method
3. Minor: awaitKeyword2 variable naming in XUnitMigrationCodeFixProvider.cs:1027

Verdict

APPROVE - All reported bugs fixed with good test coverage for most changes

[thomhurst]

Summary

Fixes multiple migration code fixer issues including handling of ref/out/in parameters, Theory attribute, and await keyword spacing.

Critical Issues

None found ✅

Suggestions

Pragmatic Exception to "NEVER block on async" Rule

The PR introduces .Wait() calls for methods with ref/out/in parameters, which appears to conflict with TUnit's "NEVER block on async" rule in CLAUDE.md:42. However, this is a pragmatic exception to a hard C# language constraint:

• C# does not allow async methods with ref/out/in parameters (CS1988 compiler error)
• The alternatives are: (1) use .Wait(), (2) leave code unchanged for manual migration, or (3) refactor to eliminate ref/out/in parameters
• The PR chooses option 1, which was explicitly suggested in issue NUnit Migration: Methods with ref/out parameters incorrectly converted to async #4341

Recommendation: Consider documenting this exception in CLAUDE.md or mandatory-rules.md to clarify when blocking on async is acceptable:

**NEVER block on async** - No `.Result` or `.GetAwaiter().GetResult()`.
Exception: Code generation/migration tools may use `.Wait()` when targeting methods that cannot be async (e.g., methods with ref/out/in parameters).

This would prevent future confusion and make the trade-off explicit.

Test Coverage

The PR includes comprehensive test coverage for the ref/out parameter scenarios. Well done on the thorough testing approach!

Verdict

✅ APPROVE - No critical issues

The .Wait() usage is a legitimate workaround for a C# language constraint, not a violation of best practices. The implementation is clean, well-tested, and addresses multiple reported issues.