Skip to content

Conversation

@thomhurst
Copy link
Owner

Summary

NUnit's [Test] attribute supports properties like Description and Author that TUnit's [Test] attribute does not support. This change:

  • Extends NUnitAttributeRewriter to strip named properties from [Test] and [Theory] attributes via ConvertTestArguments()
  • Extends NUnitTestCasePropertyRewriter to extract Description and Author from [Test] attributes and convert them to [Property] attributes
  • Adds tests verifying using directive removal for NUnit.Framework and subnamespaces

Test plan

  • All 500 NUnit migration tests pass
  • New tests verify [Test(Description="...")] is correctly converted

🤖 Generated with Claude Code

NUnit's [Test] attribute supports properties like Description and Author that
TUnit's [Test] attribute does not support. This change:

- Extends NUnitAttributeRewriter to strip named properties from [Test] and
  [Theory] attributes (ConvertTestArguments)
- Extends NUnitTestCasePropertyRewriter to extract Description/Author from
  [Test] attributes and convert them to [Property] attributes
- Adds tests verifying using directive removal for NUnit.Framework and
  subnamespaces

Fixes #4426

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

Summary

Fixes #4426 by extending NUnit migration to handle [Test(Description="...")] and other named properties on [Test] and [Theory] attributes.

Critical Issues

None found ✅

Suggestions

1. Consider handling [Theory] attributes in GetTestAttributes

The GetTestAttributes method in NUnitTestCasePropertyRewriter.cs:64-82 correctly identifies both Test and Theory attributes, which is good since ConvertTestArguments strips properties from both. However, I notice that NUnit's [Theory] attribute doesn't actually support Description or Author properties (only [Test] does). While this doesn't cause incorrect behavior (it just extracts nothing), you might want to document this or narrow the scope to only [Test] in ExtractTestProperties.

2. Minor: Duplicate property merging could be more efficient

In MergeProperties at TUnit.Analyzers.CodeFixers/NUnitTestCasePropertyRewriter.cs:210-228, the method adds all source properties to target HashSets. Since these are HashSets, duplicates are handled automatically. However, if the goal is to preserve only unique values across Test and TestCase attributes, consider documenting this behavior - it's correct but subtle.

3. Test coverage: Consider edge case with ExpectedResult

The comment at TUnit.Analyzers.CodeFixers/NUnitMigrationCodeFixProvider.cs:453 mentions "ExpectedResult is not applicable for [Test] without parameters" - this is correct. However, NUnit allows [Test(ExpectedResult = value)] with a return type. While your code silently ignores it (reasonable), consider adding a test case to verify this edge case is handled gracefully (i.e., the ExpectedResult is stripped without errors).

Verdict

APPROVE - No critical issues

The implementation correctly:

  • Strips named properties from [Test] and [Theory] attributes that TUnit doesn't support
  • Extracts Description and Author properties and converts them to [Property] attributes
  • Handles the dual-path conversion (both ConvertTestArguments stripping AND ExtractTestProperties extraction)
  • Includes comprehensive test coverage for the new functionality
  • Properly removes argument lists when all arguments are named properties

The code is well-structured and the test coverage verifies the core scenarios. Nice work! 🤖

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.

TUNU0001 code fixer: NUnit [Test(Description=...)] produces uncompilable code

2 participants