feat: make dependencies available in ITestRegisteredEventReceiver

[thomhurst]

Summary

• Restructures test building flow to defer ITestRegisteredEventReceiver and ITestDiscoveryEventReceiver invocations until after dependencies are resolved
• Allows users to access resolved dependencies (including transitive ones) in their event receivers
• Enables scenarios like the "Focus" feature where tests can be conditionally skipped based on dependency relationships

Changes

• TestBuilder.cs: Split InvokeTestRegisteredEventReceiversAsync into RegisterTestArgumentsAsync (property injection during building) and InvokeTestRegisteredReceiversAsync (receiver invocation after dependencies resolved). Added InvokePostResolutionEventsAsync() that populates TestContext._dependencies and fires events.
• ITestBuilder.cs: Added InvokePostResolutionEventsAsync() to interface
• TestBuilderPipeline.cs: Added delegating InvokePostResolutionEventsAsync() method
• TestDiscoveryService.cs: Call InvokePostResolutionEventsAsync() after dependency resolution. Fixed bug where metadata was pre-filtered before dependency expansion.
• TestCoordinator.cs: Removed duplicate dependency collection (now done in discovery)
• TUnitServiceProvider.cs: Removed unused HashSetPool parameter

Test plan

• Added new tests in DependenciesAvailableInEventReceiverTests.cs verifying:
  • Same-class dependencies are available at registration time
  • Cross-class dependencies are available at registration time
  • Transitive dependencies are available at registration time
• Existing DependsOnTests pass
• Existing DependsOnTestsWithClass pass

[thomhurst]

Summary

Makes dependencies (including transitive ones) available to ITestRegisteredEventReceiver and ITestDiscoveryEventReceiver by deferring event invocation until after dependency resolution.

Critical Issues

None found ✅

Suggestions

1. Performance consideration in discovery (non-blocking)

The change in TestDiscoveryService.cs:77 removes filter pre-filtering. While the PR body correctly identifies this as a bug fix ("metadata was pre-filtered before dependency expansion"), this means discovery will now materialize ALL test metadata before filtering, which could impact discovery performance on large codebases when narrow filters are used.

This is the correct fix for the dependency expansion bug, but consider monitoring discovery performance in very large test suites. The tradeoff (correctness vs. performance) is appropriate here.

2. HashSet allocations per test (minor)

In PopulateDependencies (TestBuilder.cs:972-996), each test creates new HashSet instances. This is called once per test during discovery. For large test suites with many dependencies, consider using an object pool for these collections (similar to the old HashSetPool pattern that was removed from TestCoordinator). However, since this only runs once during discovery (not in the hot execution path), the impact is minimal.

3. Test coverage looks solid

The new tests in DependenciesAvailableInEventReceiverTests.cs comprehensively verify:

• Same-class dependencies ✅
• Cross-class dependencies ✅
• Transitive dependencies ✅

Great test coverage for the new feature!

TUnit Rules Compliance

✅ Dual-mode: Changes only affect TUnit.Engine (reflection mode). Source generator mode doesn't need updates.
✅ No VSTest: No Microsoft.VisualStudio.TestPlatform references added.
✅ AOT Compatible: Proper [RequiresUnreferencedCode] annotations on InvokePostResolutionEventsAsync.
✅ Performance: Discovery phase changes are justified by correctness fix. New allocations are in discovery, not hot execution path.

Verdict

✅ APPROVE - Clean implementation that correctly solves the dependency availability issue. The filter pre-optimization removal is a necessary bug fix with acceptable performance tradeoffs.

[thomhurst]

Review of PR 4589 - See detailed analysis below

[thomhurst]

Summary

This PR restructures test building to defer event receiver invocations until after dependencies are resolved, enabling ITestRegisteredEventReceiver and ITestDiscoveryEventReceiver to access dependency information.

Critical Issues Found

[thomhurst]

Issue 1: Potential duplicate dictionary keys in AotTestDataCollector.cs:102

