fix: ensure IClassConstructor and ITestEndEventReceiver share the same instance for state management

[thomhurst]

Fixes#3939

[claude]

Claude finished @thomhurst's task —— View job

---

Pull Request Review

Reviewing PR #3941: "fix: ensure IClassConstructor and ITestEndEventReceiver share the same instance for state management"

Review Checklist

• Read repository guidelines (CLAUDE.md)
• Analyze the changed files and understand the fix
• Understand the context of issue IClassConstructor and ITestEndEventReceiver no longer use the same instance #3939
• Review code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Assess security concerns
• Review test coverage

Understanding the Issue

Issue #3939 involves a bug where IClassConstructor and ITestEndEventReceiver implementations that are meant to share state (like DI scopes) were using different instances. This breaks scenarios where:

• A ClassConstructor creates a DI scope during Create()
• The same instance should dispose the scope in OnTestEnd()
• Previously, different instances were being used, causing state loss

Code Review Analysis

✅ Excellent Fix Implementation

The Fix (TUnit.Core/Helpers/ClassConstructorHelper.cs:54-55):

// OLD: Always created new instancevarclassConstructor=(IClassConstructor)Activator.CreateInstance(classConstructorType)!;// NEW: Reuse existing instance if availablevarclassConstructor=testBuilderContext.ClassConstructor??(IClassConstructor)Activator.CreateInstance(classConstructorAttribute.ClassConstructorType)!;

Analysis: The fix correctly implements instance reuse by checking if a ClassConstructor already exists in the TestBuilderContext before creating a new one. This ensures the same instance is used across the test lifecycle.

✅ Code Quality & Best Practices

1. Modern C# Syntax: Uses null-coalescing operator (??) appropriately ✓
2. Clear Comments: Added helpful comment explaining the reuse logic ✓
3. Consistent Style: Follows TUnit coding standards from CLAUDE.md ✓
4. Proper State Management: Uses testBuilderContext.ClassConstructor for persistence ✓

✅ Test Coverage

The regression test (TUnit.TestProject/Bugs/3939/Tests.cs) is excellent:

1. Clear Test Intent: Comments explain exactly what the test verifies
2. Robust Failure Detection: Will throw with descriptive message if bug reoccurs
3. Realistic Scenario: Simulates actual DI scope management pattern
4. Self-Contained: Complete test case with setup and verification

Key Test Logic:

// In Create(): _scope = new object(); // Set state// In OnTestEnd(): if (_scope == null) throw; // Verify same instance

✅ Performance Considerations

• Performance Impact: Positive - avoids unnecessary object creation when instance can be reused
• Memory Efficiency: Improved - reduces allocations in test scenarios with shared state
• Hot Path: This is in test setup/teardown, not in the critical execution hot path

✅ Security Concerns

• No Security Issues: The change only affects instance reuse logic
• State Isolation: Properly maintains test isolation through TestBuilderContext
• AOT Compatibility: Uses existing AOT-compatible patterns with proper attributes

✅ Potential Issues Analysis

Reviewed for common issues:

• Thread Safety: ✓ Uses AsyncLocal<TestBuilderContext?> for proper async context isolation
• Resource Leaks: ✓ Test verifies cleanup in OnTestEnd()
• Breaking Changes: ✓ No API changes, only behavior fix
• Dual-Mode Compatibility: ✓ Change is in shared helper, works for both source-gen and reflection modes

🎯 Minor Suggestions

1. Consider Adding Documentation: While the code is clear, adding XML doc comments to the ClassConstructor property in TestBuilderContext would be helpful:

/// <summary>/// Gets or sets the class constructor instance. When set, this instance will be reused/// across the test lifecycle to maintain state (e.g., for DI scope management)./// </summary>internalIClassConstructor?ClassConstructor{get;set;}

🏆 Overall Assessment

This is a high-quality bug fix that:

• ✅ Correctly identifies and fixes the root cause
• ✅ Includes comprehensive regression test
• ✅ Follows TUnit development guidelines
• ✅ Has minimal performance impact (actually improves it)
• ✅ Maintains backward compatibility
• ✅ Uses modern C# patterns appropriately

