Enable IClassConstructor instances to subscribe to test events

[thomhurst]

Fixes #3077

[claude]

Claude finished @thomhurst's task —— View job

---

PR Review: Enable IClassConstructor instances to subscribe to test events

Reviewing this pull request that fixes issue #3077. This appears to be a significant refactoring with 150 additions and 229 deletions across 24 files.

Review Checklist:

• Understand the issue being fixed by reading test cases
• Review core implementation changes
• Analyze code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Assess security concerns
• Review test coverage
• Provide comprehensive feedback

---

🎯 Summary

This PR enables IClassConstructor instances to subscribe to test events by making them available in the event system. The key change is exposing the class constructor through the TestContext so it can participate in the event lifecycle (e.g., ITestStartEventReceiver, ITestEndEventReceiver).

✅ Positive Aspects

1. Clear Problem Resolution: The test case in TUnit.TestProject/Bugs/3077/Tests.cs:7-42 clearly demonstrates the issue and validates the fix - an IClassConstructor that also implements event receivers should be able to initialize test data.

2. Good Architectural Changes:

   • Moving from individual parameters to TestBuilderContext in ClassConstructorHelper.TryCreateInstanceWithClassConstructor (TUnit.Core/Helpers/ClassConstructorHelper.cs:39-68) is cleaner
   • Exposing ClassConstructor through TestContext (TUnit.Core/TestContext.cs:148) enables event participation
   • Making it available in GetEligibleEventObjects (TUnit.Engine/Extensions/TestContextExtensions.cs:9) integrates it properly

3. Code Quality Improvements: Several modern C# pattern improvements throughout:

   • TUnit.Core.SourceGenerator/Generators/TestMetadataGenerator.cs:982 - Using property patterns
   • Multiple files showing better null checks and pattern matching

4. Proper Cleanup: Removal of ResettableLazy.cs and ClassConstructorExtensions.cs suggests elimination of unused/redundant code.

⚠️ Areas of Concern

Performance Considerations

1. Event System Overhead: Adding class constructors to GetEligibleEventObjects means they'll be checked for every event type. This is efficient only if the class constructor actually implements event interfaces.

2. Potential Memory Retention: Class constructors are now held in TestBuilderContext.ClassConstructor and accessible through TestContext, which could extend their lifetime beyond what's necessary.

Potential Issues

1. Missing Event Registration Logic: In TUnit.Core/Helpers/ClassConstructorHelper.cs:61, the class constructor is stored but there's no explicit event registration. The system relies on GetEligibleEventObjects discovery, which may not be obvious to maintainers.

2. Constructor Lifecycle Clarity: It's unclear when/how the class constructor gets disposed. The TestBuilderContext holds a reference, but disposal logic isn't evident in the reviewed changes.

3. Behavioral Parity Risk: The architectural changes around TestBuilderContext need verification in both source-generated and reflection modes to ensure identical behavior.

Minor Code Issues

1. TUnit.Core/GenericTestMetadata.cs:66: The parameter simplification is good, but ensure the testContext parameter contains all necessary data that was previously passed individually.

2. Pattern Consistency: Some files use modern patterns while others don't. Consider applying pattern matching updates consistently across the codebase.

🔒 Security Assessment

No security concerns identified. The changes don't introduce new attack vectors or expose sensitive data.

🧪 Test Coverage Assessment

Good: The bug fix includes a specific test case that validates the functionality.
Missing: Consider adding tests for:

• Class constructors that implement multiple event interfaces
• Event ordering when class constructors are involved
• Disposal/cleanup scenarios
• Both source-generated and reflection mode behaviors

📋 Recommendations

1. Performance: Consider optimizing event object filtering to skip class constructors that don't implement event interfaces.

2. Documentation: Add XML documentation to TestBuilderContext.ClassConstructor explaining its role in event subscription.

3. Testing: Run the snapshot tests as mentioned in CLAUDE.md:

   dotnet test TUnit.Core.SourceGenerator.Tests
   dotnet test TUnit.PublicAPI

4. Lifecycle Management: Clarify and document the class constructor disposal pattern.

🎯 Overall Assessment

This is a well-executed fix that addresses the specific issue while improving code quality. The architectural changes are sound, and the test case validates the functionality. The main considerations are performance optimization and ensuring behavioral parity across execution modes.

Recommendation: ✅ Approve with minor suggestions - The changes are solid and the issue is properly resolved.

---

[claude]

Claude finished @thomhurst's task —— View job

---

PR Review: Enable IClassConstructor instances to subscribe to test events

Reviewing this pull request that fixes issue #3077. This appears to be a significant refactoring with 190 additions and 445 deletions across 28 files.

Review Checklist:

• Read repository guidelines (CLAUDE.md)
• Understand the issue being fixed by examining test cases
• Review core implementation changes
• Analyze code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Assess security concerns
• Review test coverage
• Provide comprehensive feedback

---

🎯 Summary

This PR enables IClassConstructor instances to subscribe to test events by making them available in the event system. The key change is exposing the class constructor through the TestContext so it can participate in the event lifecycle (e.g., ITestStartEventReceiver, ITestEndEventReceiver).

✅ Positive Aspects

1. Clear Problem Resolution: The test case in TUnit.TestProject/Bugs/3077/Tests.cs:7-42 clearly demonstrates the issue and validates the fix - an IClassConstructor that also implements event receivers should be able to initialize test data.

2. Good Architectural Changes:

   • Moving from individual parameters to TestBuilderContext in ClassConstructorHelper.TryCreateInstanceWithClassConstructor (TUnit.Core/Helpers/ClassConstructorHelper.cs:39-68) is cleaner
   • Exposing ClassConstructor through TestContext (TUnit.Core/TestContext.cs:148) enables event participation
   • Making it available in GetEligibleEventObjects (TUnit.Engine/Extensions/TestContextExtensions.cs:9) integrates it properly

3. Code Quality Improvements: Several modern C# pattern improvements throughout:

   • TUnit.Core.SourceGenerator/Generators/TestMetadataGenerator.cs:982 - Using property patterns
   • Multiple files showing better null checks and pattern matching

4. Proper Cleanup: Removal of ResettableLazy.cs and ClassConstructorExtensions.cs suggests elimination of unused/redundant code.

⚠️ Areas of Concern

Performance Considerations

1. Event System Overhead: Adding class constructors to GetEligibleEventObjects means they'll be checked for every event type. This is efficient only if the class constructor actually implements event interfaces.

2. Potential Memory Retention: Class constructors are now held in TestBuilderContext.ClassConstructor and accessible through TestContext, which could extend their lifetime beyond what's necessary.

Potential Issues

1. Missing Event Registration Logic: In TUnit.Core/Helpers/ClassConstructorHelper.cs:61, the class constructor is stored but there's no explicit event registration. The system relies on GetEligibleEventObjects discovery, which may not be obvious to maintainers.

2. Constructor Lifecycle Clarity: It's unclear when/how the class constructor gets disposed. The TestBuilderContext holds a reference, but disposal logic isn't evident in the reviewed changes.

3. Behavioral Parity Risk: The architectural changes around TestBuilderContext need verification in both source-generated and reflection modes to ensure identical behavior.

Minor Code Issues

1. TUnit.Core/GenericTestMetadata.cs:66: The parameter simplification is good, but ensure the testContext parameter contains all necessary data that was previously passed individually.

2. Pattern Consistency: Some files use modern patterns while others don't. Consider applying pattern matching updates consistently across the codebase.

🔒 Security Assessment

No security concerns identified. The changes don't introduce new attack vectors or expose sensitive data.

🧪 Test Coverage Assessment

Good: The bug fix includes a specific test case that validates the functionality.
Missing: Consider adding tests for:

• Class constructors that implement multiple event interfaces
• Event ordering when class constructors are involved
• Disposal/cleanup scenarios
• Both source-generated and reflection mode behaviors

📋 Recommendations

1. Performance: Consider optimizing event object filtering to skip class constructors that don't implement event interfaces.

2. Documentation: Add XML documentation to TestBuilderContext.ClassConstructor explaining its role in event subscription.

3. Testing: Run the snapshot tests as mentioned in CLAUDE.md:

   dotnet test TUnit.Core.SourceGenerator.Tests
   dotnet test TUnit.PublicAPI

4. Lifecycle Management: Clarify and document the class constructor disposal pattern.

🎯 Overall Assessment

This is a well-executed fix that addresses the specific issue while improving code quality. The architectural changes are sound, and the test case validates the functionality. The main considerations are performance optimization and ensuring behavioral parity across execution modes.

Recommendation: ✅ Approve with minor suggestions - The changes are solid and the issue is properly resolved.

---