feat: add DisplayName property for dynamic tests

[thomhurst]

No description provided.

[claude]

PR Review: Add DisplayName Property for Dynamic Tests

Summary

This PR adds support for custom display names for dynamic tests, allowing users to specify a DisplayName property on DynamicTest<T> objects. The implementation unifies the metadata handling across AOT/source-generated and reflection modes by consolidating three separate metadata classes into a single DynamicTestMetadata class.

---

✅ Strengths

1. Excellent Dual-Mode Implementation

The PR correctly implements the feature for both execution modes:

• ✅ Source-generated mode (AOT): Uses DynamicTestMetadata in AotTestDataCollector.cs
• ✅ Reflection mode: Uses DynamicTestMetadata in ReflectionTestDataCollector.cs
• ✅ Runtime mode: Uses DynamicTestMetadata in TestRegistry.cs

This follows Rule 1 from CLAUDE.md perfectly.

2. Code Consolidation

The PR eliminates significant code duplication by unifying three separate metadata implementations:

• Removed: AotDynamicTestMetadata (69 lines)
• Removed: DynamicReflectionTestMetadata (82 lines)
• Removed: RuntimeDynamicTestMetadata (78 lines)
• Added: Single unified DynamicTestMetadata class (125 lines)

Net result: -232 deletions, +260 additions = cleaner, more maintainable codebase.

3. Proper Snapshot Updates

✅ All 4 public API snapshot files updated correctly (DotNet8_0, DotNet9_0, DotNet10_0, Net4_7)

4. Good Test Coverage

The new DynamicTestDisplayNameTests.cs includes:

• Tests with custom display names
• Parameterized display names
• Default display name behavior (when not set)

5. Backward Compatibility

The DisplayName property is nullable (string?), so existing code without display names will continue to work with the fallback to TestName.

---

🔍 Potential Issues & Suggestions

1. Missing TestClassInstanceFactory in Reflection Mode ⚠️

In TUnit.Engine/Discovery/ReflectionTestDataCollector.cs:1922, the DynamicTestMetadata is created without setting TestClassInstanceFactory:

var metadata = new DynamicTestMetadata(result)
{
    TestName = testName,
    TestClassType = result.TestClassType,
    // ... other properties ...
};

However, in DynamicTestMetadata.CreateExecutableTest(), there's logic to use this factory:

if (modifiedContext.TestClassInstanceFactory != null)
{
    return await modifiedContext.TestClassInstanceFactory();
}

Issue: The TestClassInstanceFactory is never set, so this branch is dead code in reflection mode. The AOT mode (AotTestDataCollector.cs:131) also doesn't set it.

Recommendation: Either:

• Remove the dead code path, OR
• Populate TestClassInstanceFactory in the discovery collectors if it's needed for a specific scenario

2. DisplayName Assignment Redundancy 🟡

In TestBuilderPipeline.cs:331-335:

// Set custom display name for dynamic tests if specified
if (dynamicMetadata.DisplayName != null)
{
    context.Metadata.DisplayName = dynamicMetadata.DisplayName;
}

This seems redundant because:

• Line 298 already uses: var baseDisplayName = dynamicMetadata.DisplayName ?? resolvedMetadata.TestName;
• Line 307 creates TestDetails with TestName = resolvedMetadata.TestName
• The context.Metadata is the same as resolvedMetadata, which is already a DynamicTestMetadata

Question: Is this assignment necessary, or does it serve a different purpose (e.g., for discovery event receivers)?

If it's needed, consider adding a comment explaining why.

3. Test Method Implementation Pattern 🟡

In DynamicTestDisplayNameTests.cs:11-14:

public async Task TestWithCustomDisplayName()
{
    await Assert.That(TestContext.Current!.Metadata.DisplayName).IsEqualTo("My Custom Dynamic Test Name");
}

Minor suggestion: Consider using a local variable for readability:

public async Task TestWithCustomDisplayName()
{
    var actualDisplayName = TestContext.Current!.Metadata.DisplayName;
    await Assert.That(actualDisplayName).IsEqualTo("My Custom Dynamic Test Name");
}

This is purely stylistic but can make debugging easier.

4. Performance Consideration 🟢

The CreateExecutableTest method in DynamicTestMetadata.cs:44 is allocated on-demand via:

[field: AllowNull, MaybeNull]
public override Func<ExecutableTestCreationContext, TestMetadata, AbstractExecutableTest> CreateExecutableTestFactory
{
    get => field ??= CreateExecutableTest;
}

✅ This is good! Lazy initialization avoids allocating delegates for tests that might not be executed. Follows TUnit's performance-first philosophy.

5. Documentation 📝

The XML documentation is good, but consider adding an example to DynamicTest<T>.DisplayName:

/// <summary>
/// Custom display name for this dynamic test. If not set, a default name will be generated.
/// </summary>
/// <example>
/// <code>
/// context.AddTest(new DynamicTest&lt;MyTests&gt;
/// {
///     TestMethod = c => c.MyTest(),
///     DisplayName = "Custom Test Name"
/// });
/// </code>
/// </example>
public string? DisplayName { get; set; }

---

🧪 Testing Recommendations

1. ✅ Dual-mode tests: The current tests should run in both modes. Verify by checking test output.

2. Additional test scenarios to consider:

   • Test with DisplayName containing special characters (e.g., "Test <with> & symbols")
   • Test with very long display names (edge case handling)
   • Test with repeat attributes (e.g., [Repeat(3)]) to ensure repeat suffix is correctly appended
   • Test that display name appears correctly in test output/reports

3. Manual verification: Run the new tests with:

   cd TUnit.TestProject
   dotnet run -- --treenode-filter "/*/*/DynamicTestDisplayNameTests/*"

---

🎯 CLAUDE.md Compliance Checklist

• ✅ Rule 1 (Dual-Mode): Implemented in both source-gen and reflection modes
• ✅ Rule 2 (Snapshots): All public API snapshots updated and committed
• ✅ Rule 3 (No VSTest): Not applicable
• ✅ Rule 4 (Performance): Lazy initialization, no hot path changes
• ✅ Rule 5 (AOT): Uses proper [DynamicallyAccessedMembers] annotations
• ✅ Code Style: Uses modern C# (collection expressions, file-scoped namespaces, nullable references)
• ✅ Test Coverage: Includes comprehensive tests

---

🏁 Verdict

Overall Assessment: ✅ Approve with minor suggestions

This is a well-implemented PR that:

1. Adds a useful feature for dynamic test display names
2. Significantly improves code quality by consolidating duplicate metadata classes
3. Maintains dual-mode parity
4. Includes proper tests and documentation
5. Updates all required snapshots

The issues identified are minor and mostly about potential dead code cleanup and documentation improvements. They don't block the PR but would be nice to address.

---

📋 Action Items (Optional)

1. Investigate whether TestClassInstanceFactory logic is needed or can be removed
2. Clarify the purpose of the DisplayName assignment at line 331-335 in TestBuilderPipeline.cs
3. Consider adding usage examples to XML documentation
4. Manually verify tests run successfully in both modes

Great work! 🎉

[Copilot]

Pull request overview

This PR adds the ability to set custom display names for dynamic tests created via the DynamicTest<T> class. Previously, dynamic tests could only use auto-generated display names based on the test method name; now they can have human-readable custom names specified at test creation time.

Key changes:

• Added DisplayName property to the IDynamicTestMetadata interface and DynamicTest<T> class
• Created unified DynamicTestMetadata class to consolidate dynamic test metadata handling across AOT/source-generated and reflection modes
• Updated test execution pipeline to honor custom display names when provided

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
TUnit.Core/IDynamicTestMetadata.cs	Added DisplayName property to the interface for dynamic test metadata
TUnit.Core/AbstractDynamicTest.cs	Added DisplayName property to DynamicTest<T> and propagated it to DynamicDiscoveryResult
TUnit.Core/DynamicTestMetadata.cs	New unified metadata class for dynamic tests that consolidates logic from three separate implementations (AOT, Reflection, TestRegistry)
TUnit.Engine/Services/TestRegistry.cs	Replaced RuntimeDynamicTestMetadata with unified DynamicTestMetadata class
TUnit.Engine/Discovery/ReflectionTestDataCollector.cs	Replaced DynamicReflectionTestMetadata with unified DynamicTestMetadata class
TUnit.Engine/Building/Collectors/AotTestDataCollector.cs	Replaced AotDynamicTestMetadata with unified DynamicTestMetadata class
TUnit.Engine/Building/TestBuilderPipeline.cs	Updated to use custom DisplayName when building dynamic tests
TUnit.TestProject/DynamicTests/DynamicTestDisplayNameTests.cs	Integration tests validating custom display names work correctly for dynamic tests
TUnit.PublicAPI/*.verified.txt	Updated public API snapshots across all .NET versions to reflect new DisplayName property

[Copilot]

This code block sets the DisplayName on the context.Metadata, but this seems redundant because the same DisplayName value is already being passed to CreateExecutableTestFactory through the ExecutableTestCreationContext on line 343. The DynamicTestMetadata.CreateExecutableTest method already handles setting the display name via the DisplayName property passed in the context parameter (line 49 in DynamicTestMetadata.cs).

Consider removing this redundant assignment since the DisplayName is already properly propagated through the ExecutableTestCreationContext. This would simplify the code and avoid setting the same value twice through different paths.

Suggested change

	if (dynamicMetadata.DisplayName != null)
	{
	context.Metadata.DisplayName = dynamicMetadata.DisplayName;
	}