Skip to content

Conversation

@thomhurst
Copy link
Owner

No description provided.

Copilot AI review requested due to automatic review settings December 6, 2025 15:26
@claude
Copy link
Contributor

claude bot commented Dec 6, 2025

Pull Request Review: IAsyncDiscoveryInitializer Feature

Thank you for this PR! I've reviewed the implementation and have the following feedback:

Strengths

  1. Well-documented interface: The XML documentation clearly explains the use case and relationship to IAsyncInitializer.

  2. Correct deduplication: The implementation properly uses ObjectInitializer to prevent double-initialization during discovery and execution phases.

  3. Good test coverage: The regression test in TUnit.TestProject/Bugs/3997/Tests.cs demonstrates the feature and validates the behavior.

  4. Snapshots updated: Public API snapshots have been properly updated for all target frameworks.

  5. Clean separation of concerns: InitializeDiscoveryObjectsAsync is separate from InitializeDeferredClassDataAsync, making the distinction clear.


🔍 Critical Issue: Dual-Mode Implementation

According to CLAUDE.md Rule 1 (Mandatory), ALL changes must work identically in both execution modes:

  • Reflection mode: Implemented in TUnit.Engine/Building/TestBuilder.cs
  • Source-generated mode: NOT implemented in TUnit.Core.SourceGenerator

Impact: This feature will only work in reflection mode. Users relying on source-generated tests will not have access to IAsyncDiscoveryInitializer.

Required Action:

  1. Implement discovery initialization in the source generator
  2. Generate code that calls initialization before method data sources are evaluated
  3. Add snapshot tests to TUnit.Core.SourceGenerator.Tests to verify the generated code

📋 Additional Observations

1. Performance Consideration

The loop in InitializeDiscoveryObjectsAsync is called during test discovery, which is a hot path per CLAUDE.md Rule 4. The current implementation is fine (simple loop, no LINQ), but the pattern is correct.

2. Code Style

  • ✅ Modern C# syntax used correctly
  • ✅ Proper async patterns with Task (not blocking)
  • ✅ Clear naming conventions
  • ✅ File-scoped namespaces

3. Test Design

The test in Bugs/3997/Tests.cs is well-designed:

  • ✅ Simulates a realistic scenario (Docker container initialization)
  • ✅ Verifies initialization happens exactly once
  • ✅ Tests that data is accessible to InstanceMethodDataSource

Minor suggestion: Consider adding a test that verifies the object is NOT initialized during discovery if it only implements IAsyncInitializer (without IAsyncDiscoveryInitializer). This would validate the distinction between the two interfaces.


🎯 Recommendations

Before merging:

  1. CRITICAL: Implement source-generated mode support

    • Check TUnit.Core.SourceGenerator/Generators/TestMetadataGenerator.cs
    • Look at how other initialization is handled in generated code
    • Add snapshot tests for the generated code
  2. Run the Pre-Commit Checklist from CLAUDE.md

  3. Verify dual-mode parity:

    • Run the test in both source-gen and reflection modes
    • Ensure identical behavior

📊 Overall Assessment

Quality: High ✅
Completeness: Partial ⚠️ (missing source-gen mode)
Recommendation: Request changes - implement dual-mode support before merging

The reflection-mode implementation is excellent, but the missing source-generated mode support makes this feature incomplete per TUnit's architecture requirements. Once dual-mode support is added, this will be a valuable addition to TUnit!

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a new IAsyncDiscoveryInitializer interface that enables asynchronous initialization of data sources during test discovery, allowing fixtures like Docker containers or database connections to be initialized before test case enumeration.

Key Changes:

  • New IAsyncDiscoveryInitializer interface extending IAsyncInitializer for discovery-time initialization
  • Engine support in TestBuilder.cs to initialize discovery initializers before evaluating method data sources
  • Integration test demonstrating the feature with a simulated container scenario

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

