fix IAsyncInitializer order when using nested property injection

[thomhurst]

Fixes #4032

[claude]

Code Review - PR #4036

Summary

This PR fixes issue #4032 where nested IAsyncInitializer dependencies weren't being initialized in the correct order, breaking patterns like WebApplicationFactory + Testcontainers.

---

Strengths

1. Root Cause Fix: The change to CollectRootObjects correctly addresses the initialization order issue by only adding direct test class properties at depth 0, allowing nested properties to be discovered at appropriate depths (1+).

2. Excellent Test Coverage: The new test file NestedAsyncInitializerTests.cs is comprehensive:

   • Tests the exact scenario from issue IAsyncInitializer lifecycle change broke complex test infrastructure orchestration #4032 (WebApplicationFactory + Database container pattern)
   • Tests deep nesting (3 levels) with initialization order tracking
   • Uses thoughtful assertions to verify not just success, but correct ordering
   • Mock classes are well-designed with clear failure modes

3. Clear Documentation:

   • Inline comments explain the fix rationale
   • References GitHub issue IAsyncInitializer lifecycle change broke complex test infrastructure orchestration #4032
   • XML documentation in CollectRootObjects explains the depth-0 constraint

4. CLAUDE.md Addition: The update about running filters one at a time is a helpful operational note.

---

Code Quality Observations

ObjectGraphDiscoverer.cs (Lines 497-514)

Correctness: The logic is sound. Filtering by testClassPrefix ensures only direct properties are added at depth 0.

Performance: The StartsWith check with StringComparison.Ordinal is appropriate and efficient.

Edge Case Question: What happens if testDetails.ClassType.FullName is null? (Unlikely but worth considering)