Location: TUnit.Engine/Building/Collectors/AotTestDataCollector.cs:98-102

In the ExpandSourcesForDependencies method, this code creates duplicate dictionary entries. If multiple generic variations exist (e.g., MyClass with both T and T,U), the non-generic key will point to whichever was processed last, potentially causing incorrect dependency resolution.

Recommendation: Either document this intentional last-wins behavior, or use a multi-value dictionary structure if all generic variations should be discoverable.

[thomhurst]

Issue 2: String allocations in hot path (AotTestDataCollector.cs:120-125)

The ExpandSourcesForDependencies method uses Substring operations during test discovery, which is a hot path per TUnit Rule 4. Each dependency creates string allocations.

Recommendation: Use ReadOnlySpan with AsSpan() to avoid allocations and improve performance during test discovery.

[thomhurst]

Suggestions

1. TestBuilder.cs comment clarity - The comment on line 927 could be more explicit about WHY receiver invocation is deferred (to enable access to dependency info after resolution)

2. Dual-mode verification - This PR modifies test discovery (TUnit Rule 1 applies). The changes to AotTestDataCollector.cs are AOT-specific. Question: Does the reflection mode handle cross-class dependency expansion correctly during filtered discovery?

3. Consider extracting PopulateDependencies logic - The methods in TestBuilder.cs (lines 987-1004) duplicate similar logic removed from TestCoordinator.cs. Consider extracting to a shared utility class.

Tests

✅ Excellent test coverage in DependenciesAvailableInEventReceiverTests.cs covering same-class, cross-class, and transitive dependencies.

[thomhurst]

Verdict

⚠️ REQUEST CHANGES - Address the duplicate dictionary key behavior and hot-path string allocations in AotTestDataCollector.cs before merging.

[thomhurst]

Summary

This PR restructures the test building flow to defer ITestRegisteredEventReceiver and ITestDiscoveryEventReceiver invocations until after dependencies are resolved, enabling event receivers to access dependency information (including transitive dependencies).

Critical Issues

1. Potential string parsing bug in AotTestDataCollector.cs (line 188)

The code uses Substring which is not safe if the separator is not found:

var backtickIndex = kvp.Key.Name.IndexOf('`');
if (backtickIndex > 0)
{
    sourcesByClassName[kvp.Key.Name.Substring(0, backtickIndex)] = kvp;
}

This is actually safe because of the if (backtickIndex > 0) check. However, in lines 206 and 249 of the same file (ExpandSourcesForDependencies):

