feat: add DynamicTestIndex property for unique test ID generation in dynamic tests

[thomhurst]

Fixes #4052

[claude]

Code Review - PR #4056

Summary

This PR successfully addresses issue #4052 by adding a DynamicTestIndex property to ensure unique test IDs when multiple dynamic tests target the same method. The implementation is clean and follows TUnit architecture patterns.

Strengths

1. Correct Root Cause Fix: The issue was that multiple dynamic tests targeting the same method would generate identical test IDs because MethodDataLoopIndex was always 0. Using a sequential index (starting at 1 to match data source conventions) elegantly solves this.

2. Dual-Mode Implementation: The feature is correctly implemented in both execution modes - source-gen mode (AotTestDataCollector.cs:298) and reflection mode (ReflectionTestDataCollector.cs:2111 and TestRegistry.cs:356). This follows TUnit critical Rule 1 from CLAUDE.md.

3. Public API Snapshots Updated: All 4 verified.txt files were properly updated across .NET versions (8.0, 9.0, 10.0, Net4.7), following snapshot testing rules.

4. Good Documentation: XML doc comments clearly explain the purpose and usage of DynamicTestIndex.

5. Minimal, Focused Changes: Only 61 additions / 8 deletions - precisely what is needed without over-engineering.

Observations and Recommendations

1. Missing Test Coverage for the Bug Fix

Issue: There is no test that explicitly validates the bug fix - that multiple dynamic tests targeting the same method now generate unique IDs.

Recommendation: Add a test case to TUnit.TestProject that creates multiple dynamic tests for the same method with different arguments, then verify all tests execute with unique IDs. This would prevent regression.

2. Consider Thread Safety

Observation: DynamicTestBuilderContext._nextIndex uses a simple increment (_nextIndex++).

Question: Is AddTest() ever called concurrently? The [RequiresUnreferencedCode] attribute suggests it is called during test discovery, which is typically single-threaded, but worth confirming.

If concurrent access is possible: Consider using Interlocked.Increment
If guaranteed single-threaded: Add a comment to document this assumption.

3. Interface Design

Observation: IDynamicTestMetadata changed from a marker interface to having a property.

Comment: This is good design - the property belongs on the interface since all implementations need it. Well done.

Performance Considerations

No performance concerns:

• Simple integer increment (no allocations)
• Property access is trivial
• No reflection involved in the hot path
• Follows TUnit performance-first principles

Security Considerations

No security concerns:

• Integer overflow is not a practical concern (would need billions of dynamic tests)
• No user input involved
• No sensitive data exposed

Pre-Commit Checklist

Based on CLAUDE.md requirements:

• Dual-mode implementation (source-gen + reflection) ✓
• Public API snapshots updated and committed ✓
• Code follows modern C# style ✓
• No performance regressions ✓
• No VSTest references ✓
• Missing: Explicit test coverage for the bug fix (see Recommendation 1)

Final Verdict

APPROVE with minor recommendation

This is a solid fix that correctly addresses the root cause. The only improvement would be adding explicit test coverage to prevent regression. The implementation is clean, follows TUnit conventions, and properly handles both execution modes.

Great work solving this issue!

[Copilot]

Pull request overview

This PR adds a DynamicTestIndex property to support unique test ID generation for dynamic tests. This addresses issue #4052 where multiple dynamic tests targeting the same method would generate identical test IDs, causing collisions.

Key changes:

• Added DynamicTestIndex property to IDynamicTestMetadata interface and related classes to provide unique indexing for dynamic tests
• Modified DynamicTestBuilderContext to automatically assign sequential indices (starting at 1) when tests are added
• Updated all three execution paths (source-generated/AOT, reflection, and runtime) to use DynamicTestIndex in test ID generation via MethodDataLoopIndex

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
TUnit.PublicAPI/*.verified.txt	Updated public API snapshots across all target frameworks (.NET 4.7, 8.0, 9.0, 10.0) to reflect new DynamicTestIndex property
TUnit.Core/IDynamicTestMetadata.cs	Changed from marker interface to include DynamicTestIndex property; added documentation
TUnit.Core/AbstractDynamicTest.cs	Added DynamicTestIndex property to both AbstractDynamicTest and DynamicDiscoveryResult classes
TUnit.Core/DynamicTestBuilderContext.cs	Implemented auto-incrementing index assignment (starting at 1) when tests are added
TUnit.Engine/Building/TestBuilderPipeline.cs	Updated both dynamic test generation paths to use DynamicTestIndex for MethodDataLoopIndex
TUnit.Engine/Services/TestRegistry.cs	Implemented IDynamicTestMetadata.DynamicTestIndex in RuntimeDynamicTestMetadata
TUnit.Engine/Discovery/ReflectionTestDataCollector.cs	Implemented IDynamicTestMetadata.DynamicTestIndex in DynamicReflectionTestMetadata
TUnit.Engine/Building/Collectors/AotTestDataCollector.cs	Implemented IDynamicTestMetadata.DynamicTestIndex in AotDynamicTestMetadata

[claude]

Pull Request Review: DynamicTestIndex for Unique Test ID Generation

Overview

This PR successfully fixes issue #4052 by introducing a DynamicTestIndex property to ensure unique test IDs when multiple dynamic tests target the same method. The implementation is well-designed and follows TUnit's architectural patterns.

---

✅ Strengths

1. Excellent Dual-Mode Implementation

The fix correctly works in both execution modes:

• Source-Generated Mode: The source generator (DynamicTestsGenerator.cs:94) creates a DynamicTestBuilderContext which automatically increments _nextIndex in its AddTest method
• Reflection Mode: Three separate implementations (AotDynamicTestMetadata, DynamicReflectionTestMetadata, RuntimeDynamicTestMetadata) all properly expose DynamicTestIndex via the IDynamicTestMetadata interface

This is a textbook example of using shared abstractions to achieve dual-mode parity. ✨

2. Clean Architecture

• The IDynamicTestMetadata interface provides a clean contract for accessing the dynamic test index
• The index is assigned once at creation time in DynamicTestBuilderContext.AddTest(), preventing mutation issues
• The MethodDataLoopIndex is cleverly reused for ID generation, avoiding changes to the ID generation algorithm

3. Comprehensive Testing

• Integration test in TUnit.TestProject/DynamicTests/DynamicTestIndexTests.cs validates the real-world scenario
• Engine test in TUnit.Engine.Tests/DynamicTestIndexTests.cs verifies that all 5 tests execute successfully
• Tests inherit from InvokableTestBase which automatically tests both source-gen and reflection modes ✅

4. Proper Documentation

• XML comments clearly explain the purpose of DynamicTestIndex
• Code comments in TestBuilderPipeline.cs:155-156 explain the implementation choice
• Starting at index 1 aligns with existing conventions (noted in comment at DynamicTestBuilderContext.cs:14)

5. Snapshot Tests Updated

All four public API snapshot files were correctly updated to reflect the new properties. This follows the mandatory Rule 2 from CLAUDE.md. ✅

---

🔍 Code Quality Observations

Performance Considerations

• No allocations in hot path: The index is a simple int increment ✅
• No reflection overhead: The value is computed at discovery time, not execution time ✅
• Minimal impact: Only adds one integer field to existing structures ✅

Thread Safety

• ⚠️ Minor concern: DynamicTestBuilderContext._nextIndex is not thread-safe. However, this appears acceptable because:
  • The DynamicTestBuilderAttribute documentation likely expects single-threaded usage
  • Each builder context is isolated to a single method invocation
  • Adding synchronization would harm performance for the common case

Recommendation: Consider documenting that DynamicTestBuilderContext is not thread-safe if this isn't already clear in the API docs.

---

📝 Suggestions for Improvement

1. Test Coverage Enhancement (Minor)

The current test validates that 5 tests execute, but could be more specific:

// Current test only checks counts
result.ResultSummary.Counters.Total.ShouldBe(5)

// Could also validate unique test IDs
result.Tests.Select(t => t.TestId).Should().OnlyHaveUniqueItems()
// And verify all arguments were used
result.Tests.Select(t => t.Arguments[0]).Should().BeEquivalentTo([1, 2, 3, 4, 5])

This would more explicitly validate the bug fix (unique IDs) rather than just successful execution.

2. Edge Case: RepeatCount Interaction (Question)

In TestBuilderPipeline.cs:151-176, dynamic tests are multiplied by RepeatCount. The test ID generation uses:

• RepeatIndex for the repeat dimension
• DynamicTestIndex for the dynamic test dimension

Question: Have you verified that a dynamic test with [Repeat(3)] generates correct unique IDs? This would be worth adding as a test case to ensure the two indices don't conflict.

Example test case:

// Add to TUnit.TestProject/DynamicTests/DynamicTestIndexTests.cs
[Repeat(2)]
public void RepeatedTestMethod(int value) { }

// In builder: add 2 dynamic tests targeting RepeatedTestMethod
// Expected: 4 total tests (2 dynamic × 2 repeats) with unique IDs

3. Minor: Consistent Commenting Style

In TestBuilderPipeline.cs, the comment appears twice:

// Line 155: Use DynamicTestIndex from the metadata to ensure unique test IDs for multiple dynamic tests
// Line 276: Use DynamicTestIndex from the metadata to ensure unique test IDs for multiple dynamic tests

This is fine, but could be slightly more concise since it's an internal implementation detail.

---

🔒 Security & Safety

• ✅ No security concerns
• ✅ No unsafe code introduced
• ✅ AOT-compatible (no new reflection usage beyond existing RequiresUnreferencedCode warnings)
• ✅ No breaking changes to public API (only additions)

---

📋 CLAUDE.md Compliance Checklist

Based on TUnit's development guidelines:

• ✅ Rule 1 - Dual-Mode Implementation: Both source-gen and reflection modes implemented
• ✅ Rule 2 - Snapshot Testing: All four .verified.txt files updated
• ✅ Rule 3 - No VSTest: Not applicable
• ✅ Rule 4 - Performance First: Minimal overhead, no allocations
• ✅ Rule 5 - AOT/Trimming: No new reflection, compatible with AOT
• ✅ Code Style: Modern C# syntax, proper naming conventions
• ✅ Testing: Comprehensive test coverage in both modes

---

🎯 Final Verdict

LGTM with minor suggestions ✅

This is a solid fix that:

1. Solves the reported issue (Cannot add more than one dynamically created test for specific method #4052)
2. Follows TUnit's architectural patterns
3. Maintains dual-mode parity
4. Has good test coverage
5. Introduces no breaking changes

The suggestions above are minor enhancements rather than blocking issues. The PR is ready to merge as-is, though the additional test case for RepeatCount interaction would provide extra confidence.

---

💡 Additional Notes

The issue reporter also mentioned wanting custom test names (similar to NUnit's TestCaseData.SetName). While this PR doesn't address that, I see that DynamicDiscoveryResult.DisplayName already exists for this purpose. You might want to:

1. Document this capability in the dynamic tests documentation
2. Update the test example to show DisplayName usage
3. Consider adding this to the follow-up work

Example for future documentation:

context.AddTest(new DynamicTest<MyClass>
{
    TestMethod = c => c.MyTest(type),
    TestMethodArguments = [type],
    DisplayName = $"Smoke test for {type.Name}"  // Custom name here
});

Great work! 🚀