refactor: Consolidate initialisation logic

[thomhurst]

Fixes #3992

[claude]

Pull Request Review: Consolidate Initialization Logic

Summary

This PR addresses issue #3992 by fixing a critical bug where IAsyncInitializer objects (like Docker containers) were being initialized during test discovery instead of test execution.

✅ Strengths

1. Correct Bug Fix

Excellent fix introducing IAsyncDiscoveryInitializer to distinguish discovery-time vs execution-time initialization. This prevents expensive resource initialization during discovery.

2. Excellent Consolidation

Significantly reduces code duplication with ObjectGraphDiscoverer and helper classes.

3. Performance Optimizations

Good attention to performance with pre-calculated depths, capacity pre-allocation, and lazy initialization.

4. Thread Safety

Proper synchronization with Lazy and reference equality.

5. Comprehensive Testing

Bug recreation test validates the fix correctly.

---

⚠️ CRITICAL ISSUE

ReferenceEqualityComparer Generic Type Missing

Location: TUnit.Core/Helpers/ReferenceEqualityComparer.cs

The code uses ReferenceEqualityComparer<object>.Instance in multiple places but the class is defined as non-generic. This will cause compilation errors.

Found in:

• StructuralEquivalencyAssertion.cs:85
• NotStructuralEquivalencyAssertion.cs:114

Fix needed: Either make ReferenceEqualityComparer generic or change all usages to just Instance.

---

⚠️ Other Issues

1. Memory Leak Risk: Static DiscoveryErrors collection grows indefinitely
2. Lock Contention: AddToDepth locks on HashSet in hot path
3. Error Swallowing: Discovery errors not surfaced to users
4. Debug-Only Logging: Recursion depth warnings only in DEBUG
5. Inconsistent Null Handling: Different behavior across methods
6. Missing Tests: No tests for circular refs, errors, cancellation

---

📊 Assessment

Category	Rating
Bug Fix	⭐⭐⭐⭐⭐
Code Quality	⭐⭐⭐⭐☆
Performance	⭐⭐⭐⭐☆
Thread Safety	⭐⭐⭐⭐⭐
Documentation	⭐⭐⭐⭐☆
Test Coverage	⭐⭐⭐☆☆

---

🎯 Verdict

Conditional Approval - Excellent refactoring but CRITICAL issue must be fixed before merge.

Required:

1. Fix ReferenceEqualityComparer compilation issue
2. Run dotnet publish -p:PublishAot=true
3. Add edge case tests
4. Document cleanup requirements

Great work overall! 🎉

[Copilot]

Pull request overview

This PR consolidates initialization logic to fix issue #3992 by implementing phase-aware object initialization. The refactoring separates discovery-phase initialization (IAsyncDiscoveryInitializer) from execution-phase initialization (IAsyncInitializer), ensuring resources like Docker containers aren't started until actually needed during test execution.

Key Changes

• Phase-aware initialization: Split initialization into discovery and execution phases with explicit control via InitializeForDiscoveryAsync and InitializeAsync methods
• Consolidated object graph discovery: Created ObjectGraphDiscoverer in TUnit.Core to eliminate duplicate graph traversal logic across multiple services
• Performance optimizations: Added caching, removed LINQ allocations in hot paths, and introduced ParallelTaskHelper for efficient parallel task execution

Reviewed changes

