Fix ClassDataSource nested property injection not respecting shared types

[thomhurst]

Fixes #3803

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review Progress

Reviewing PR: Fix ClassDataSource nested property injection not respecting shared types

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze the core fix in ClassDataSources.cs
• Review test implementation files
• Check code quality and best practices
• Evaluate performance considerations
• Assess security concerns
• Verify test coverage
• Provide comprehensive feedback

---

📋 Code Review Summary

This PR fixes a critical bug where nested ClassDataSource properties with SharedType.PerTestSession were creating multiple instances instead of sharing a single instance across the test session. The issue was in the ClassDataSources.Create method, which was handling nested dependency injection manually instead of relying on the proper dual-mode architecture.

---

✅ Positive Aspects

1. Correct Problem Identification

The bug was properly identified: nested properties marked with SharedType.PerTestSession were not respecting the sharing behavior because the manual property injection was bypassing the established TestDataContainer infrastructure.

2. Clean, Minimal Fix

The fix correctly removes the problematic manual property injection code and simplifies the Create method to rely on the framework's existing dual-mode architecture.

3. Comprehensive Test Coverage

Excellent test implementation that:

• Tests across multiple test classes to verify session-level sharing
• Uses thread-safe counters to track instance creation
• Includes detailed logging for debugging
• Verifies both initialization and disposal behaviors
• Tests the complete dependency chain (Factory → Containers)

4. Proper Use of Threading Constructs

Test containers use Interlocked.Increment for thread-safe counter management, which is appropriate for concurrent test execution.

---

🔧 Technical Analysis

Root Cause

The old CreateWithNestedDependencies method was manually handling property injection using PropertySourceRegistry, which bypassed the TestDataContainer.GetGlobalInstance() calls that implement the sharing behavior. This created new instances for each nested property instead of reusing shared instances.

Solution Assessment

The fix is architecturally sound:

• Before: Manual property injection → bypasses sharing
• After: Relies on framework's dual-mode architecture → proper sharing

The framework already handles nested properties correctly through:

1. Source Generation Mode: Generated code calls appropriate TestDataContainer.GetXXXInstance methods
2. Reflection Mode: Property discovery and injection respects the sharing types

---

💡 Recommendations

1. Consider Dual-Mode Verification ⚠️

According to TUnit's Rule 1 (Dual-Mode Implementation), this fix should be tested in both execution modes. Consider adding explicit test cases:

[Test]
[Arguments(ExecutionMode.SourceGenerated)]
[Arguments(ExecutionMode.Reflection)]
public async Task SharedContainers_WorkInBothModes(ExecutionMode mode)
{
    // Test the fix works in both source-gen and reflection modes
}

2. Performance Consideration ✅

The fix actually improves performance by:

• Removing unnecessary reflection calls in PropertySourceRegistry.GetSource()
• Eliminating recursive property traversal
• Reducing object allocations through proper sharing

3. Add AOT Compatibility Test

Since this touches object creation, verify AOT compatibility:

cd TUnit.TestProject
dotnet publish -c Release -p:PublishAot=true --use-current-runtime

---

🐛 Potential Concerns

1. Missing Error Handling Context

The simplified Create method loses the context about which properties were being injected. If property injection fails in the new approach, the error messages might be less informative.

Mitigation: The framework's property injection should handle this, but monitor for any error message degradation.

2. Regression Risk Assessment

Low Risk - The fix removes problematic code rather than adding complexity. The existing dual-mode architecture handles the functionality correctly.

---

📊 Test Quality Assessment

Strengths

• ✅ Tests actual bug scenario with realistic nested dependencies
• ✅ Uses proper async/await patterns
• ✅ Includes detailed logging for debugging
• ✅ Tests multiple test classes (session-level sharing)
• ✅ Verifies both creation and initialization counts
• ✅ Uses thread-safe operations

Potential Improvements

• Consider testing edge cases (circular dependencies, deeply nested properties)
• Add negative tests (what happens when sharing fails)

---

🔒 Security Review

No security concerns identified. The changes:

• Reduce reflection usage
• Don't introduce new attack vectors
• Maintain existing access controls

---

📋 Compliance with TUnit Guidelines

• ✅ Modern C# Syntax: Code uses appropriate patterns
• ✅ Performance-First: Fix improves performance
• ✅ AOT Compatibility: Maintains existing AOT annotations
• ⚠️ Dual-Mode Testing: Should verify both execution modes
• ✅ Proper Error Handling: Maintains exception propagation

---

🎯 Final Verdict

✅ APPROVED - This is a well-designed fix for a legitimate bug.

Key Strengths:

• Addresses the root cause correctly
• Simplifies code while fixing the issue
• Includes comprehensive test coverage
• Improves performance
• Maintains architectural integrity

Recommended Actions Before Merge:

1. Add dual-mode test verification
2. Run AOT compatibility test
3. Verify no regression in error messages

This fix demonstrates a solid understanding of TUnit's architecture and correctly leverages the existing infrastructure rather than working around it.

---