feat(analyzers): add NUnit ExpectedResult migration support

[thomhurst]

Summary

• Adds NUnitExpectedResultRewriter to automatically migrate NUnit's ExpectedResult pattern to TUnit's assertion-based approach
• Integrates the rewriter into the existing NUnit migration code fix pipeline
• Handles various scenarios including expression-bodied methods, block-bodied methods with single/multiple returns, and different return types

Changes

• New NUnitExpectedResultRewriter that transforms methods like:

  [TestCase(1, 2, ExpectedResult = 3)]
  public int Add(int a, int b) => a + b;

  Into:

  [Test]
  [Arguments(1, 2)]
  public async Task Add(int a, int b)
  {
      var result = a + b;
      await Assert.That(result).IsEqualTo(3);
  }

• Handles multiple TestCase attributes with different ExpectedResult values by generating parameterized expected values

• Supports string literals, numeric values, and other constant expressions

Test plan

• Added snapshot tests for various ExpectedResult migration scenarios
• Verified expression-bodied and block-bodied method handling
• Tested multiple TestCase attributes with ExpectedResult
• Tested string ExpectedResult migration

🤖 Generated with Claude Code

[claude]

Code Review: NUnit ExpectedResult Migration Support

Thank you for this comprehensive PR! The implementation is well-structured and thoroughly tested.

Strengths

1. Excellent Test Coverage - Comprehensive snapshot tests covering expression-bodied methods, block-bodied methods, single/multiple returns, and various data types

2. Clean Code Structure - Well-organized rewriter pattern following existing conventions with proper separation of concerns

3. Thoughtful Implementation - Handles complex scenarios like multiple return statements with if-else chains

Issues and Recommendations

1. Async Modifier Verification - Location: NUnitExpectedResultRewriter.cs:97-137. The code adds the async modifier after creating the method. Tests show expected output, but consider adding an explicit test validating the generated method signature includes async.

2. Complex Control Flow Testing - Location: NUnitExpectedResultRewriter.cs:228-249. The TransformMultipleReturns method handles complex if-else chains. Recommend adding more test cases for nested control flow patterns.

3. Performance: Node Traversal - Location: NUnitExpectedResultRewriter.cs:213-226. FindAllReturnStatements uses DescendantNodes() which traverses the entire tree. Consider using OfType for better performance.

4. Unused SemanticModel Field - Location: NUnitExpectedResultRewriter.cs:14. The _semanticModel field is unused but kept for consistency. Either use it for validation or remove it.

5. Edge Cases to Consider - Add tests for: generic return types with nullable, methods with no parameters but with ExpectedResult, invalid cases like async methods with ExpectedResult

6. Trivia Handling - Multiple places use WithoutTrivia() which could remove comments or mess up formatting. Consider preserving trivia.

7. Attribute Properties - Location: NUnitExpectedResultRewriter.cs:456. Comment says skip other named arguments for now - this loses TestCase properties like Description and Category. Document which properties are supported.

8. Performance Benchmark Files - The PR adds 100+ nearly identical test files totaling 11K+ lines. This increases repository size significantly and makes code review harder. Consider using source generators at build time or document the purpose in a README.

9. Hardcoded Newline - Line 428 uses hardcoded newline. Should use Environment.NewLine for cross-platform compatibility.

10. Method Complexity - TransformStatementsWithElseChain is 54 lines with nested loops. Consider extracting helper methods for readability.

Summary

This is a solid implementation. The core logic is sound, tests are comprehensive, and it integrates well with the existing codebase.

Main concerns: async/await verification, edge cases, and the massive benchmark file addition.

Recommendation: Approve with minor changes requested

Great work! The ExpectedResult migration will be very helpful for teams migrating from NUnit to TUnit.

[claude]

Pull Request Review

I've reviewed PR #4170 and have the following feedback:

✅ Strengths

1. Well-Structured Implementation: The NUnitExpectedResultRewriter follows the established codebase patterns and integrates cleanly into the existing code fix pipeline.

2. Comprehensive Test Coverage: The test cases in NUnitMigrationAnalyzerTests.cs cover important scenarios:

   • Expression-bodied methods
   • Block-bodied methods with single/multiple returns
   • String and numeric expected results
   • Multiple TestCase attributes

3. Proper Integration: The rewriter is correctly integrated into ApplyFrameworkSpecificConversions in the code fix provider, ensuring it runs before attribute conversion.

4. Modern C# Syntax: The code uses collection expressions, pattern matching, and other modern C# features as required by CLAUDE.md.

---

🔍 Issues & Recommendations

CRITICAL: Missing Return Type Conversion

Location: NUnitExpectedResultRewriter.cs:97

var asyncTaskType = SyntaxFactory.ParseTypeName("Task").WithTrailingTrivia(SyntaxFactory.Space);

Issue: The code doesn't preserve the original return type's namespace qualification. If the original method returns System.Threading.Tasks.Task<int>, this will just generate Task without proper using directives or qualification.

Impact: Compilation errors when using System.Threading.Tasks; is not present.

Recommendation:

// Use fully qualified name or ensure using directive
var asyncTaskType = SyntaxFactory.ParseTypeName("System.Threading.Tasks.Task")
    .WithTrailingTrivia(SyntaxFactory.Space);

---

MAJOR: Incomplete Method Body Transformation for Complex Cases

Location: NUnitExpectedResultRewriter.cs:228-249

Issue: The TransformMultipleReturns method doesn't handle cases where:

1. Early return statements are inside nested scopes (try/catch, using blocks, etc.)
2. Methods have both conditional returns and throw statements
3. Return values depend on out parameters or ref variables

Example that may break:

