test: add regression tests for ClassDataSource with SharedType.None

[thomhurst]

Fixes #3855

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review for PR #3868

Todo List:

• Read CLAUDE.md for repository guidelines
• Analyze TestBuilder.cs changes
• Review new regression test file
• Assess code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Review security concerns
• Assess test coverage
• Provide comprehensive feedback

---

Code Quality Assessment ✅

This is an excellent pull request that addresses a critical bug with a minimal, surgical fix accompanied by comprehensive regression tests.

The Fix (TestBuilder.cs)

Problem Identified: Line 295 was calling await classDataFactory() a second time instead of reusing the cached result from line 209, causing:

• Double construction of class data source objects
• Double initialization for IAsyncInitializer implementations
• Unexpected behavior for SharedType.None scenarios

Solution Applied:

• Store classDataFactory() result in classDataResult variable (line 209-210)
• Reuse classDataResult on line 296 instead of calling the factory again

This follows the Single Responsibility Principle perfectly - the factory should only be called once per data source iteration.

Regression Tests (ClassDataSourceSharedNoneRegressionTests.cs)

Excellent Test Coverage:

1. Construction counting test - Verifies objects are constructed exactly once
2. Async initialization counting test - Verifies IAsyncInitializer.InitializeAsync() called exactly once
3. Instance isolation tests - Verifies no leakage across multiple test methods

Test Quality Highlights:

• ✅ Thread-safe counters using Interlocked.Increment
• ✅ Clear test documentation explaining the regression scenario
• ✅ Proper setup/cleanup with [Before(TestSession)] hook
• ✅ Comprehensive assertions verifying both count and state
• ✅ Real-world scenarios testing both construction and initialization patterns

Potential Issues Found 🔍

Minor Issues:

1. TestBuilder.cs line 74: The comment "Initialize any deferred IAsyncInitializer objects in class data" could be more specific about when deferral occurs
2. Missing Edge Case: No test coverage for when classDataFactory() throws an exception (though this may be covered elsewhere)

Performance Considerations ⚡

• Positive Impact: The fix eliminates redundant object construction and async initialization calls
• No Performance Regression: Change is purely an optimization with no additional overhead
• Memory Efficiency: Prevents duplicate object allocation for SharedType.None scenarios

Security Assessment 🔒

• No Security Concerns: The changes are internal optimization with no external API surface
• Thread Safety: Maintained through existing patterns and test classes use proper thread-safe counters

Best Practices Compliance ✅

• ✅ Modern C# Syntax: Uses collection expressions []
• ✅ Proper Error Handling: Maintains existing exception handling patterns
• ✅ Code Style: Follows repository conventions (verified against CLAUDE.md)
• ✅ Documentation: Clear XML comments explaining the regression scenario

Architecture Alignment 🏗️

Dual-Mode Compatibility: This fix applies to the reflection mode path (TUnit.Engine). The source generation mode likely doesn't have this issue since it generates separate factory calls per usage. This maintains the required dual-mode behavioral parity.

Recommendations 💡

1. Consider adding a unit test that verifies the factory is called exactly once (currently only tested through side effects)
2. Potential follow-up: Review other similar patterns in the codebase for similar double-invocation issues

Overall Assessment 🎯

Grade: A+

This PR demonstrates:

• Clear problem identification and surgical fix
• Comprehensive regression testing with real-world scenarios
• Zero breaking changes with positive performance impact
• Excellent code documentation explaining the issue and solution

The fix is minimal, safe, and effective - exactly what you want for a critical bug fix. The regression tests provide strong confidence that this issue won't reoccur.

Recommendation: ✅ APPROVE - Ready for merge

---

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review for PR #3868

Todo List:

• Read CLAUDE.md for repository guidelines
• Analyze TestBuilder.cs changes
• Review new regression test file
• Assess code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Review security concerns
• Assess test coverage
• Provide comprehensive feedback

---

Code Quality Assessment ✅

This is an excellent pull request that addresses a critical bug with a minimal, surgical fix accompanied by comprehensive regression tests.

The Fix (TestBuilder.cs)

Problem Identified: Line 296 was calling await classDataFactory() a second time instead of reusing the cached result from line 209, causing:

• Double construction of class data source objects
• Double initialization for IAsyncInitializer implementations
• Unexpected behavior for SharedType.None scenarios

Solution Applied:

• Store classDataFactory() result in classDataResult variable (line 209-210)
• Reuse classDataResult on line 296 instead of calling the factory again

This follows the Single Responsibility Principle perfectly - the factory should only be called once per data source iteration.

Regression Tests (ClassDataSourceSharedNoneRegressionTests.cs)

Excellent Test Coverage:

1. Construction counting test - Verifies objects are constructed exactly once
2. Async initialization counting test - Verifies IAsyncInitializer.InitializeAsync() called exactly once
3. Instance isolation tests - Verifies no leakage across multiple test methods

Test Quality Highlights:

• ✅ Thread-safe counters using Interlocked.Increment
• ✅ Clear test documentation explaining the regression scenario
• ✅ Proper setup/cleanup with [Before(TestSession)] hook
• ✅ Comprehensive assertions verifying both count and state
• ✅ Real-world scenarios testing both construction and initialization patterns

Potential Issues Found 🔍

Minor Issues:

1. TestBuilder.cs line 74: The comment "Initialize any deferred IAsyncInitializer objects in class data" could be more specific about when deferral occurs
2. Missing Edge Case: No test coverage for when classDataFactory() throws an exception (though this may be covered elsewhere)

Performance Considerations ⚡

• Positive Impact: The fix eliminates redundant object construction and async initialization calls
• No Performance Regression: Change is purely an optimization with no additional overhead
• Memory Efficiency: Prevents duplicate object allocation for SharedType.None scenarios

Security Assessment 🔒

• No Security Concerns: The changes are internal optimization with no external API surface
• Thread Safety: Maintained through existing patterns and test classes use proper thread-safe counters

Best Practices Compliance ✅

• ✅ Modern C# Syntax: Uses collection expressions []
• ✅ Proper Error Handling: Maintains existing exception handling patterns
• ✅ Code Style: Follows repository conventions (verified against CLAUDE.md)
• ✅ Documentation: Clear XML comments explaining the regression scenario

Architecture Alignment 🏗️

Dual-Mode Compatibility: This fix applies to the reflection mode path (TUnit.Engine). The source generation mode likely doesn't have this issue since it generates separate factory calls per usage. This maintains the required dual-mode behavioral parity.

Recommendations 💡

1. Consider adding a unit test that verifies the factory is called exactly once (currently only tested through side effects)
2. Potential follow-up: Review other similar patterns in the codebase for similar double-invocation issues

Overall Assessment 🎯

Grade: A+

This PR demonstrates:

• Clear problem identification and surgical fix
• Comprehensive regression testing with real-world scenarios
• Zero breaking changes with positive performance impact
• Excellent code documentation explaining the issue and solution

The fix is minimal, safe, and effective - exactly what you want for a critical bug fix. The regression tests provide strong confidence that this issue won't reoccur.

Recommendation: ✅ APPROVE - Ready for merge

---