Copilot reviewed 44 out of 44 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
TUnit.Core/ObjectInitializer.cs	Refactored to use Lazy pattern with phase-aware initialization methods
TUnit.Core/Discovery/ObjectGraphDiscoverer.cs	New centralized service consolidating object graph traversal logic
TUnit.Core/Services/ObjectInitializationService.cs	New public API service delegating to ObjectInitializer
TUnit.Engine/ObjectLifecycleService.cs	Updated to separate property injection from initialization with phase control
TUnit.Engine/TestInitializer.cs	Split into PrepareTest and InitializeTestObjectsAsync for phased execution
TUnit.Engine/TestExecutor.cs	Modified to initialize test objects after BeforeClass hooks
TUnit.Core/Helpers/ParallelTaskHelper.cs	New helper eliminating LINQ allocations in parallel operations
TUnit.Core/PropertyInjection/PropertyCacheKeyGenerator.cs	New utility centralizing cache key generation
TUnit.Core/Tracking/ObjectTracker.cs	Enhanced with better thread safety and disposal callback handling
TUnit.Assertions/Conditions/Helpers/*.cs	New assertion helpers consolidating reflection and type checking logic
TUnit.PublicAPI/*.verified.txt	Updated API snapshots reflecting new public interfaces

[Copilot]

Using StringComparison.Ordinal is good practice. However, on line 33, the second Contains call doesn't specify a comparison type. For consistency and to avoid culture-specific issues, consider using Contains("=>", StringComparison.Ordinal) if available in the target framework, or using IndexOf("=>", StringComparison.Ordinal) >= 0.

Suggested change

	if (variable.Contains("=>") || variable.StartsWith("()", StringComparison.Ordinal))
	if (variable.Contains("=>", StringComparison.Ordinal) || variable.StartsWith("()", StringComparison.Ordinal))

[Copilot]

This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.

[Copilot]

This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.

[Copilot]

This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.

[Copilot]

This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.

Suggested change

	foreach (var obj in kvp.Value)
	{
	if (!alreadyTracked.Contains(obj))
	{
	newTrackableObjects.Add(obj);
	}
	foreach (var obj in kvp.Value.Where(obj => !alreadyTracked.Contains(obj)))
	{
	newTrackableObjects.Add(obj);

[Copilot]

This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.

[claude]

PR Review: Consolidate Initialization Logic

Summary: This is a well-structured refactoring that consolidates scattered initialization logic across the codebase. The PR successfully addresses issue #3992 by ensuring IAsyncInitializer objects are not initialized during test discovery, only during test execution.

---

Strengths

1. Excellent Consolidation and DRY Principles

• Successfully consolidates duplicate object graph traversal logic from ObjectGraphDiscoveryService and TrackableObjectGraphProvider into a single ObjectGraphDiscoverer class
• Eliminates code duplication in assertion helpers by extracting shared logic into ExpressionHelper, ReflectionHelper, and TypeHelper
• Unified ReferenceEqualityComparer across all components with singleton pattern

2. Strong Separation of Concerns

• Clear interface segregation with IObjectGraphDiscoverer (read-only) vs IObjectGraphTracker (mutating)
• PropertyCacheManager properly extracted from ObjectGraphDiscoverer following Single Responsibility Principle
• Phase-based lifecycle management in ObjectLifecycleService: Register -> Inject -> Initialize -> Cleanup

3. Performance Optimizations

• PropertyCacheManager includes automatic cache cleanup to prevent memory leaks (MaxCacheSize: 10000)
• Uses explicit loops instead of LINQ in hot paths
• ParallelTaskHelper provides allocation-free parallel execution
• Pre-allocated task arrays in parallel operations
• Lazy initialization patterns with Lazy for thread-safe one-time execution

4. Thread Safety

• Proper use of ConcurrentDictionary with ReferenceEqualityComparer
• Lock coordination for HashSet modifications in ObjectGraphDiscoverer.AddToDepth
• Interlocked.CompareExchange for cache cleanup coordination in PropertyCacheManager
• Lazy with LazyThreadSafetyMode.ExecutionAndPublication in ObjectInitializer

5. Excellent Documentation

• Comprehensive XML documentation throughout
• Clear justifications for UnconditionalSuppressMessage attributes
• Debug output for diagnostics
• Discovery errors collected for debugging rather than thrown

6. AOT/Trimming Compatibility

• Proper [DynamicallyAccessedMembers] annotations on reflection methods
• Clear justifications for trimming suppressions
• Reflection positioned as fallback with source-gen as primary path

---

Issues and Recommendations

MEDIUM: Cache Cleanup Strategy in PropertyCacheManager

Issue: PropertyCacheManager.GetCachedProperties (lines 44-72) uses a simple remove-first-half strategy when cache exceeds limit.

Location: TUnit.Core/Discovery/PropertyCacheManager.cs:44-72

Concerns:

1. No LRU or usage-based eviction - frequently used types might be evicted
2. ToArray() creates a snapshot, but eviction is based on arbitrary enumeration order
3. No guarantee that recently accessed types are preserved

Recommendation:

• Add a comment explaining the trade-off (simplicity vs. optimality)
• Note that in practice, type sets are relatively stable per test session
• If cache thrashing becomes an issue, consider LRU implementation

---

MINOR: Dual Visited Set Tracking

Issue: StructuralEquivalencyAssertion.CompareObjects now tracks both visitedActual and visitedExpected, which is correct for preventing infinite recursion in both object graphs.

Location: TUnit.Assertions/Conditions/StructuralEquivalencyAssertion.cs:90-148

Recommendation: Add a comment explaining why both tracking sets are needed (cycles in both actual and expected graphs can cause infinite recursion).

---

MINOR: Missing Disposal Pattern Test

Issue: The bug test (BugRecreation.cs) verifies that DummyContainer is not initialized during discovery but only during execution. However, it does not verify the disposal behavior.

Location: TUnit.TestProject/Bugs/3992/BugRecreation.cs

Recommendation: Add a test verifying that DisposeAsync is called exactly once after test execution completes.

---

MINOR: Error Collection Without Limits

Issue: ObjectGraphDiscoverer.DiscoveryErrors (line 63) is a ConcurrentBag that grows unbounded until ClearDiscoveryErrors() is called.

Location: TUnit.Core/Discovery/ObjectGraphDiscoverer.cs:63

Scenario: In a long-running test session with many property access failures, this could accumulate significant memory.

Recommendation:

• Add a maximum error count (similar to PropertyCacheManager.MaxCacheSize)
• Or document that it is acceptable since it is cleared at session end
• Or use a bounded collection that automatically evicts oldest errors

---

Checklist Verification

Based on CLAUDE.md requirements:

• ✅ Dual-Mode Implementation: Not applicable (refactoring, not new feature)
• ✅ Snapshot Testing: Public API snapshots updated (4 .verified.txt files committed)
• ✅ No VSTest: No VSTest references introduced
• ✅ Performance First: Cached reflection, pre-allocated arrays, avoids LINQ in hot paths
• ✅ AOT/Trimming: Proper [DynamicallyAccessedMembers] annotations
• ✅ Code Style: Modern C# (collection expressions, file-scoped namespaces, ValueTask)

---

Test Coverage

Good:

• Bug reproduction test verifies fix for Test discovery calls async initializers? #3992
• Tests verify initialization is deferred from discovery to execution

Could Improve:

• Add explicit test for PropertyCacheManager cleanup behavior
• Test ObjectGraphDiscoverer.GetDiscoveryErrors() diagnostics
• Verify thread-safety of concurrent object graph discovery
• Test disposal lifecycle end-to-end

---

Security

No security concerns identified.

---

Overall Assessment

Rating: 8.5/10

This is a high-quality refactoring that significantly improves code organization, eliminates duplication, and fixes a real bug (#3992). The code follows modern C# best practices, includes excellent documentation, and demonstrates strong understanding of concurrency and performance.

Should Fix:

1. Add comment explaining dual visited set tracking in CompareObjects
2. Document cache cleanup strategy in PropertyCacheManager

Nice to Have:
3. Add disposal lifecycle test
4. Add bounds to error collection (or document it is acceptable)

Recommendation: Approve and merge. Items 1-2 are minor and could be addressed in a follow-up PR if desired.

---

Great work on this consolidation! The separation of discovery vs. initialization phases is much clearer now, and the fix for #3992 ensures Docker containers and other expensive resources are not spun up during test discovery.