Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Aug 3, 2025

This PR extends the DisplayNameAttribute to support class-level usage, enabling custom display names for test classes with parameter substitution capabilities.

Changes Made

Enhanced AttributeTargets

  • Updated DisplayNameAttribute from AttributeTargets.Method to AttributeTargets.Method | AttributeTargets.Class
  • Allows the attribute to be applied to both test methods and test classes

Improved Parameter Substitution

  • Enhanced FormatDisplayName method to handle class constructor parameters
  • Method parameters are substituted first, then class parameters for any remaining placeholders
  • Maintains full backward compatibility with existing method-level usage

Updated Documentation

  • Comprehensive XML documentation with examples for both method and class usage
  • Clear explanation of parameter substitution behavior in different contexts

Usage Examples

Class-level DisplayName without parameters:

[DisplayName("Integration Tests for User Management")]
public class UserManagementTests
{
    [Test]
    public void TestUserCreation() { ... }
}

Class-level DisplayName with parameter substitution:

[Arguments("UserService")]
[DisplayName("Tests for $serviceName")]
public class ServiceTests(string serviceName)
{
    [Test]
    public void TestServiceFunctionality() { ... }
}

Method-level DisplayName (existing functionality preserved):

public class MyTests
{
    [Test]
    [Arguments("John", 25)]
    [DisplayName("User $name is $age years old")]
    public void TestUser(string name, int age) { ... }
}

Technical Details

  • Leverages existing TUnit infrastructure for parameter formatting and display
  • Works with both source generation and reflection execution modes
  • Integrates seamlessly with the test discovery and execution pipeline
  • Supports existing argument display formatters

Compatibility

  • ✅ Fully backward compatible with existing method-level usage
  • ✅ Works with both source generation and reflection modes
  • ✅ No breaking changes to existing APIs
  • ✅ Maintains existing parameter substitution behavior for methods

This enhancement addresses the common scenario where developers want to provide meaningful names for test class scenarios, particularly useful for integration testing where each class represents a specific test scenario.

Fixes #2689.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI changed the title [WIP] Allow for DisplayNameAttribute on class Allow DisplayNameAttribute on classes with parameter substitution support Aug 3, 2025
Copilot AI requested a review from thomhurst August 3, 2025 19:56
Copilot finished work on behalf of thomhurst August 3, 2025 19:56
@thomhurst thomhurst marked this pull request as ready for review August 3, 2025 21:40
@thomhurst thomhurst enabled auto-merge (squash) August 3, 2025 21:40
@claude
Copy link
Contributor

claude bot commented Aug 6, 2025

Claude finished @Copilot's task —— View job


PR Review Complete ✅

I've completed a thorough review of this PR that adds class-level support to DisplayNameAttribute. Here's my comprehensive feedback:

Todo List

  • Read repository guidelines (CLAUDE.md) - No file found
  • Review DisplayNameAttribute implementation changes
  • Review new test file for coverage adequacy
  • Check API verification file changes
  • Analyze backward compatibility claims
  • Provide comprehensive feedback

Code Quality Assessment

Excellent Implementation Quality

The implementation in TUnit.Core/Attributes/TestMetadata/DisplayNameAttribute.cs:38-78 is well-crafted:

  • AttributeTargets change: Correctly updated from AttributeTargets.Method to AttributeTargets.Method | AttributeTargets.Class
  • Parameter substitution logic: Smart priority-based approach - method parameters first, then class parameters for remaining placeholders
  • Null safety: Proper handling of edge cases with array length checks and Contains('$') validation
  • Performance: Efficient with early returns and minimal string operations

Robust Documentation

The XML documentation is comprehensive with:

  • Clear examples for both method and class usage
  • Detailed parameter substitution explanations
  • Proper <param> and <remarks> sections

Potential Issues Found

⚠️ Minor Performance Consideration

In DisplayNameAttribute.cs:61, the mutableDisplayName.Contains('$') check is a good optimization, but consider:

  • For very large display name templates, this could be called multiple times
  • Consider extracting to a variable if the string becomes large

⚠️ Edge Case: Empty Arrays

The code handles empty arrays correctly, but consider adding explicit documentation for behavior when:

  • Class has no constructor parameters
  • Method has no parameters
  • Both scenarios occur simultaneously

Test Coverage Analysis

Good Basic Coverage

The new test file TUnit.TestProject/ClassDisplayNameAttributeTests.cs covers:

  • Basic class display name without parameters
  • Class display name with parameter substitution using $value placeholder
  • Integration with existing test infrastructure (EngineTest, TestContext.Current.GetDisplayName())

⚠️ Missing Test Scenarios

Consider adding tests for:

  1. Mixed parameter substitution: Class with both method and class parameters
  2. Invalid placeholders: $nonExistentParameter handling
  3. Special characters in display names: Unicode, quotes, etc.
  4. Edge cases: Empty strings, null parameter handling
  5. Complex inheritance scenarios: Derived classes with base class parameters

Security & Performance

No Security Concerns

  • No dynamic code execution or reflection vulnerabilities
  • Safe string manipulation operations
  • No external data sources or user input handling

Performance Acceptable

  • String operations are minimal and efficient
  • Good use of early returns and condition checks
  • ArgumentFormatter.Format reuse is appropriate

API Changes Review

Backward Compatible Changes

The API verification files show the expected change:

[(.Class | .Method, Inherited=false)]
public sealed class DisplayNameAttribute : ...

This confirms:

  • Only the AttributeTargets was expanded (additive change)
  • No breaking changes to existing method signatures
  • Full backward compatibility maintained

Recommendations

🔧 Enhancements (Optional)

  1. Error Handling: Consider logging/warning for unresolved placeholders
  2. Performance: Cache processed display names if called frequently
  3. Validation: Add parameter name validation during attribute construction
  4. Testing: Expand test coverage for edge cases mentioned above

🔧 Documentation Enhancement

Consider adding a note about parameter resolution precedence in the main class documentation.

Final Assessment

This is a high-quality implementation that successfully extends DisplayNameAttribute functionality while maintaining full backward compatibility. The code is clean, well-documented, and follows established patterns in the codebase.

Recommendation: ✅ Approve - This PR is ready for merge with the minor suggestions above as potential future enhancements.

The implementation correctly addresses issue #2689 and provides the requested functionality without introducing breaking changes or security concerns.


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.

Allow for DisplayNameAttribute on class

2 participants