feat: prevent side effects from non-IAsyncInitializer property getters during discovery

[thomhurst]

Fixes #4049

[claude]

Code Review for PR 4054

Summary

This PR fixes issue 4049 by preventing side effects from property getters during test discovery. The fix adds a type check before accessing properties to only evaluate those that could contain IAsyncInitializer instances.

Strengths

1. Excellent Problem Diagnosis: The fix correctly identifies that the issue stems from indiscriminately calling property getters during discovery, which triggers lazy initialization in properties like WebApplicationFactory.Server.

2. Minimal, Focused Change: The fix is surgical - adding just 11 lines to filter properties by type before accessing them. This aligns with TUnit philosophy of avoiding over-engineering.

3. Comprehensive Test Coverage: The test file is exemplary with both negative and positive test cases, using threading primitives to detect side effects reliably, and includes clear documentation.

4. Clear Documentation: The inline comments provide excellent context for future maintainers.

Observations & Considerations

1. Dual-Mode Implementation - This change only affects reflection mode (runtime discovery in TUnit.Engine). The source generator path handles test discovery at compile-time and wouldn't invoke property getters at runtime. Dual-mode implementation not required for this fix.

2. Type Safety Trade-off - The fix requires users to properly type their properties as IAsyncInitializer for automatic discovery. Properties typed as object won't be discovered. Consider adding an analyzer rule to detect this pattern and suggest proper typing.

3. Performance Impact - The type check is performed on the type, not the instance, with minimal overhead. Actually improves performance by skipping unnecessary property accesses.

Potential Issues

1. Breaking Change - Users who relied on implicit discovery of loosely-typed properties will need to update their code. Document this in release notes as a breaking change and consider adding a diagnostic analyzer.

2. Test Isolation - The test uses a static counter which could be problematic if tests run in parallel. Consider resetting the counter in a hook or making it instance-based.

Recommendations

High Priority: Merge as-is - The core fix is sound and solves the immediate problem.

Medium Priority (Follow-up): Document breaking change in release notes, create analyzer rule for loosely-typed properties, add test case for generic constraints.

Checklist Review (per CLAUDE.md)

• Rule 1 (Dual-Mode): Not applicable - reflection-only runtime discovery
• Rule 2 (Snapshots): Not applicable - no source generator or public API changes
• Rule 3 (No VSTest): Not applicable
• Rule 4 (Performance): Actually improves performance
• Rule 5 (AOT/Trimming): Already has UnconditionalSuppressMessage attributes
• Code Style: Uses modern C# syntax, proper comments
• Test Coverage: Excellent coverage for both positive and negative cases

Final Verdict

Approve with minor follow-up suggestions

This is a well-crafted fix that solves the reported issue elegantly, improves performance, includes comprehensive tests, has minimal code footprint, and follows TUnit conventions. The type safety trade-off is reasonable.

Great work!

[Copilot]

Pull request overview

This PR fixes issue #4049 by preventing TUnit from calling property getters during discovery unless the property is typed as IAsyncInitializer. Previously, the reflection-based discovery would access all properties to check if their runtime value implements IAsyncInitializer, causing unintended side effects in properties like WebApplicationFactory.Server that perform initialization on access.

Key Changes:

• Added type check before property access in TraverseInitializerProperties to skip properties not typed as IAsyncInitializer
• Added comprehensive regression tests covering both the fix (preventing side effects) and ensuring nested IAsyncInitializer properties are still discovered

Reviewed changes

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

File	Description
TUnit.Core/Discovery/ObjectGraphDiscoverer.cs	Added IsAssignableFrom check at line 451 to filter properties by declared type before calling getters, preventing side effects from unrelated properties
TUnit.TestProject/Bugs/4049/PropertyGetterSideEffectTests.cs	Added regression tests with two test classes: one verifying non-IAsyncInitializer properties aren't accessed, and one ensuring properly-typed IAsyncInitializer properties are still discovered

[Copilot]

Test classes use static counters but lack the NotInParallel attribute. If these tests run concurrently, the static fields _sideEffectCount, _parentInitCount, and _childInitCount could experience race conditions, leading to flaky test failures. Add [NotInParallel] attribute to both test classes to ensure sequential execution.

[Copilot]

Test class uses static counters but lacks the NotInParallel attribute. If these tests run concurrently with other tests, the static fields _parentInitCount and _childInitCount could experience race conditions, leading to flaky test failures. Add [NotInParallel] attribute to ensure sequential execution.

Suggested change

	[EngineTest(ExpectedResult.Pass)]
	[EngineTest(ExpectedResult.Pass)]
	[NotInParallel]