feat(tests): Fix property injection initialisation within base classes

[thomhurst]

Fixes #3597

[claude]

Claude finished @thomhurst's task —— View job

---

PR Review: Property Injection Initialization Fix

Reviewing PR #3601 that fixes property injection initialization within base classes (issue #3597).

🎯 Code Quality Review

✅ Excellent Improvements

1. Bug Fix - Property Injection Order

• Problem: Properties with [ClassDataSource] in base classes weren't being initialized before IAsyncInitializer.InitializeAsync() was called
• Solution: Enhanced inheritance discovery in source generator (lines 73-77, 99-117) and fixed UnsafeAccessor to target correct containing type (lines 268-274)
• Result: Property injection now works correctly across inheritance hierarchies

2. Architectural Simplification

• Removed: Strategy pattern complexity (4 strategy classes + pipeline)
• Added: Direct orchestration with cleaner separation of concerns
• Impact: -370 lines of code while maintaining functionality

3. Circular Dependency Resolution

• Problem: PropertyInjectionService and ObjectRegistrationService had circular dependencies
• Solution: Introduced IObjectRegistry interface with two-phase initialization (PropertyInjectionService.cs:25-29)
• Result: Clean dependency injection without compromising type safety

✅ Technical Excellence

Dual-Mode Implementation ✅

Both source generation and reflection modes are properly implemented:

• Source Generator: Enhanced inheritance support (PropertyInjectionSourceGenerator.cs:67-118)
• Reflection: Maintained compatibility in PropertyInitializationOrchestrator.cs:56-73
• Tests: Dedicated test class verifies both modes work identically

Performance Optimizations ✅

• Parallel property initialization maintained (PropertyInitializationOrchestrator.cs:43-46)
• Object pooling and caching preserved
• Removed strategy pattern overhead
• Zero-allocation improvements with proper use of ConcurrentDictionary<object, byte>

Security & Safety ✅

• UnsafeAccessor: Properly scoped to .NET 8+ with reflection fallback
• AOT Compatibility: Proper [UnconditionalSuppressMessage] annotations
• Type Safety: ContainingType correctly used instead of derived class type

Code Style ✅

• Modern C# patterns used consistently
• Proper async/await patterns maintained
• Clear separation of concerns
• Excellent documentation and comments

🔍 Minor Areas for Consideration

1. Error Handling Robustness

The error handling in PropertyInjectionService.cs:129-139 is good, but could benefit from more specific exception types for different failure modes.

2. Test Coverage Gap

While the bug fix test is excellent, consider adding a test that specifically verifies the initialization order with multiple inheritance levels.

3. Performance Testing

Given this touches hot paths (property initialization), consider running performance benchmarks to ensure the architectural change doesn't introduce regressions.

📊 Impact Assessment

Aspect	Rating	Notes
Correctness	⭐⭐⭐⭐⭐	Fixes the reported bug completely
Performance	⭐⭐⭐⭐⭐	Maintains perf while reducing complexity
Maintainability	⭐⭐⭐⭐⭐	Significant improvement (-370 LOC)
Safety	⭐⭐⭐⭐⭐	Proper AOT/trimming support
Test Coverage	⭐⭐⭐⭐	Good coverage, could be enhanced

🎯 Recommendation: APPROVE

This is an excellent refactoring that:

• ✅ Fixes a critical inheritance bug
• ✅ Significantly simplifies the architecture
• ✅ Maintains performance and dual-mode compliance
• ✅ Follows TUnit development best practices
• ✅ Reduces maintenance burden

The code quality is high and the architectural changes represent a clear improvement. The fix is targeted and well-tested.

---

[claude]

Claude finished @thomhurst's task —— View job

---

PR Review: Property Injection Initialization Fix

Reviewing PR #3601 that fixes property injection initialization within base classes (issue #3597).

Todo List:

• Read repository guidelines (CLAUDE.md)
• Understand the bug being fixed by examining test files
• Review source generator changes
• Analyze property initialization orchestrator changes
• Examine deleted strategy classes and architectural changes
• Check dual-mode implementation compliance
• Assess test coverage and potential issues
• Provide comprehensive review feedback