File Description
TUnit.Core/Interfaces/IAsyncDiscoveryInitializer.cs New marker interface extending IAsyncInitializer with comprehensive documentation explaining discovery-time vs execution-time initialization
TUnit.Engine/Building/TestBuilder.cs Adds InitializeDiscoveryObjectsAsync method called during test building to initialize IAsyncDiscoveryInitializer objects before method data sources are evaluated
TUnit.TestProject/Bugs/3997/Tests.cs Integration test with SimulatedContainer demonstrating that discovery initializers are called before InstanceMethodDataSource accesses container data
TUnit.PublicAPI/Tests.Core_Library_Has_No_API_Changes.*.verified.txt Updated public API snapshots for all .NET target frameworks (.NET 4.7, 8.0, 9.0, 10.0) reflecting the new interface addition

Comment on lines 81 to 90
foreach (var data in classData)
{
if (data is IAsyncDiscoveryInitializer && data is not IDataSourceAttribute)
{
// Uses ObjectInitializer which handles deduplication.
// This also prevents double-init during execution since ObjectInitializer
// tracks initialized objects.
await ObjectInitializer.InitializeAsync(data);
}
}
Copy link

Copilot AI Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copilot uses AI. Check for mistakes.
@claude
Copy link
Contributor

claude bot commented Dec 6, 2025

Pull Request Review

Thank you for implementing IAsyncDiscoveryInitializer! This is a well-documented feature that solves a real problem. However, I have identified a critical issue that must be addressed before merging.


CRITICAL ISSUE: Dual-Mode Implementation Violation

Problem: This PR only implements the feature in reflection mode (TUnit.Engine) but is missing the source-generated mode implementation (TUnit.Core.SourceGenerator).

According to CLAUDE.md Rule 1:

  • REQUIREMENT: ALL changes must work identically in both execution modes
  • If you implement only ONE mode, the feature is INCOMPLETE and MUST NOT be committed

Evidence:

  • Reflection mode: Implemented in TUnit.Engine/Building/TestBuilder.cs:74-91
  • Source-gen mode: No implementation found in TUnit.Core.SourceGenerator

Impact:

Users relying on source-generated mode will not get IAsyncDiscoveryInitializer initialization during discovery, causing:

  • InstanceMethodDataSource to access uninitialized objects
  • Runtime exceptions when properties/methods expect initialized state
  • Inconsistent behavior between execution modes

Required Action:

  1. Implement IAsyncDiscoveryInitializer support in the source generator
  2. Generate code that calls initialization before evaluating data sources
  3. Add tests that verify both modes work identically

Positive Aspects

Excellent Documentation

  • Interface has comprehensive XML docs with use cases (TUnit.Core/Interfaces/IAsyncDiscoveryInitializer.cs:3-27)
  • Clear explanations of when to use vs IAsyncInitializer
  • Implementation comments explain the why not just the what

Good Test Coverage

  • Regression test in TUnit.TestProject/Bugs/3997/Tests.cs validates the scenario
  • Test includes helpful comments and simulates real-world usage
  • Snapshot tests updated correctly for all target frameworks

Clean Implementation

  • Leverages existing ObjectInitializer to prevent double-initialization
  • Multiple call sites properly covered
  • Code follows modern C# patterns

Additional Recommendations

1. Performance (Minor)

The InitializeDiscoveryObjectsAsync method iterates through all class data and type-checks each object. This is in a hot path. While impact is likely minimal, consider profiling with large test suites.

2. Test Coverage

Consider adding:

  • Test verifying no re-initialization during test execution
  • Negative test cases for initialization failures
  • Edge cases for nested data sources

3. Security & Safety

  • No security concerns identified
  • Proper async/await patterns used
  • AOT-compatible

Verdict

Status: Requires Changes (Critical: Missing dual-mode implementation)

The reflection mode implementation is excellent, but without source-gen mode parity, this feature is incomplete per TUnit architecture requirements. Once source-gen support is added and tested, this will be a valuable addition.

Great work on documentation and test coverage! The implementation quality is high - it just needs to be complete across both execution modes.


Reviewed by: Claude Code (Sonnet 4.5)
Guidelines: TUnit CLAUDE.md v2.0

This was referenced Dec 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants