fix: prevent IAsyncInitializer from running during test discovery when using InstanceMethodDataSource

[thomhurst]

Fixes #3992

[claude]

Pull Request Review

Summary

This PR fixes issue #3992 by preventing IAsyncInitializer objects from being initialized during test discovery when using InstanceMethodDataSource.

Strengths

1. Correct Fix Strategy

The fix is architecturally sound. By checking 'is IAsyncDiscoveryInitializer' before calling ObjectInitializer.InitializeAsync(), it provides users explicit control over when initialization happens. This prevents expensive operations like Docker containers from running during IDE test discovery.

2. Comprehensive Test Coverage

The test file InstanceMethodDataSourceWithAsyncInitializerTests.cs is exemplary with well-documented explanations, realistic scenario simulating Docker containers, concrete assertions, and verification of shared fixture behavior.

3. Consistent Application

The fix is applied to all 5 locations in DataSourceHelpers.cs ensuring no code path is missed.

4. Clear Comments

Each change includes helpful comments explaining the rationale.

Code Quality

• Follows TUnit conventions per CLAUDE.md
• Modern C# patterns with appropriate use of 'is' pattern matching
• Dual-mode parity maintained - both reflection and source-gen modes use the same DataSourceHelpers

Analysis

Edge Cases

Since IAsyncDiscoveryInitializer extends IAsyncInitializer, the check handles mixed implementations correctly. ObjectInitializer tracks initialized objects preventing double-initialization.

Performance

Negligible performance impact from type checks, but massive gain in preventing unwanted initialization during discovery.

Consistency

I verified that TUnit.Engine/Building/TestBuilder.cs has similar logic. The fix aligns with existing patterns.

Testing

All required tests are covered. The test quality is production-grade and serves as excellent documentation.

Security and Performance

• No security concerns - actually improves security by preventing unintended resource allocation
• Positive performance impact: prevents expensive initialization during IDE refreshes and list-tests

Alignment with TUnit Philosophy

• Fast: Prevents unnecessary container startup
• Reliable: Comprehensive test, dual-mode parity maintained
• Enjoyable: Users opt-in to discovery initialization explicitly

Final Verdict

This PR is ready to merge.

Demonstrates clear understanding, elegant minimal solution, thorough testing, and alignment with TUnit conventions.

Recommendation: APPROVE AND MERGE

Great work on this fix! The test documentation is particularly well-crafted.

[Copilot]

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

[Copilot]

Critical Test Design Flaw: _testCases starts as an empty list and is only populated during InitializeAsync(). After the fix, InitializeAsync() won't be called during discovery, so Fixture.TestCases will be empty when InstanceMethodDataSource evaluates TestExecutions at line 64. This means zero test cases will be generated, and the test will never execute.

This contradicts the test's purpose - the After(Class) hook at line 88 expects to verify that exactly 3 tests ran and initialization happened once. But with empty test cases, no tests will run, and the verification will never execute.

Solution: Initialize _testCases with default data before InitializeAsync(), so test discovery can proceed:

private readonly List<string> _testCases = ["TestCase1", "TestCase2", "TestCase3"];

public Task InitializeAsync()
{
    Interlocked.Increment(ref _initializationCount);
    // Do something else to prove initialization ran (e.g., set a flag, modify data, etc.)
    IsInitialized = true;
    return Task.CompletedTask;
}

Note: If users genuinely need data populated during discovery (like the original issue #3992), they should use IAsyncDiscoveryInitializer instead of IAsyncInitializer (see Bug 3997 test for correct pattern).

[thomhurst]

@copilot reanalyse

[Copilot]

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

[claude]

Pull Request Review - Fix for Issue #3992

Thank you for this fix! This PR addresses a critical bug where IAsyncInitializer was being invoked during test discovery instead of only during test execution. Here's my detailed review:

---

✅ Strengths

1. Correct Problem Identification

The fix properly distinguishes between:

• IAsyncDiscoveryInitializer - should run during discovery (opt-in)
• IAsyncInitializer - should only run during execution (default)

This is the right design pattern and follows the framework's intent from PR #4000.

2. Comprehensive Coverage

The changes cover all the relevant code paths in DataSourceHelpers.cs:

• GetInitializedData (line 181-187)
• GetFirstInitializedValue (line 205-210, 236-241, 246-251)
• GetWrapperValue (line 599-605)

3. Excellent Test Coverage

The test file InstanceMethodDataSourceWithAsyncInitializerTests.cs is exemplary:

• Clear documentation explaining the scenario
• Tests both initialization timing and sharing semantics
• Validates that initialization happens exactly once
• Uses realistic simulation (Docker container fixture)

4. Clear Code Comments

Each conditional check includes a comment explaining why only IAsyncDiscoveryInitializer should be initialized during discovery.

---

🔍 Potential Issues & Improvements

1. Dual-Mode Implementation ⚠️ (Per CLAUDE.md Rule 1)

CRITICAL: The CLAUDE.md file mandates that ALL changes work in both execution modes:

• ✅ Reflection mode: Fixed in TUnit.Core/Helpers/DataSourceHelpers.cs
• ❓ Source-generated mode: Needs verification

Action Required: Verify that source-generated tests also respect this distinction. Check if the source generator calls DataSourceHelpers methods or has its own initialization logic that needs updating.

2. Pattern Consistency

All five locations use identical conditional logic:

// Only initialize during discovery if explicitly opted-in via IAsyncDiscoveryInitializer
// Regular IAsyncInitializer objects are initialized during test execution by ObjectLifecycleService
if (actualData is IAsyncDiscoveryInitializer)
{
    await ObjectInitializer.InitializeAsync(actualData);
}

Suggestion: Consider extracting this into a helper method to reduce duplication and make future changes easier:

private static async ValueTask InitializeIfDiscoveryInitializer(object? obj)
{
    // Only initialize during discovery if explicitly opted-in via IAsyncDiscoveryInitializer
    // Regular IAsyncInitializer objects are initialized during test execution by ObjectLifecycleService
    if (obj is IAsyncDiscoveryInitializer)
    {
        await ObjectInitializer.InitializeAsync(obj);
    }
}

Then replace all five occurrences with:

await InitializeIfDiscoveryInitializer(actualData);

This improves maintainability and ensures consistency.

3. Test Assertion Logic

In InstanceMethodDataSourceWithAsyncInitializerTests.cs:112-114:

await Assert.That(_initializationCount)
    .IsEqualTo(1)
    .Because("IAsyncInitializer should only be called once during execution, not during discovery");

Question: This assumes SharedType.PerClass behavior. What happens if someone changes the test to use SharedType.Globally or SharedType.PerTestSession? Should the test be more explicit about its assumptions or test multiple sharing scenarios?

4. Edge Case: What if data is BOTH interfaces?

public class MyData : IAsyncInitializer, IAsyncDiscoveryInitializer
{
    // Will be initialized during discovery (correct)
    // Will it be re-initialized during execution? (potential issue?)
}

The current code checks is IAsyncDiscoveryInitializer, which will be true if it implements both interfaces. According to ObjectInitializer.cs:38-46, it uses ConditionalWeakTable to track initialized objects, so double-initialization is prevented.

Status: ✅ Handled correctly by ObjectInitializer

However, the comment in IAsyncDiscoveryInitializer.cs:22-26 states:

The framework guarantees exactly-once initialization semantics - objects will not be re-initialized during test execution.

Suggestion: Add a test case verifying this edge case behavior to ensure it's documented and protected.

---

🎯 Code Quality Assessment

Aspect	Rating	Notes
Correctness	✅ Excellent	Fixes the reported issue
Code Style	✅ Good	Follows C# conventions, clear comments
Test Coverage	✅ Excellent	Comprehensive test with good documentation
Performance	✅ Good	No performance regression
Security	✅ N/A	No security concerns
Dual-Mode	⚠️ Needs Verification	Reflection mode ✅, Source-gen mode ❓

---

📋 Pre-Commit Checklist (from CLAUDE.md)

Based on the repository's guidelines:

• ✅ Code follows modern C# style
• ✅ Tests added for the fix
• ⚠️ Dual-mode implementation: Needs verification for source-generated mode
• ❓ Snapshot tests: If source generator output changed, did you run TUnit.Core.SourceGenerator.Tests and accept snapshots?
• ❓ Public API: Did this change any public APIs? If so, run TUnit.PublicAPI tests

---

🎓 Recommendations

Must Do:

1. Verify source-generated mode handles this correctly (or document why it's not needed)
2. Run snapshot tests if applicable

Should Do:

1. Consider extracting the repeated conditional logic into a helper method
2. Add edge case test for objects implementing both IAsyncInitializer and IAsyncDiscoveryInitializer

Nice to Have:

1. Add test coverage for different SharedType scenarios
2. Add integration test that verifies the fix with actual --list-tests command

---

🏁 Conclusion

This is a high-quality fix that correctly addresses issue #3992. The code is clean, well-documented, and includes excellent test coverage.

Main Action Items:

1. Verify dual-mode implementation (source-generated tests)
2. Consider the refactoring suggestions to reduce code duplication
3. Ensure snapshot tests pass if applicable

Great work on the detailed test documentation - it makes the intent and behavior crystal clear! 🎉

[Copilot]

@thomhurst I've opened a new pull request, #4003, to work on those changes. Once the pull request is ready, I'll request review from you.