---

🔍 Review Summary

This PR successfully fixes the property injection initialization bug in base class inheritance scenarios while significantly simplifying the architecture. The changes are well-implemented and maintain TUnit's critical dual-mode compatibility.

✅ Strengths

1. Effective Bug Fix

• Root Cause: Property injection in derived classes wasn't correctly accessing base class properties
• Solution: Enhanced source generator to discover inherited properties (lines 73-77, 99-117) and use proper ContainingType in UnsafeAccessor methods (lines 269, 366-374)
• Test Coverage: Added comprehensive test case that verifies fix works in both execution modes

2. Architectural Simplification

• Removed complexity: Eliminated strategy pattern with 4 strategy classes + pipeline orchestrator (705 deletions)
• Added clarity: Direct property initialization in PropertyInitializationOrchestrator is much more readable
• Dependency breaking: New IObjectRegistry interface elegantly solves circular dependency issues

3. Dual-Mode Compliance ✅

• Source-generated mode: Enhanced inheritance detection and proper ContainingType handling
• Reflection mode: Parallel implementation maintained in PropertyInitializationOrchestrator
• Mode switching: Both paths correctly use SourceRegistrar.IsEnabled for dynamic selection

4. Performance Improvements

• Parallel processing: Properties initialized concurrently via Task.WhenAll() (lines 43-46, 69-72)
• Reduced allocations: Eliminated intermediate strategy objects and pipeline overhead
• Caching maintained: PropertyInjectionCache usage preserved for performance

⚠️ Minor Concerns & Suggestions

1. Code Quality Issues

PropertyInjectionService.cs:19 - Null assignment with null-forgiving operator:

_orchestrator = new PropertyInitializationOrchestrator(dataSourceInitializer, null!);

Suggestion: Consider using a placeholder or making the constructor parameter nullable with proper null handling.

PropertyInjectionService.cs:162-163 - Reflection usage in source-generated path:

var property = metadata.ContainingType.GetProperty(metadata.PropertyName);

Concern: This adds reflection overhead even in source-generated mode. Consider caching PropertyInfo or pre-generating property accessors.

2. Test Coverage Gaps

Missing edge cases:

• Multiple inheritance levels (grandparent → parent → child)
• Mix of virtual/override properties with data sources
• Properties with different access modifiers (protected, internal)
• Properties with custom init-only setters

Suggestion: Add additional test cases for these scenarios.

3. Documentation

Missing XML documentation for key public interfaces:

• IObjectRegistry methods need more detailed parameter documentation
• PropertyInitializationOrchestrator public methods need examples

🛡️ Security & Safety

• AOT Compatibility: Proper [UnconditionalSuppressMessage] usage for trimming (lines 78-80, 137-139)
• Null Safety: Good null checking throughout, though some null-forgiving operators could be improved
• Thread Safety: Concurrent collections used appropriately for parallel execution

🚀 Performance Impact

Positive:

• Eliminated strategy instantiation overhead
• Parallel property initialization
• Simplified call stack

Neutral:

• Reflection usage in source-gen mode (line 162) - consider optimization

📋 Pre-Commit Checklist Status

Based on CLAUDE.md requirements:

• ✅ Dual-mode implementation maintained
• ⚠️ Snapshot tests may need updating (source generator changes)
• ✅ Performance optimized (parallel execution)
• ✅ Modern C# syntax used
• ✅ No VSTest dependencies

Recommendation: Run dotnet test TUnit.Core.SourceGenerator.Tests to verify snapshot tests.

🎯 Final Verdict

✅ APPROVE - This is a high-quality PR that:

1. Successfully fixes the reported bug
2. Significantly improves code maintainability
3. Maintains performance and TUnit's dual-mode architecture
4. Includes appropriate test coverage

The architectural simplification from strategy pattern to direct orchestration is a welcome improvement that makes the codebase more accessible while maintaining all functionality.

---

[claude]

Claude finished @thomhurst's task —— View job

---

PR Review: Property Injection Initialization Fix

Reviewing PR #3601 that fixes property injection initialization within base classes (issue #3597).

Todo List:

• Read repository guidelines (CLAUDE.md)
• Understand the bug being fixed by examining test files
• Review source generator changes
• Analyze property initialization orchestrator changes
• Examine deleted strategy classes and architectural changes
• Check dual-mode implementation compliance
• Assess test coverage and potential issues
• Provide comprehensive review feedback

---

🔍 Review Summary

This PR successfully fixes the property injection initialization bug in base class inheritance scenarios while significantly simplifying the architecture. The changes are well-implemented and maintain TUnit's critical dual-mode compatibility.

✅ Strengths

1. Effective Bug Fix

• Root Cause: Property injection in derived classes wasn't correctly accessing base class properties
• Solution: Enhanced source generator to discover inherited properties (lines 73-77, 99-117) and use proper ContainingType in UnsafeAccessor methods (lines 269, 366-374)
• Test Coverage: Added comprehensive test case that verifies fix works in both execution modes

2. Architectural Simplification

• Removed complexity: Eliminated strategy pattern with 4 strategy classes + pipeline orchestrator (705 deletions)
• Added clarity: Direct property initialization in PropertyInitializationOrchestrator is much more readable
• Dependency breaking: New IObjectRegistry interface elegantly solves circular dependency issues

3. Dual-Mode Compliance ✅

• Source-generated mode: Enhanced inheritance detection and proper ContainingType handling
• Reflection mode: Parallel implementation maintained in PropertyInitializationOrchestrator
• Mode switching: Both paths correctly use SourceRegistrar.IsEnabled for dynamic selection

4. Performance Improvements

• Parallel processing: Properties initialized concurrently via Task.WhenAll() (lines 43-46, 69-72)
• Reduced allocations: Eliminated intermediate strategy objects and pipeline overhead
• Caching maintained: PropertyInjectionCache usage preserved for performance

⚠️ Minor Concerns & Suggestions

1. Code Quality Issues

PropertyInjectionService.cs:19 - Null assignment with null-forgiving operator:

_orchestrator = new PropertyInitializationOrchestrator(dataSourceInitializer, null!);

Suggestion: Consider using a placeholder or making the constructor parameter nullable with proper null handling.

PropertyInjectionService.cs:162-163 - Reflection usage in source-generated path:

var property = metadata.ContainingType.GetProperty(metadata.PropertyName);

Concern: This adds reflection overhead even in source-generated mode. Consider caching PropertyInfo or pre-generating property accessors.

2. Test Coverage Gaps

Missing edge cases:

• Multiple inheritance levels (grandparent → parent → child)
• Mix of virtual/override properties with data sources
• Properties with different access modifiers (protected, internal)
• Properties with custom init-only setters

Suggestion: Add additional test cases for these scenarios.

3. Documentation

Missing XML documentation for key public interfaces:

• IObjectRegistry methods need more detailed parameter documentation
• PropertyInitializationOrchestrator public methods need examples

🛡️ Security & Safety

• AOT Compatibility: Proper [UnconditionalSuppressMessage] usage for trimming (lines 78-80, 137-139)
• Null Safety: Good null checking throughout, though some null-forgiving operators could be improved
• Thread Safety: Concurrent collections used appropriately for parallel execution

🚀 Performance Impact

Positive:

• Eliminated strategy instantiation overhead
• Parallel property initialization
• Simplified call stack

Neutral:

• Reflection usage in source-gen mode (line 162) - consider optimization

📋 Pre-Commit Checklist Status

Based on CLAUDE.md requirements:

• ✅ Dual-mode implementation maintained
• ⚠️ Snapshot tests may need updating (source generator changes)
• ✅ Performance optimized (parallel execution)
• ✅ Modern C# syntax used
• ✅ No VSTest dependencies

Recommendation: Run dotnet test TUnit.Core.SourceGenerator.Tests to verify snapshot tests.

🎯 Final Verdict

✅ APPROVE - This is a high-quality PR that:

1. Successfully fixes the reported bug
2. Significantly improves code maintainability
3. Maintains performance and TUnit's dual-mode architecture
4. Includes appropriate test coverage

The architectural simplification from strategy pattern to direct orchestration is a welcome improvement that makes the codebase more accessible while maintaining all functionality.

---