-
-
Notifications
You must be signed in to change notification settings - Fork 108
feat: add DisplayName, Skip, and Categories support for parameterized tests #4214
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
… tests Adds the ability to customize individual test cases with metadata: - `TestDataRow<T>` wrapper type for method/class data sources - `DisplayName`, `Skip`, and `Categories` properties on `[Arguments]` attribute - Parameter substitution in display names using `$paramName` or `$arg1` syntax This addresses #4212 by allowing users to set custom display names for data-driven tests without needing custom formatters. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
SummaryAdds TestDataRow wrapper and metadata properties (DisplayName, Skip, Categories) to [Arguments] attribute for customizing individual parameterized test cases. Critical IssuesBLOCKING: Dual-Mode Violation (CRITICAL RULE #1)Problem: This feature only works in reflection mode (TUnit.Engine) but NOT in source-gen mode (TUnit.Core.SourceGenerator). Evidence:
Impact: Users running with source generators will not get DisplayName, Skip, or Categories metadata from [Arguments] attributes or TestDataRow wrappers. Required Fix: Add source generator support in TUnit.Core.SourceGenerator to:
See .claude/docs/mandatory-rules.md - Rule #1 requires both source-gen AND reflection modes to work. AOT Compatibility ConcernsIssue: DataSourceMetadataExtractor.cs uses UnconditionalSuppressMessage with weak justification. Suggestion: Add [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties)] to ensure the trimmer preserves properties needed for reflection. SuggestionsPerformance: DisplayNameSubstitutor allocationsLocation: TUnit.Core/Helpers/DisplayNameSubstitutor.cs:37-53 Multiple string allocations in loop. Consider StringBuilder if this becomes a bottleneck, though current implementation is likely acceptable since it runs once per test during discovery. VerdictREQUEST CHANGES - Critical dual-mode violation must be fixed before merge. The feature is well-designed and code quality is good, but it fundamentally violates TUnit critical rule #1 by only supporting reflection mode. |
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 adds support for per-row metadata (DisplayName, Skip, Categories) to parameterized tests. It introduces a new TestDataRow<T> wrapper type for method/class data sources and adds corresponding properties to the [Arguments] attribute. Display names support parameter substitution using $paramName or $arg1 syntax.
Key Changes:
- New public API:
TestDataRow<T>record type for wrapping test data with metadata - ArgumentsAttribute extended with
DisplayName,Skip, andCategoriesproperties - Display name substitution engine with parameter and positional placeholder support
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
TUnit.Core/TestDataRow.cs |
New public wrapper type with ITestDataRow interface for AOT compatibility |
TUnit.Core/TestDataRowMetadata.cs |
Internal metadata record with merge logic for precedence handling |
TUnit.Core/Helpers/TestDataRowUnwrapper.cs |
AOT-compatible unwrapping logic using interface-based access |
TUnit.Core/Helpers/DisplayNameSubstitutor.cs |
Parameter substitution engine for $paramName and $arg1 placeholders |
TUnit.Core/Attributes/TestData/ArgumentsAttribute.cs |
Added DisplayName and Categories properties to both generic and non-generic versions |
TUnit.Core/TestContext.cs |
Added DataSourceDisplayName property and SetDataSourceDisplayName method |
TUnit.Core/TestContext.Metadata.cs |
Updated GetDisplayName to check DataSourceDisplayName and apply substitution |
TUnit.Core/TestDataCombination.cs |
Added Skip and Categories properties for data source metadata |
TUnit.Engine/Helpers/DataSourceMetadataExtractor.cs |
Reflection-based metadata extraction from IDataSourceAttribute implementations |
TUnit.Engine/Helpers/DataUnwrapper.cs |
Enhanced with metadata-aware unwrapping methods |
TUnit.Engine/Building/TestBuilder.cs |
Metadata application logic in BuildTestAsync for reflection mode |
docs/docs/test-authoring/test-data-row.md |
Comprehensive documentation with examples for all features |
docs/docs/test-authoring/arguments.md |
Updated with metadata property examples and usage guidance |
docs/sidebars.ts |
Added new documentation page to sidebar navigation |
| /// <summary> | ||
| /// Skip reason from the data source. When set, this test combination will be skipped. | ||
| /// </summary> | ||
| public string? Skip { get; init; } | ||
|
|
||
| /// <summary> | ||
| /// Categories from the data source to apply to this specific test combination. | ||
| /// </summary> | ||
| public string[]? Categories { get; init; } |
Copilot
AI
Jan 2, 2026
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.
The TestDataCombination class now has Skip and Categories properties, but there's no corresponding logic in the source generator to populate these properties when creating TestDataCombination instances from ArgumentsAttribute metadata.
For dual-mode parity, the source generator needs to extract these properties from ArgumentsAttribute when generating test combinations. The reflection mode handles this via DataSourceMetadataExtractor.ExtractFromAttribute and TestBuilder.BuildTestAsync (lines 874-897), but the source-generated code path needs equivalent logic.
| public string? DisplayName { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets categories to apply to this specific test case. | ||
| /// </summary> | ||
| /// <example> | ||
| /// <code> | ||
| /// [Arguments("value", Categories = ["smoke", "integration"])] | ||
| /// </code> | ||
| /// </example> | ||
| public string[]? Categories { get; set; } |
Copilot
AI
Jan 2, 2026
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.
The new DisplayName and Categories properties are defined but not applied to the test context. The ArgumentsAttribute implements ITestRegisteredEventReceiver with an OnTestRegistered method (lines 81-90) that only handles the Skip property.
For dual-mode parity, the OnTestRegistered method needs to be updated to also apply DisplayName and Categories properties, similar to how the reflection mode handles this in TestBuilder.cs lines 874-897. Without this, the source-generated mode won't respect these properties.
The OnTestRegistered method should:
- Call context.SetDataSourceDisplayName(DisplayName) when DisplayName is not null/empty
- Add Categories to context.TestContext.Metadata.TestDetails.Categories when Categories is not null/empty
| public string? DisplayName { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets categories to apply to this specific test case. | ||
| /// </summary> | ||
| public string[]? Categories { get; set; } |
Copilot
AI
Jan 2, 2026
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.
The new DisplayName and Categories properties are defined but not applied to the test context. The generic ArgumentsAttribute also implements ITestRegisteredEventReceiver with an OnTestRegistered method (lines 120-129) that only handles the Skip property.
For dual-mode parity, the OnTestRegistered method needs to be updated to also apply DisplayName and Categories properties, similar to how the reflection mode handles this in TestBuilder.cs lines 874-897. Without this, the source-generated mode won't respect these properties.
The OnTestRegistered method should:
- Call context.SetDataSourceDisplayName(DisplayName) when DisplayName is not null/empty
- Add Categories to context.TestContext.Metadata.TestDetails.Categories when Categories is not null/empty
| // Substitute by parameter name ($paramName) | ||
| for (var i = 0; i < parameters.Length && i < arguments.Length; i++) | ||
| { | ||
| var paramName = parameters[i].Name; | ||
| if (!string.IsNullOrEmpty(paramName)) | ||
| { | ||
| var placeholder = $"${paramName}"; | ||
| if (result.Contains(placeholder)) | ||
| { | ||
| var formatted = ArgumentFormatter.Format(arguments[i], effectiveFormatters); | ||
| result = result.Replace(placeholder, formatted); | ||
| } | ||
| } |
Copilot
AI
Jan 2, 2026
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.
The substitution logic checks for a dollar sign in the display name and performs replacements, but consider edge cases where parameter names or values themselves might contain dollar signs. While unlikely in typical scenarios, this could lead to unintended substitutions.
Consider using a more explicit placeholder syntax (e.g., {paramName} or {{paramName}}) or documenting this limitation if the current approach is intentional.
- Add DisplayName and Categories handling to ArgumentsAttribute.OnTestRegistered This fixes the dual-mode violation where the feature only worked in reflection mode, not source-gen mode - Add AOT DynamicDependency annotations to DataSourceMetadataExtractor for proper trimmer support of known TUnit data source types 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
SummaryThis PR adds DisplayName, Skip, and Categories support for parameterized tests via ArgumentsAttribute properties and a new TestDataRow wrapper for method/class data sources. Critical IssuesNone found ✅ The second commit successfully addresses the dual-mode requirement by implementing DisplayName and Categories handling in ArgumentsAttribute.OnTestRegistered (source-gen mode), and DataSourceMetadataExtractor properly handles reflection mode with appropriate AOT annotations. |
Suggestions1. Consider caching in DisplayNameSubstitutorLocation: TUnit.Core/Helpers/DisplayNameSubstitutor.cs:22-58 The Substitute method uses multiple string.Contains and string.Replace calls in a loop. For frequently-used display names, this could be optimized by using a compiled regex or Span based approach if this becomes a hot path. Impact: Minor - only affects display name generation, not test execution. Worth monitoring if users report slowdowns with many parameterized tests. |
2. Categories list duplication check could be more efficientLocation: TUnit.Core/Attributes/TestData/ArgumentsAttribute.cs:94-99 (and similar in ArgumentsAttribute) If Categories is a List, the Contains check is O(n). If many categories are added across multiple data sources, consider using a HashSet for Categories if not already, or checking the implementation of context.TestDetails.Categories. Impact: Low - unlikely to have many categories, but worth noting for consistency. |
Previous Review StatusThe second commit (436f382) addresses previous review feedback by:
Verdict✅ APPROVE - No critical issues. The implementation correctly follows TUnit's critical rules:
The suggestions above are optional optimizations, not blocking issues. |
Updates snapshots to include new Categories, DisplayName, Skip properties on ArgumentsAttribute and TestDataRow types. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
SummaryAdds Critical Issues❌ BLOCKING: Dual-Mode Rule Violation (#1) This PR implements the new metadata features (DisplayName, Skip, Categories) only in reflection mode (TUnit.Engine). The source generator mode (TUnit.Core.SourceGenerator) is missing the implementation. Evidence:The reflection mode correctly extracts these properties in:
However, the source generator does NOT preserve these properties:
Required Fix:Update
Why This Matters:Without this fix, tests written like: [Arguments(1, 2, DisplayName = "Custom name", Skip = "Not ready")]Will work correctly in reflection mode but will silently ignore DisplayName/Skip/Categories in source generator mode, creating inconsistent behavior. ✅ Snapshot Tests: Correctly updated .verified.txt files (no .received.txt committed) ✅ AOT Compatibility: Proper use of DynamicallyAccessedMembers and ITestDataRow interface to avoid reflection in hot paths ✅ Performance: TestDataRowUnwrapper uses interface-based access instead of reflection Verdict |
…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]>
…ments] attribute in NUnit migration (#4216) * fix: use inline DisplayName and Categories properties on [Arguments] 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]> * test: add comprehensive test coverage for inline property migration - 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]> --------- Co-authored-by: Claude Opus 4.5 <[email protected]>
Summary
TestDataRow<T>wrapper type for method/class data sources to specify per-row metadataDisplayName,Skip, andCategoriesproperties to[Arguments]attribute$paramNameand$arg1substitution in display namesIDataSourceAttributeimplementationCloses #4212
Features
TestDataRow Wrapper
Arguments Attribute Properties
New Files
TUnit.Core/TestDataRow.cs- Public wrapper typeTUnit.Core/TestDataRowMetadata.cs- Internal metadata recordTUnit.Core/Helpers/TestDataRowUnwrapper.cs- AOT-compatible unwrappingTUnit.Core/Helpers/DisplayNameSubstitutor.cs- Parameter substitutionTUnit.Engine/Helpers/DataSourceMetadataExtractor.cs- Reflection mode supportdocs/docs/test-authoring/test-data-row.md- DocumentationTest plan
[Arguments(DisplayName = "...")]shows custom name in test outputTestDataRow<T>with DisplayName works with MethodDataSource$paramNamesubstitution works correctly🤖 Generated with Claude Code