Recommendation: ✅ APPROVE - This fix properly addresses the instance sharing issue while maintaining code quality standards.

[claude]

Claude finished @thomhurst's task —— View job

---

Pull Request Code Review

Reviewing PR #3941: "fix: ensure IClassConstructor and ITestEndEventReceiver share the same instance for state management"

Review Checklist

• Read repository guidelines (CLAUDE.md)
• Analyze the changed files and understand the fix
• Understand the context of issue IClassConstructor and ITestEndEventReceiver no longer use the same instance #3939
• Review code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Assess security concerns
• Review test coverage

Understanding the Issue

Issue #3939 involves a bug where IClassConstructor and ITestEndEventReceiver implementations that are meant to share state (like DI scopes) were using different instances. This breaks scenarios where:

• A ClassConstructor creates a DI scope during Create()
• The same instance should dispose the scope in OnTestEnd()
• Previously, different instances were being used, causing state loss

Code Review Analysis

✅ Excellent Fix Implementation

The Fix (TUnit.Core/Helpers/ClassConstructorHelper.cs:54-55):

// OLD: Always created new instancevarclassConstructor=(IClassConstructor)Activator.CreateInstance(classConstructorType)!;// NEW: Reuse existing instance if availablevarclassConstructor=testBuilderContext.ClassConstructor??(IClassConstructor)Activator.CreateInstance(classConstructorAttribute.ClassConstructorType)!;

Analysis: The fix correctly implements instance reuse by checking if a ClassConstructor already exists in the TestBuilderContext before creating a new one. This ensures the same instance is used across the test lifecycle.

✅ Code Quality & Best Practices

1. Modern C# Syntax: Uses null-coalescing operator (??) appropriately ✓
2. Clear Comments: Added helpful comment explaining the reuse logic ✓
3. Consistent Style: Follows TUnit coding standards from CLAUDE.md ✓
4. Proper State Management: Uses testBuilderContext.ClassConstructor for persistence ✓

✅ Test Coverage

The regression test (TUnit.TestProject/Bugs/3939/Tests.cs) is excellent:

1. Clear Test Intent: Comments explain exactly what the test verifies
2. Robust Failure Detection: Will throw with descriptive message if bug reoccurs
3. Realistic Scenario: Simulates actual DI scope management pattern
4. Self-Contained: Complete test case with setup and verification

Key Test Logic:

// In Create(): _scope = new object(); // Set state// In OnTestEnd(): if (_scope == null) throw; // Verify same instance

✅ Performance Considerations

• Performance Impact: Positive - avoids unnecessary object creation when instance can be reused
• Memory Efficiency: Improved - reduces allocations in test scenarios with shared state
• Hot Path: This is in test setup/teardown, not in the critical execution hot path

✅ Security Concerns

• No Security Issues: The change only affects instance reuse logic
• State Isolation: Properly maintains test isolation through TestBuilderContext
• AOT Compatibility: Uses existing AOT-compatible patterns with proper attributes

✅ Potential Issues Analysis

Reviewed for common issues:

• Thread Safety: ✓ Uses AsyncLocal<TestBuilderContext?> for proper async context isolation
• Resource Leaks: ✓ Test verifies cleanup in OnTestEnd()
• Breaking Changes: ✓ No API changes, only behavior fix
• Dual-Mode Compatibility: ✓ Change is in shared helper, works for both source-gen and reflection modes

🎯 Minor Suggestions

1. Consider Adding Documentation: While the code is clear, adding XML doc comments to the ClassConstructor property in TestBuilderContext would be helpful:

/// <summary>/// Gets or sets the class constructor instance. When set, this instance will be reused/// across the test lifecycle to maintain state (e.g., for DI scope management)./// </summary>internalIClassConstructor?ClassConstructor{get;set;}

🏆 Overall Assessment

This is a high-quality bug fix that:

• ✅ Correctly identifies and fixes the root cause
• ✅ Includes comprehensive regression test
• ✅ Follows TUnit development guidelines
• ✅ Has minimal performance impact (actually improves it)
• ✅ Maintains backward compatibility
• ✅ Uses modern C# patterns appropriately

Recommendation: ✅ APPROVE - This fix properly addresses the instance sharing issue while maintaining code quality standards.