[TestCase(5, ExpectedResult = true)]
public bool TryParse(int value)
{
    try
    {
        if (value > 0) return true;
        throw new Exception();
    }
    catch
    {
        return false;
    }
}

The generated if-else chain may not preserve exception handling semantics.

Recommendation: Add test cases for these edge cases and handle nested scope transformations more carefully, or document limitations.

---

MODERATE: Semantic Model Usage

Location: NUnitExpectedResultRewriter.cs:13-18

// Kept for consistency with other rewriters and potential future semantic analysis needs
private readonly SemanticModel _semanticModel;

Issue: The semantic model is stored but never used. This is mentioned in the comment, but it adds unnecessary overhead.

Recommendation: Either remove it (breaking consistency) or use it for type resolution to fix the Task namespace issue mentioned above.

---

MODERATE: Missing Async Modifier for Already-Async Methods

Location: NUnitExpectedResultRewriter.cs:107-136

Issue: The code adds async modifier if not present, but what if the original method was already async int (which is invalid but might exist in migration scenarios)?

Recommendation: Add validation or a diagnostic for methods that are already async with non-Task return types.

---

MINOR: Hardcoded Attribute Names

Location: NUnitExpectedResultRewriter.cs:52

if (name is "TestCase" or "NUnit.Framework.TestCase" or "TestCaseAttribute" or "NUnit.Framework.TestCaseAttribute")

Issue: This list is duplicated from NUnitAttributeRewriter. Changes to attribute handling require updating multiple locations.

Recommendation: Extract attribute name constants to a shared location.

---

MINOR: Code Style - Braces

Location: NUnitExpectedResultRewriter.cs:108-115

foreach (var modifier in method.Modifiers)
{
    if (modifier.IsKind(SyntaxKind.AsyncKeyword))
    {
        hasAsyncModifier = true;
        break;
    }
}

Observation: This follows the required style guide (always use braces). Good adherence to CLAUDE.md requirements. ✅

---

QUESTION: Performance Benchmark Files

Observation: This PR adds ~300 nearly identical test files in TUnit.PerformanceBenchmarks/. These seem unrelated to the ExpectedResult migration feature.

Questions:

1. Are these files intentionally part of this PR, or were they accidentally included?
2. If intentional, what's the connection to ExpectedResult migration?
3. Should this be a separate PR focused on performance benchmarking infrastructure?

Recommendation: If unrelated, consider splitting into a separate PR to keep changes focused and reviewable.

---

🧪 Test Coverage Gaps

The test suite is comprehensive but missing:

1. Generic return types: [TestCase(ExpectedResult = default(T))]
2. Nullable return types: public int? GetValue()
3. Tuple return types: public (int, string) GetPair()
4. Methods with ref/out parameters combined with ExpectedResult
5. Nested return statements in try/catch/finally blocks
6. Methods with yield return (though ExpectedResult shouldn't apply here)

Recommendation: Add tests for at least scenarios 1-3 to ensure robustness.

---

🔐 Security Considerations

✅ No security concerns identified. The code:

• Operates on syntax trees only (no dynamic execution)
• Doesn't generate SQL, shell commands, or other injection vectors
• Doesn't handle user credentials or sensitive data

---

📊 Performance Considerations

Observation: The rewriter creates multiple passes over the syntax tree:

1. FindAllReturnStatements (line 213): O(n) traversal
2. TransformStatementsWithElseChain (line 251): O(n) traversal
3. Nested ContainsReturn checks (line 348): O(n²) in worst case

Impact: For large methods with many return statements, this could be slow. However, Roslyn code fixers are expected to be fast, and this is not a hot path.

Recommendation:

• Consider caching the return statement analysis
• Add performance tests if methods with >10 return statements are common in NUnit codebases

---

📝 Documentation & Style

✅ Good adherence to CLAUDE.md:

• Uses modern C# syntax (collection expressions, pattern matching)
• File-scoped namespaces (line 6)
• Follows naming conventions
• No LINQ in transformation logic (uses loops instead)

❌ Missing:

• No XML documentation comments on public methods
• The class-level summary comment (line 8-10) is good but methods lack documentation

Recommendation: Add XML docs to key public methods, especially TransformMethod, TransformBody, and TransformMultipleReturns.

---

✅ Pre-Commit Checklist Status

Based on CLAUDE.md requirements:

• ✅ Tests written and passing
• ❓ Dual-mode implementation: This is an analyzer/code fixer, so dual-mode doesn't apply
• ❓ Snapshot tests: Should TUnit.Analyzers.Tests have snapshot tests? I don't see any .verified.txt files for this change
• ✅ Code follows style guide
• ✅ No performance regressions (not a hot path)
• ✅ AOT/Trimming: Not applicable (Roslyn analyzers run at compile time)

Action Required: Confirm whether analyzer snapshot tests are needed per Rule 2 in CLAUDE.md.

---

🎯 Summary

Overall Assessment: This is a well-implemented feature that adds valuable NUnit migration support. The core logic is sound, but there are some edge cases and the massive addition of performance benchmark files needs clarification.

Blocking Issues:

1. ❌ Missing Task namespace qualification (may cause compilation errors)
2. ❓ Clarify intent of 300+ performance benchmark files

Non-Blocking Recommendations:

• Add test coverage for generic/nullable return types
• Extract shared attribute name constants
• Add XML documentation
• Handle complex nested return scenarios

Suggested Next Steps:

1. Fix the Task namespace issue
2. Clarify/separate the performance benchmark files
3. Add a few more edge case tests
4. Consider adding snapshot tests if required by the project

---

Great work overall! The ExpectedResult migration feature will be very useful for teams migrating from NUnit. 🚀