The current implementation should handle this correctly (null would create prefix "null." which wouldn't match real properties), but consider adding a defensive null-check.

---

NestedAsyncInitializerTests.cs

Test Design: Excellent use of static counters, [NotInParallel], [Before(Class)], and shared instances.

Suggestions:

1. Test Isolation: Consider adding [After(Class)] cleanup as well for robustness
2. Thread Safety: Static collections are properly locked - good!

---

Testing Recommendations

Suggested Additional Test Cases:

1. Multiple test classes using same shared dependency
2. Mixed depth properties (both direct and nested with IAsyncInitializer)
3. Circular dependency detection

Dual-Mode Testing (Per CLAUDE.md Rule 1):
The tests should verify both source-generated and reflection modes. Current tests use [EngineTest] which may only test one mode. Consider adding explicit dual-mode variants to ensure the fix works in both execution paths.

---

Security & Performance

Security: No security concerns. The fix is contained to object graph discovery logic.

Performance:

• The StartsWith check adds minimal overhead
• No new allocations in hot path
• Maintains existing caching strategy

AOT Compatibility: No new reflection usage; existing suppressions are appropriate.

---

Pre-Commit Checklist Verification

Based on CLAUDE.md:

• Dual-mode feature? Yes - Both source-gen and reflection paths affected
  • Recommendation: Add explicit dual-mode tests
• Snapshot tests? N/A - No source generator output changes
• Public API changes? No
• Performance-critical? No - One-time initialization logic
• Reflection usage? Existing patterns maintained
• Code style? Modern C# syntax, proper documentation

---

Final Verdict

Recommendation: Approve with minor suggestions

This is a well-crafted fix for a critical initialization ordering bug. The code change is minimal and surgical, the tests are comprehensive, and the documentation is clear.

Suggested improvements before merge:

1. Add explicit dual-mode test variants (source-gen vs reflection)
2. Consider adding defensive null-check for ClassType.FullName
3. Add [After(Class)] cleanup to test classes for robustness

Nice work! The test coverage is particularly impressive - the mock classes clearly demonstrate the failure scenario and verify the fix.

[Copilot]

Pull request overview

This PR fixes issue #4032 where nested IAsyncInitializer dependencies weren't being initialized before their parent's InitializeAsync was called. This broke real-world patterns like WebApplicationFactory + Testcontainers where a container must be fully initialized before the factory tries to access its connection string.

Key Changes:

• Modified ObjectGraphDiscoverer.CollectRootObjects to filter property injection so only direct test class properties are added at depth 0
• Nested properties are now discovered through normal graph traversal at depth 1+, ensuring proper initialization order
• Added comprehensive regression tests simulating the WebApplicationFactory + Testcontainers pattern
• Updated CLAUDE.md with guidance about running TUnit.TestProject tests with filters

Reviewed changes

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

File	Description
TUnit.Core/Discovery/ObjectGraphDiscoverer.cs	Core fix: Filters injected properties to only include direct test class properties at depth 0, allowing nested properties to be initialized in correct order through graph traversal
TUnit.TestProject/Bugs/4032/NestedAsyncInitializerTests.cs	Comprehensive regression tests with mock classes simulating database containers and web application factories, including both 2-level and 3-level deep nesting scenarios
CLAUDE.md	Added important guidance about avoiding OR patterns in treenode-filter commands to prevent matching thousands of unintended tests

[Copilot]

Write to static field from instance method, property, or constructor.

[Copilot]

Write to static field from instance method, property, or constructor.

[Copilot]

Write to static field from instance method, property, or constructor.

[Copilot]

Write to static field from instance method, property, or constructor.

[Copilot]

Write to static field from instance method, property, or constructor.

Suggested change

	public DeepNestedDependency()
	{
	InstanceId = Interlocked.Increment(ref _instanceCount);
	}
	private DeepNestedDependency(int instanceId)
	{
	InstanceId = instanceId;
	}
	public static DeepNestedDependency Create()
	{
	var id = Interlocked.Increment(ref _instanceCount);
	return new DeepNestedDependency(id);
	}

[Copilot]

Write to static field from instance method, property, or constructor.

Suggested change

	public MiddleDependency()
	{
	InstanceId = Interlocked.Increment(ref _instanceCount);
	}
	private MiddleDependency(int instanceId)
	{
	InstanceId = instanceId;
	}
	public static MiddleDependency Create()
	{
	var id = Interlocked.Increment(ref _instanceCount);
	return new MiddleDependency(id);
	}

[Copilot]

Write to static field from instance method, property, or constructor.

[Copilot]

Write to static field from instance method, property, or constructor.

[Copilot]

Write to static field from instance method, property, or constructor.

[Copilot]

These 'if' statements can be combined.

Suggested change

	if (kvp.Key.StartsWith(testClassPrefix, StringComparison.Ordinal) && kvp.Value != null)
	{
	if (tryAdd(kvp.Value, 0))
	{
	onRootObjectAdded(kvp.Value);
	}
	if (kvp.Key.StartsWith(testClassPrefix, StringComparison.Ordinal) && kvp.Value != null && tryAdd(kvp.Value, 0))
	{
	onRootObjectAdded(kvp.Value);

[claude]

Code Review - PR #4036

Summary

This PR fixes a critical regression in IAsyncInitializer ordering where nested dependencies weren't being initialized before their parent objects accessed them. The fix correctly ensures that nested IAsyncInitializer dependencies (like Testcontainers) are initialized before parent objects (like WebApplicationFactory) try to use them.

Strengths

1. Correct Problem Analysis

The root cause was properly identified: nested properties of injected objects were being added at depth 0 alongside direct test class properties, which caused them to initialize in the wrong order.

2. Clean Implementation

• The IsDirectProperty check correctly distinguishes between direct test class properties and nested object properties
• GetTypeHierarchy properly walks the inheritance chain including base classes
• The cache key parsing logic correctly extracts the declaring type name

3. Excellent Test Coverage

The test file is exceptionally well-designed:

• Clear documentation explaining the regression and pattern
• Mock classes that simulate the real-world Testcontainers + WebApplicationFactory scenario
• Tests for both 2-level nesting (Factory -> Database) and 3-level deep nesting
• Initialization order tracking to verify correct sequencing
• Proper use of shared instances

4. Documentation

• Comprehensive XML comments explaining the fix
• CLAUDE.md update with helpful filter usage guidance
• Test classes have excellent inline documentation

Code Quality Analysis

Performance Considerations

• GetTypeHierarchy creates a new HashSet on each call to CollectRootObjects, but this is called once per test during discovery (not in hot path)
• LastIndexOf and Substring are efficient string operations
• The overhead is minimal compared to the critical correctness fix

Memory and Allocations

• The HashSet allocation is reasonable given the one-time nature of test discovery
• No unnecessary LINQ usage
• Modern C# collection expressions used appropriately

Potential Issues

1. Generic Type Handling

The FullName of generic types includes type parameters. The cache key might not match this format exactly depending on how the key is constructed elsewhere.

Recommendation: Add a test case with generic test classes to verify this works correctly.

2. Nested Generic Types

Nested types have FullName format like Outer+Inner. If there are nested classes with properties, the LastIndexOf might not correctly separate the type from property name if the cache key format differs.

3. Null FullName Edge Case (Already Handled)

Line 534 checks if currentType.FullName != null - good defensive coding for compiler-generated types.

CLAUDE.md Compliance

• Dual-Mode Implementation: This is a reflection-only change in TUnit.Core, which is appropriate since it affects the discovery engine
• Modern C# Syntax: Collection expressions used appropriately
• Performance First: No allocations in hot paths
• Code Style: Proper naming, XML comments, file-scoped namespaces
• Testing: Comprehensive test coverage with clear intent

Testing Recommendations

Tests to consider adding:

1. Generic test classes with injected properties
2. Multiple levels of inheritance with properties at different levels
3. Edge case: Cache keys with unusual characters or nested types
4. Stress test: Very deep inheritance hierarchy (10+ levels)

Minor Suggestions

1. Line 559: Consider using AsSpan for micro-optimization (optional)
2. Add example cache key format in comments
3. Consider splitting the 417-line test file if it grows further

Final Verdict

Recommendation: APPROVE with minor suggestions

This is a high-quality fix that:

• Correctly solves the reported issue
• Has excellent test coverage
• Follows TUnit conventions
• Uses modern C# idioms
• Maintains performance characteristics
• Is well-documented

The implementation is sound and the test coverage is exemplary. The minor suggestions above are optional improvements, not blockers.

Great work fixing this regression! The test infrastructure patterns (Testcontainers + WebApplicationFactory) are common in real-world testing scenarios, so this fix is important for users.

---

Risk Level: Low
Breaking Changes: None
Performance Impact: Negligible
Test Coverage: Excellent