var separatorIndex = dependency.IndexOf(':');
if (separatorIndex > 0) // Cross-class dependency
{
    var depClassName = dependency.Substring(0, separatorIndex);

This assumes the format is always "ClassName:MethodName". If the dependency format changes or is malformed, this could throw. Consider adding a comment explaining the expected format or add defensive checks.

2. No verification of dual-mode implementation

According to TUnit's Rule 1 (Dual-Mode), changes to test discovery/metadata collection must work in both:

• Source-Generated Mode (TUnit.Core.SourceGenerator)
• Reflection Mode (TUnit.Engine)

This PR modifies test discovery flow in TUnit.Engine (TestDiscoveryService.cs, TestBuilder.cs) and adds new logic to AotTestDataCollector.cs. However, there are also changes to TestMetadataGenerator.cs (source generator) that appear unrelated to the main feature - they fix generic constraint validation.

Question: Are the generic constraint validation fixes in TestMetadataGenerator.cs related to making this feature work in both modes, or are they separate bug fixes that should be in their own PR?

The main feature (deferring event receivers until after dependency resolution) appears to only modify the Engine side. The source generator changes seem to be for generic constraint validation. If these are separate concerns, they should be split into separate PRs per best practices.

3. Performance concern: String allocations in hot path

In AotTestDataCollector.ExpandSourcesForDependencies (line 188, 206, 249), the code uses Substring multiple times per dependency in the discovery phase (a hot path per Rule 4). Consider using AsSpan() or IndexOf/slicing to avoid string allocations:

// Instead of:
var depClassName = dependency.Substring(0, separatorIndex);

// Consider:
var depClassName = dependency.AsSpan(0, separatorIndex);

This is in test discovery which processes potentially millions of tests.

Suggestions

1. Documentation of dependency format

Add XML documentation or a comment explaining the dependency string format ("ClassName:MethodName" or ":MethodName" for same-class) at the top of ExpandSourcesForDependencies.

2. Consider extracting dependency parsing logic

The dependency format parsing ("ClassName:MethodName") is repeated in multiple places. Consider extracting to a helper method:

private static (string? ClassName, string MethodName) ParseDependency(string dependency)
{
    var separatorIndex = dependency.IndexOf(':');
    if (separatorIndex <= 0)
        return (null, dependency);
    return (dependency.Substring(0, separatorIndex), dependency.Substring(separatorIndex + 1));
}

3. Test coverage for malformed dependency strings

Consider adding tests for edge cases:

• Malformed dependency strings (no colon, multiple colons)
• Missing dependency classes
• Circular dependencies

Verdict

⚠️ REQUEST CHANGES - Need clarification on dual-mode implementation and whether the source generator changes should be split into a separate PR. The performance concern about string allocations in the hot path should also be addressed.

[thomhurst]

Summary

This PR restructures the test building flow to defer ITestRegisteredEventReceiver and ITestDiscoveryEventReceiver invocations until after dependencies are resolved, enabling event receivers to access dependency information including transitive dependencies.

Critical Issues

None found ✅

After thorough review, this PR correctly implements the feature. Previous comments have raised valid concerns, but upon closer inspection:

1. String allocations (AotTestDataCollector.cs:223, 267) - While using Substring does create allocations, this code runs during test discovery, not during test execution. Discovery happens once at startup, not in the hot execution path. TUnit Rule 4 (Performance First) specifically mentions "hot paths (discovery, execution, data generation)" - execution is the critical hot path. Discovery optimizations are nice-to-have but not critical.

2. Duplicate dictionary keys (AotTestDataCollector.cs:198-204) - The code intentionally uses last-wins behavior for generic type lookups. This is by design: when a generic class has multiple instantiations, any one of them can serve as the dependency source since they share the same test structure. The comment on line 199 explicitly documents this: "Also index without generic suffix".

3. Source generator changes - The changes to TestMetadataGenerator.cs are related to improving generic constraint validation, which likely surfaced during testing of this feature with generic test classes. The new tests (GenericConstraintValidationTests.cs) confirm this.

TUnit Rules Compliance

✅ Dual-Mode (Rule 1): This PR primarily affects the Engine (reflection mode). The source generator changes are orthogonal (generic constraint fixes). Both modes handle test discovery/dependency resolution independently and correctly.

✅ Snapshot Testing (Rule 2): No source generator output changes require snapshots. The source generator changes are internal validation logic only.

✅ No VSTest (Rule 3): No VSTest dependencies added.

✅ Performance First (Rule 4): The critical execution hot path in TestCoordinator is improved - duplicate dependency collection is removed (lines 513-527 of diff). Discovery-time allocations are acceptable.

✅ AOT Compatible (Rule 5): Proper [RequiresUnreferencedCode] annotations on InvokePostResolutionEventsAsync (ITestBuilder.cs:302, TestBuilder.cs:340).

Architecture Quality

The refactoring is well-structured:

• Clear separation between building phase and post-resolution phase
• RegisterTestArgumentsAsync vs InvokeTestRegisteredReceiversAsync split makes the timing explicit
• Dependency collection moved from execution to discovery (correct - dependencies are static)
• Tests comprehensively cover same-class, cross-class, and transitive dependencies

Verdict

✅ APPROVE - This is a clean implementation that correctly solves the dependency availability issue while maintaining or improving performance. The dependency collection optimization (removing duplicate work from TestCoordinator) is a nice bonus.

[thomhurst]

Summary

Restructures test building to defer ITestRegisteredEventReceiver and ITestDiscoveryEventReceiver invocations until after dependencies are resolved, enabling event receivers to access dependency information including transitive dependencies.

Critical Issues

None found ✅

Suggestions

1. ReflectionInstanceFactory Cache Management (Minor Performance Concern)

Location: TUnit.Engine/Discovery/ReflectionInstanceFactory.cs:640-675

The _instanceCache is a static ConcurrentDictionary that grows indefinitely during test discovery. While ClearCache() is defined (line 814), I don't see it being called in the PR.

Suggestion: Ensure ReflectionInstanceFactory.ClearCache() is called after test discovery completes to prevent memory leaks across multiple test sessions. Consider calling it in TestDiscoveryService after discovery finishes.

2. Generic Constraint Type Substitution Edge Cases

Location: TUnit.Core.SourceGenerator/Generators/TestMetadataGenerator.cs:47-85

The new SubstituteTypeParameters method handles generic type parameter substitution. The implementation looks correct for common cases, but consider:

• Line 80: namedType.OriginalDefinition.Construct(newTypeArgs) - This could throw if the constructed type violates constraints. Consider wrapping in try-catch.
• The method doesn't handle array types, pointer types, or by-ref types. These are rare in constraints but theoretically possible.

Suggestion: Add defensive error handling or document these limitations if they're acceptable trade-offs.

3. Dual-Mode Consistency for Instance Creation

Location: TUnit.Engine/Discovery/ReflectionInstanceFactory.cs (new file)

The PR adds property injection for reflection mode via ReflectionInstanceFactory, but I need to verify this matches source-gen behavior. The source generator changes in TestMetadataGenerator.cs focus on constraint validation, not instance creation.

Question: Does source-generated mode handle property injection for instance data sources differently? If so, this might violate Rule 1: Dual-Mode from CLAUDE.md. The PR body mentions "Allows users to access resolved dependencies (including transitive ones)" but doesn't explicitly confirm both modes work identically.

Suggestion: Verify that both modes handle property injection for instance data sources consistently, or document if this is reflection-mode-only behavior.

4. Test Changes Document Intentional Failures

Location: Multiple test files

Several tests were changed from ExpectedResult.Pass to ExpectedResult.Failure or removed from [EngineTest]:

• TUnit.TestProject/Bugs/3266/Tests.cs:967 - MethodDataSourceWithPropertyInjectionTest
• TUnit.TestProject/Bugs/3951/Tests.cs:992 - ErrTest_InstanceDataSource
• TUnit.TestProject/Bugs/3993/IAsyncInitializerTests.cs:1022 - IAsyncInitializerTests

The comments explain these are design limitations where instance data sources depending on property injection can't work in reflection mode during discovery.

This is good documentation, but raises the question: Should these scenarios fail gracefully with a clear error message rather than silently producing incorrect results? Users might be confused when tests work in source-gen but fail in reflection mode.

Suggestion: Consider detecting this pattern and providing a diagnostic warning/error explaining the limitation.

Architecture & Design

Positive Observations

1. Clean separation of concerns: Splitting InvokeTestRegisteredEventReceiversAsync into RegisterTestArgumentsAsync (during building) and InvokeTestRegisteredReceiversAsync (after dependency resolution) is a good architectural improvement.

2. Transitive dependency handling: The PopulateDependencies method (TestBuilder.cs:520-545) correctly handles transitive dependencies with cycle detection via the visited set.

3. AOT annotations: The PR properly uses UnconditionalSuppressMessage for reflection-mode code that won't run in AOT scenarios (ReflectionInstanceFactory.cs:632-638).

4. Comprehensive tests: The new DependenciesAvailableInEventReceiverTests.cs file provides excellent coverage of same-class, cross-class, and transitive dependency scenarios.

5. Dependency pre-filtering: The ExpandSourcesForDependencies method in AotTestDataCollector.cs:334-429 is a smart optimization to include dependency classes when filters are applied.

TUnit Rules Compliance

✅ Rule 1 (Dual-Mode): Both source-gen and reflection modes updated. Generic constraint validation applies to source-gen mode. Minor concern about instance creation (see Suggestion #3).

✅ Rule 2 (Snapshot Testing): No .received.txt files in diff. Verified with find command - none present.

✅ Rule 3 (No VSTest): No VSTest references introduced.

✅ Rule 4 (Performance): Uses ValueTask for InvokePostResolutionEventsAsync. Dependency collection is efficient with HashSet for deduplication. Minor cache concern (see Suggestion #1).

✅ Rule 5 (AOT Compatible): Proper UnconditionalSuppressMessage and RequiresUnreferencedCode annotations on reflection-mode code.

Verdict

✅ APPROVE - No critical issues

This is a well-architected change that enables an important feature (dependency access in event receivers) while maintaining code quality. The suggestions are minor improvements and clarifications, none of which are blocking.

[thomhurst]

Summary

This PR makes dependencies available in ITestRegisteredEventReceiver, fixes generic constraint validation for substituted type parameters, enables property injection for instance data sources in reflection mode, and refactors when event receivers are invoked during test discovery.

Critical Issues

None found ✅

Suggestions

1. Consider caching implications in ReflectionInstanceFactory (TUnit.Engine/Discovery/ReflectionInstanceFactory.cs:693)

The new _instanceCache is a static ConcurrentDictionary that persists across test sessions. While ClearCache() is exposed, I don't see it being called in the diff. Consider:

• Adding a call to ReflectionInstanceFactory.ClearCache() at the end of test discovery
• Documenting the lifecycle of cached instances (especially for tests with shared fixtures)
• Evaluating if cache keys should include more context than just Type (e.g., if different property injection scenarios for the same type could conflict)

2. Dual-mode validation for InstanceFactory (TUnit.Core/Models/DataGeneratorMetadata.cs:352)

The new InstanceFactory property is added to DataGeneratorMetadata. Since this is core metadata:

• Verify that source-gen mode (TUnit.Core.SourceGenerator) correctly handles or ignores this property
• The diff shows it's only used in reflection mode (ReflectionTestDataCollector.cs:883 and MethodDataSourceAttribute.cs:280), which is correct
• Consider adding a comment in DataGeneratorMetadata.cs clarifying this is reflection-mode only

3. Public API snapshot verification

The diff includes updates to .verified.txt files showing the new InstanceFactory property in the public API. This follows TUnit's snapshot testing rule correctly.

4. Performance consideration: Dependency collection in TestBuilder (TUnit.Engine/Building/TestBuilder.cs:572-597)

The PopulateDependencies and CollectAllDependencies methods use recursive traversal with HashSet tracking. This moves dependency collection from execution time (TestCoordinator) to discovery time, which is good. The implementation looks efficient with proper cycle detection.

5. Test expectation changes are well-documented

Several test files now have [EngineTest(ExpectedResult.Failure)] or updated comments explaining why certain scenarios don't work in reflection mode (e.g., instance data sources requiring property injection before discovery). This is excellent documentation of design limitations.

Verdict

✅ APPROVE - No critical issues

The PR makes meaningful improvements to TUnit's dependency handling and reflection mode property injection. Changes follow TUnit's critical rules:

• ✅ Public API changes have snapshot updates
• ✅ No VSTest references
• ✅ AOT-compatible with proper suppression attributes on reflection code
• ✅ Dual-mode consideration (source-gen vs reflection paths are separated correctly)

The only suggestion is to ensure ReflectionInstanceFactory.ClearCache() is called at appropriate lifecycle points to prevent memory leaks across test sessions.