-
-
Notifications
You must be signed in to change notification settings - Fork 108
fix: use inline DisplayName, Skip, and Categories properties on [Arguments] attribute in NUnit migration #4216
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…attribute - Update NUnitMigrationCodeFixProvider to map TestName → DisplayName inline on [Arguments] - Update NUnitMigrationCodeFixProvider to map Category → Categories array inline on [Arguments] - Update NUnitExpectedResultRewriter to use inline properties with correct argument ordering - Remove separate [DisplayName] and [Category] attribute generation from NUnitTestCasePropertyRewriter - Update tests to expect inline property format This aligns the migration code fixers with the new inline property support added in PR #4214 for ArgumentsAttribute. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add test for Ignore → Skip inline property conversion - Add test for IgnoreReason → Skip inline property conversion - Add test with all inline properties (DisplayName, Skip, Categories) - Add comprehensive test with all properties (inline + separate attributes) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
SummaryThis PR enhances the NUnit migration code fixer to handle TestName, Category, and Ignore properties inline on the [Arguments] attribute instead of generating separate attributes. Critical IssuesNone found ✅ SuggestionsConsider argument ordering edge case In NUnitExpectedResultRewriter.cs:442-525, the code ensures proper ordering: positional args → ExpectedResult → named properties. However, in NUnitMigrationCodeFixProvider.cs:84-136, there's no similar separation of positional vs named arguments before adding the Categories array at the end. While this likely works in practice (since Categories is added last and C# allows named arguments after positional ones), for consistency with the NUnitExpectedResultRewriter approach, consider explicitly separating positional and named arguments in ConvertTestCaseArguments as well. Test coverage is excellent The new test cases comprehensively cover:
This demonstrates good testing discipline. Verdict✅ APPROVE - No critical issues. The implementation correctly handles inline property conversion with comprehensive test coverage. The suggestion is a minor consistency improvement, not a blocking issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request enhances the NUnit to TUnit migration code fixers by utilizing the new inline properties on the [Arguments] attribute (added in #4214). Instead of generating separate attributes for DisplayName, Category, and Skip, these are now specified as inline named properties on each [Arguments] attribute, resulting in cleaner and more concise migrated code.
Key Changes:
- TestCase property
TestNameis now converted to inlineDisplayNameproperty on[Arguments] - TestCase property
Categoryis now converted to inlineCategoriesarray property on[Arguments] - TestCase properties
IgnoreandIgnoreReasonare now converted to inlineSkipproperty on[Arguments]
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| TUnit.Analyzers.Tests/NUnitMigrationAnalyzerTests.cs | Updated existing tests to reflect inline property conversion and added comprehensive new tests for Ignore→Skip, IgnoreReason→Skip, and all inline properties combined |
| TUnit.Analyzers.CodeFixers/NUnitTestCasePropertyRewriter.cs | Removed extraction and generation logic for TestName/Category properties, updated documentation to clarify these are now handled inline |
| TUnit.Analyzers.CodeFixers/NUnitMigrationCodeFixProvider.cs | Implemented inline conversion of TestName→DisplayName, Category→Categories array, and Ignore/IgnoreReason→Skip within the Arguments attribute |
| TUnit.Analyzers.CodeFixers/NUnitExpectedResultRewriter.cs | Implemented the same inline property conversions for TestCase attributes with ExpectedResult, maintaining proper argument ordering (positional → ExpectedResult → named properties) |
Summary
[Arguments]attribute (added in feat: add DisplayName, Skip, and Categories support for parameterized tests #4214)TestName→DisplayName(inline on[Arguments])Category→Categoriesarray (inline on[Arguments])Ignore/IgnoreReason→Skip(inline on[Arguments])Description→[Property("Description", "...")]Author→[Property("Author", "...")]Explicit→[Explicit]Example Migration
Before (NUnit):
After (TUnit):
Test plan
🤖 Generated with Claude Code