Skip to content

Conversation

@thomhurst
Copy link
Owner

Summary

This PR addresses several migration code fixer issues:

Changes

xUnit Record.Exception() Conversion (#4332)

Converts the xUnit pattern:

var ex = Record.Exception(() => ThrowException());

To:

Exception? ex = null;
try { ThrowException(); }
catch (Exception e) { ex = e; }

xUnit Exception Property Access (#4334)

Added tests verifying that Assert.Throws<T>() with property access works correctly:

  • Capturing exception and accessing .ParamName
  • Using Assert.Contains with ex.Message

NUnit [Platform] Conversion (#4339)

Instead of removing the NUnit [Platform] attribute, converts to TUnit equivalents:

  • [Platform(Include = "Win")][RunOn(OS.Windows)]
  • [Platform(Exclude = "Linux")][ExcludeOn(OS.Linux)]
  • [Platform(Include = "Win,Linux")][RunOn(OS.Windows | OS.Linux)]

Supported platform mappings:

NUnit TUnit
Win, Win32, Windows OS.Windows
Linux, Unix OS.Linux
MacOsX, MacOS, OSX, Mac OS.MacOs

Test plan

  • All new tests pass for Record.Exception conversion
  • All new tests pass for exception property access
  • All new tests pass for Platform attribute conversion
  • Existing migration tests continue to pass

🤖 Generated with Claude Code

thomhurst and others added 3 commits January 13, 2026 17:37
Adds support for converting xUnit's Record.Exception() and
Record.ExceptionAsync() patterns to try-catch blocks during migration.

The conversion transforms:
```csharp
var ex = Record.Exception(() => SomeMethod());
```
To:
```csharp
Exception? ex = null;
try
{
    SomeMethod();
}
catch (Exception e)
{
    ex = e;
}
```

Fixes #4332

Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add tests verifying that Assert.Throws<T>() with property access
works correctly:
- Assert_Throws_With_Property_Access_Can_Be_Converted: tests capturing
  exception and accessing properties like .ParamName
- Assert_Throws_With_Message_Contains_Can_Be_Converted: tests using
  Assert.Contains with ex.Message

Both tests pass, confirming issue #4334 is working correctly.

Co-Authored-By: Claude Opus 4.5 <[email protected]>
Instead of removing the NUnit [Platform] attribute, convert it to
TUnit's equivalent attributes:
- [Platform(Include = "Win")] -> [RunOn(OS.Windows)]
- [Platform(Exclude = "Linux")] -> [ExcludeOn(OS.Linux)]
- [Platform(Include = "Win,Linux")] -> [RunOn(OS.Windows | OS.Linux)]

Supports mapping NUnit platform strings to TUnit OS enum:
- Win, Win32, Windows -> OS.Windows
- Linux, Unix -> OS.Linux
- MacOsX, MacOS, OSX, Mac -> OS.MacOs

Unknown platforms are removed (cannot convert).

Co-Authored-By: Claude Opus 4.5 <[email protected]>
@thomhurst
Copy link
Owner Author

Summary

This PR adds NUnit Platform attribute conversion to RunOn/ExcludeOn and XUnit Record.Exception conversion to try-catch blocks for the migration code fixers.

Critical Issues

None found ✅

Suggestions

1. NUnit Platform Attribute - Positional Argument Bug

In NUnitMigrationCodeFixProvider.cs:ConvertPlatformAttribute (line 222-228), the positional argument handling has a logic issue:

if (argName == "Include" || argName == null)
{
    // Named argument Include= or positional argument (which is Include)
    includeValue = value;
}

Issue: If there are multiple positional arguments (e.g., [Platform("Win", "Linux")]), the code will overwrite includeValue for each one, only keeping the last value. NUnit's Platform attribute doesn't actually support multiple positional arguments in this way, but the code should be more defensive.

Suggestion: Consider only processing the first positional argument or documenting this behavior.

2. Record.Exception - Missing Block Body Handling

In XUnitMigrationCodeFixProvider.cs:ConvertLambdaBodyToStatement (line 257-265), when a lambda has a block body with multiple statements:

if (body is BlockSyntax block)
{
    // If it's a single statement block, extract the statement
    if (block.Statements.Count == 1)
    {
        return block.Statements[0];
    }
    // Otherwise return the block as-is (will need to be a compound statement)
    return block;
}

Issue: When returning the block directly for multi-statement lambdas, it's placed inside the try block as a nested block:

try
{
    {  // Extra braces here
        statement1;
        statement2;
    }
}

Suggestion: When actionBody is already a BlockSyntax with multiple statements in CreateTryCatchStatements, extract its statements instead of nesting it:

if (actionBody is BlockSyntax blockBody && blockBody.Statements.Count > 1)
{
    // Use block's statements directly
    tryBodyStatement = SyntaxFactory.Block(blockBody.Statements)...
}

3. Record.ExceptionAsync - Missing Async/Await Verification

In XUnitMigrationCodeFixProvider.cs:CreateTryCatchStatements (line 286-298), the async handling only checks the invocation name but doesn't verify if the action body actually returns a Task:

// If async, we may need to await the action
if (isAsync && tryBodyStatement is ExpressionStatementSyntax exprStmt)
{
    var awaitExpr = SyntaxFactory.AwaitExpression(...);

Issue: This blindly adds await for any Record.ExceptionAsync usage, even if the lambda body is synchronous (e.g., () => throw new Exception()). While XUnit's Record.ExceptionAsync does expect async delegates, users might have incorrect code that still compiles.

Suggestion: Consider using semantic analysis to check if the expression is actually awaitable, or document that this migration assumes correct XUnit usage.

4. Platform Attribute - Case Sensitivity

In NUnitMigrationCodeFixProvider.cs:MapNUnitPlatformToTUnitOS (line 274), the mapping uses ToLowerInvariant():

return nunitPlatform.ToLowerInvariant() switch
{
    "win" or "win32" or ... => "Windows",

Minor: NUnit's platform matching is case-insensitive, so this is correct. However, consider adding a comment explaining this matches NUnit's behavior, as it's not immediately obvious why we're lower-casing.

5. Test Coverage - Missing Edge Cases

The tests cover basic scenarios well, but consider adding tests for:

  • [Platform(Include = "UnknownPlatform")] - what happens with unknown platforms?
  • [Platform(Include = "Win", Exclude = "Linux")] - both Include and Exclude (currently only Include is used)
  • Record.Exception with method groups: Record.Exception(ThrowingMethod) instead of lambdas
  • Record.ExceptionAsync with async lambdas: async () => await SomeAsyncMethod()

Verdict

APPROVE - No critical issues

The implementation is solid and follows TUnit conventions well. The suggestions above are optional improvements for edge cases and code robustness, but not blocking issues.

@thomhurst thomhurst merged commit 77d7428 into main Jan 13, 2026
12 of 13 checks passed
@thomhurst thomhurst deleted the fix/xunit-record-exception branch January 13, 2026 18:22
This was referenced Jan 14, 2026
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.

2 participants