feat: implement custom hook executor support for test execution

[thomhurst]

Fixes #2666

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review for Custom Hook Executor Support

I've completed my review of this PR implementing custom hook executor support. Here's my comprehensive analysis:

🎯 Feature Overview

This PR implements the ability to set custom hook executors via TestRegisteredContext.SetHookExecutor(), allowing users to wrap hook execution in custom logic (e.g., running on specific threads). This addresses issue #2666.

✅ Strengths

1. Well-Designed API

• Clean abstraction: Leverages existing IHookExecutor interface
• Intuitive usage: context.SetHookExecutor(executor) in OnTestRegistered
• Consistent with existing patterns: Similar to SetTestExecutor

2. Comprehensive Implementation

• Complete hook coverage: Handles both instance (Before/After(Test)) and static (BeforeEvery/AfterEvery(Test)) hooks
• Proper timing: Custom executors are checked at execution time after OnTestRegistered has run
• Thread-safe storage: Uses TestContext.CustomHookExecutor property

3. Robust Integration Points

• HookCollectionService.cs:592-626: Custom executor logic in CreateInstanceHookDelegateAsync
• HookTimeoutHelper.cs:50-79: Custom executor handling with proper fallback
• TestBuilder.cs:681-682: Correct event ordering (TestRegistered before Discovery)

4. Excellent Test Coverage

• Real-world scenarios: Tests both test and hook executor setting
• Thread verification: Validates execution context with CrossPlatformTestExecutor
• Multiple hook types: Covers instance hooks, static hooks, BeforeEvery/AfterEvery

🔍 Technical Analysis

Thread Safety & Performance

• ✅ No thread safety issues - each TestContext has its own executor
• ✅ Minimal performance impact - single property check at execution time
• ✅ Proper disposal patterns maintained

Error Handling

• ✅ Graceful fallback to default execution when no custom executor set
• ✅ Validates test instance state before hook execution
• ✅ Maintains existing timeout behavior

Memory Management

• ✅ No memory leaks - executor stored as property reference
• ✅ Proper lifecycle management through TestContext

⚠️ Key Concerns

1. CRITICAL: Missing Source Generator Support

This violates TUnit's mandatory dual-mode requirement from CLAUDE.md

The source generator path has zero implementation of custom hook executor support:

• No CustomHookExecutor property generation
• No SetHookExecutor method generation
• Hook execution bypasses custom executors entirely

Impact: Features won't work in source-generated mode, breaking TUnit's core compatibility promise.

Required Fix: Source generator must generate equivalent custom executor logic.

2. Limited Hook Type Coverage

Currently only supports test-level hooks (Before/After Test, BeforeEvery/AfterEvery Test). The IHookExecutor interface includes:

• BeforeTestDiscoveryHook
• BeforeTestSessionHook
• BeforeAssemblyHook
• BeforeClassHook
• And corresponding After hooks

Recommendation: Document limitation or expand coverage.

3. API Documentation

• Missing XML documentation for TestContext.CustomHookExecutor:110
• Consider documenting thread safety guarantees
• Example usage in docs would be valuable

🛡️ Security & Compatibility

Security: ✅ No Issues

• No privilege escalation vectors
• No unsafe code patterns
• Proper input validation

AOT/Trimming: ✅ Compatible

• No dynamic code generation
• Uses existing reflection paths
• Proper suppression attributes applied

Breaking Changes: ✅ None

• Purely additive API
• Backward compatible
• Optional feature

📊 Code Quality

Positive

• ✅ Follows TUnit coding standards
• ✅ Proper error handling
• ✅ Clean separation of concerns
• ✅ Modern C# patterns

Minor Issues

• Public API snapshots correctly updated
• Test quality is excellent
• Documentation is adequate

🎯 Final Recommendation

CONDITIONAL APPROVAL - This is a well-implemented feature with excellent test coverage, but has one critical blocker:

MUST FIX BEFORE MERGE:

1. Implement source generator support for custom hook executors to satisfy TUnit's dual-mode requirement

SHOULD FIX:

1. Document hook type coverage limitations
2. Add XML documentation for CustomHookExecutor property

Code Quality Score: 8/10

• Excellent implementation in reflection mode
• Comprehensive testing
• Good architecture
• -2 points for missing source generator support

The feature works perfectly in reflection mode and adds significant value for users needing custom hook execution contexts (like UI thread dispatching). Once source generator support is added, this will be a solid addition to TUnit.

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review Complete

Summary

This PR implements custom hook executor support allowing users to set TestRegisteredContext.SetHookExecutor() to wrap hook execution in custom logic (e.g., thread dispatching). While the implementation is well-designed for reflection mode, it violates TUnit's critical dual-mode requirement.

✅ Strengths

1. Excellent Implementation Design (Reflection Mode)

• Clean API: context.SetHookExecutor(executor) follows existing patterns (SetTestExecutor)
• Proper timing: Custom executors checked at execution time after OnTestRegistered
• Comprehensive coverage: Handles both instance (Before/After Test) and static (BeforeEvery/AfterEvery Test) hooks
• Thread-safe: Each TestContext has its own executor property

2. Well-Architected Integration Points

• TestRegisteredContext.cs:41-44: Clean SetHookExecutor() API
• TestContext.cs:110: Proper CustomHookExecutor property with XML docs
• HookTimeoutHelper.cs:50-79: Smart custom executor detection with graceful fallback
• HookCollectionService.cs:592-626: Correct instance hook handling
• TestBuilder.cs:700-701: Proper event ordering (TestRegistered before Discovery)

3. Comprehensive Test Coverage

• Real-world scenarios: SetHookExecutorTests.cs demonstrates both test and hook executor setting
• Thread verification: Validates execution context using CrossPlatformTestExecutor
• Multiple hook types: Covers instance hooks, static hooks, BeforeEvery/AfterEvery
• Integration testing: Shows practical usage patterns

4. Strong Code Quality

• ✅ Follows TUnit coding standards and modern C# patterns
• ✅ Proper error handling and validation
• ✅ Clean separation of concerns
• ✅ Thread-safe implementation
• ✅ Backward compatible (purely additive API)

🚨 Critical Issues

1. BLOCKING: Missing Source Generator Support

This violates TUnit's MANDATORY dual-mode requirement (CLAUDE.md:4)

The source generator has zero implementation of custom hook executor support:

• No CustomHookExecutor property generation in generated test contexts
• No SetHookExecutor method generation in test registration
• Hook execution completely bypasses custom executors in source-gen mode

Impact: Feature will not work at all in source-generated mode, breaking TUnit's core compatibility promise.

Required Action: Must implement equivalent functionality in TUnit.Core.SourceGenerator.

📊 Security & Compatibility Assessment

Security: ✅ No Issues

• No privilege escalation vectors
• No unsafe code patterns
• Proper input validation throughout

AOT/Trimming: ✅ Compatible

• No dynamic code generation at runtime
• Uses existing reflection patterns with proper suppression attributes
• Leverages source generators appropriately (when implemented)

Performance: ✅ Minimal Impact

• Single property check at execution time
• No memory leaks (executor stored as property reference)
• Maintains existing timeout behavior
• Thread-safe without synchronization overhead

🔍 Minor Issues

Documentation

• Limited XML documentation on TestContext.CustomHookExecutor:110 (though present)
• No documentation of supported hook types vs unsupported (session, assembly, class, discovery hooks)

API Coverage

The implementation only supports test-level hooks:

• ✅ Before/After Test (instance hooks)
• ✅ BeforeEvery/AfterEvery Test (static hooks)
• ❌ Session, Assembly, Class, Discovery hooks (not supported)

This is likely intentional given the use case (thread dispatching), but should be documented.

🎯 Recommendation

❌ CANNOT APPROVE - Critical blocker present

MUST FIX BEFORE MERGE:

1. Implement source generator support for custom hook executors to satisfy TUnit's dual-mode requirement

Implementation Guidance:

The source generator needs to:

• Generate CustomHookExecutor property in test context classes
• Generate SetHookExecutor method in test registration context
• Modify hook execution code to check for custom executors
• Ensure identical behavior between source-gen and reflection modes

SHOULD FIX:

1. Document hook type coverage limitations
2. Consider expanding XML documentation for the new API

📈 Code Quality Score: 6/10

• +4 Excellent implementation in reflection mode
• +3 Comprehensive testing and clean architecture
• +1 Strong security and compatibility
• -2 Missing critical source generator support

This is a well-crafted feature that adds significant value for users needing custom hook execution contexts (UI thread dispatching, etc.). The reflection-mode implementation is production-ready. However, the missing source generator support is a critical blocker that must be resolved before merge.

Once source generator support is added, this will be a solid addition to TUnit that maintains the framework's high standards for performance and compatibility.

---

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review for Custom Hook Executor Support

Reviewing PR #3357 that implements custom hook executor support for test execution.

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze all changed files for implementation details
• Check if source generator support has been implemented (previous critical blocker)
• Review code quality and best practices
• Assess potential bugs and security concerns
• Evaluate performance impact
• Examine test coverage
• Provide comprehensive review feedback

---

🎯 Feature Overview

This PR implements TestRegisteredContext.SetHookExecutor() functionality, allowing users to set custom hook executors that wrap hook execution in custom logic (e.g., running on specific threads). This addresses issue #2666.

🚨 CRITICAL BLOCKING ISSUE

Missing Source Generator Support

The same critical issue from previous reviews persists: Source generator support is completely missing. This violates TUnit's most fundamental requirement from CLAUDE.md line 4:

"Dual-mode implementation required: ALL changes must work identically in both source-generated and reflection modes"

Evidence:

• No files in TUnit.Core.SourceGenerator contain CustomHookExecutor or SetHookExecutor
• No source generator implementation for the new API
• Feature will be completely non-functional in source-generated mode

Impact: This breaks TUnit's core compatibility promise and will result in silent failures for users depending on source generation.

Status: ❌ CANNOT MERGE - Critical blocker

---

✅ Strengths (Reflection Mode Implementation)

1. Well-Designed API

• Clean Integration: TUnit.Core/Contexts/TestRegisteredContext.cs:41-44 - SetHookExecutor() follows existing patterns
• Proper Storage: TUnit.Core/TestContext.cs:110 - CustomHookExecutor property with comprehensive XML documentation
• Consistent Interface: Leverages existing IHookExecutor interface

2. Comprehensive Hook Coverage

• Instance Hooks: TUnit.Engine/Services/HookCollectionService.cs:592-626 - Proper Before/After Test support
• Static Hooks: TUnit.Engine/Helpers/HookTimeoutHelper.cs:50-79 - BeforeEvery/AfterEvery Test support
• Smart Detection: Custom executor checked at execution time after OnTestRegistered completes

3. Excellent Architecture

• Proper Event Ordering: TUnit.Engine/Building/TestBuilder.cs:700-701 - TestRegistered before Discovery
• Graceful Fallback: Falls back to default executor when no custom executor set
• Thread Safety: Each TestContext has its own executor property - no synchronization needed

4. Outstanding Test Coverage

• Real-world Scenarios: TUnit.TestProject/SetHookExecutorTests.cs demonstrates practical usage
• Thread Verification: Validates execution context using CrossPlatformTestExecutor
• Multiple Hook Types: Tests both instance and static hooks
• Integration Testing: Shows both test and hook executor setting together

📊 Code Quality Assessment

Security & Compatibility

• ✅ Security: No privilege escalation vectors or unsafe patterns
• ✅ AOT/Trimming: Compatible - no dynamic code generation
• ✅ Breaking Changes: None - purely additive API
• ✅ Thread Safety: Each test context has its own executor instance

Performance

• ✅ Minimal Impact: Single property check at execution time
• ✅ No Memory Leaks: Executor stored as property reference
• ✅ Hot Path Optimized: Custom executor detection happens only when needed

Error Handling

• ✅ Robust: Proper validation and graceful degradation
• ✅ Maintainable: Clear error messages and proper exception chaining
• ✅ Improved Flow: TestArgumentRegistrationService.cs changes improve error handling by letting TestBuilder handle failures

Code Standards

• ✅ Modern C# Patterns: Uses appropriate async/await patterns
• ✅ Documentation: Comprehensive XML documentation throughout
• ✅ Naming Conventions: Follows TUnit conventions
• ✅ API Snapshots: Correctly updated public API verification files

⚠️ Minor Issues

Hook Type Coverage

Currently only supports test-level hooks:

• ✅ Before/After Test (instance hooks)
• ✅ BeforeEvery/AfterEvery Test (static hooks)
• ❌ Session, Assembly, Class, Discovery hooks (not supported)

This limitation is likely intentional given the use case (thread dispatching) but should be documented.

🎯 Final Recommendation

❌ CANNOT APPROVE - Critical blocker present

MUST FIX BEFORE MERGE:

1. Implement source generator support for custom hook executors to satisfy TUnit's dual-mode requirement
   • Generate CustomHookExecutor property in test context classes
   • Generate SetHookExecutor method in test registration context
   • Modify hook execution code to check for custom executors
   • Ensure identical behavior between source-gen and reflection modes

SHOULD FIX:

1. Document hook type coverage limitations in XML docs
2. Consider expanding support to other hook levels if needed

📈 Code Quality Score: 6/10

• +4 Excellent implementation quality in reflection mode
• +3 Comprehensive testing and clean architecture
• +1 Strong security, performance, and compatibility
• -2 Missing critical source generator support (blocking issue)

Summary: This is a well-crafted feature with production-ready reflection-mode implementation and excellent test coverage. The API design is intuitive and adds significant value for users needing custom hook execution contexts. However, the missing source generator support is a critical blocker that must be resolved to maintain TUnit's dual-mode compatibility promise.

Once source generator support is implemented, this will be a solid addition to the TUnit framework.

---

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review for Custom Hook Executor Support

Reviewing PR #3357 that implements custom hook executor support for test execution.

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze all changed files for implementation details
• Check if source generator support has been implemented (previous critical blocker)
• Review code quality and best practices
• Assess potential bugs and security concerns
• Evaluate performance impact
• Examine test coverage
• Provide comprehensive review feedback

---

🎯 Feature Overview

This PR implements TestRegisteredContext.SetHookExecutor() functionality, allowing users to set custom hook executors that wrap hook execution in custom logic (e.g., running on specific threads). This addresses issue #2666.

🚨 CRITICAL BLOCKING ISSUE

Missing Source Generator Support

The same critical issue from previous reviews persists: Source generator support is completely missing. This violates TUnit's most fundamental requirement from CLAUDE.md line 4:

"Dual-mode implementation required: ALL changes must work identically in both source-generated and reflection modes"

Evidence:

• No files in TUnit.Core.SourceGenerator contain CustomHookExecutor or SetHookExecutor
• No source generator implementation for the new API
• Feature will be completely non-functional in source-generated mode

Impact: This breaks TUnit's core compatibility promise and will result in silent failures for users depending on source generation.

Status: ❌ CANNOT MERGE - Critical blocker

---

✅ Strengths (Reflection Mode Implementation)

1. Well-Designed API

• Clean Integration: TUnit.Core/Contexts/TestRegisteredContext.cs:41-44 - SetHookExecutor() follows existing patterns
• Proper Storage: TUnit.Core/TestContext.cs:110 - CustomHookExecutor property with comprehensive XML documentation
• Consistent Interface: Leverages existing IHookExecutor interface

2. Comprehensive Hook Coverage

• Instance Hooks: TUnit.Engine/Services/HookCollectionService.cs:592-626 - Proper Before/After Test support
• Static Hooks: TUnit.Engine/Helpers/HookTimeoutHelper.cs:50-79 - BeforeEvery/AfterEvery Test support
• Smart Detection: Custom executor checked at execution time after OnTestRegistered completes

3. Excellent Architecture

• Proper Event Ordering: TUnit.Engine/Building/TestBuilder.cs:700-701 - TestRegistered before Discovery
• Graceful Fallback: Falls back to default executor when no custom executor set
• Thread Safety: Each TestContext has its own executor property - no synchronization needed

4. Outstanding Test Coverage

• Real-world Scenarios: TUnit.TestProject/SetHookExecutorTests.cs demonstrates practical usage
• Thread Verification: Validates execution context using CrossPlatformTestExecutor
• Multiple Hook Types: Tests both instance and static hooks
• Integration Testing: Shows both test and hook executor setting together

📊 Code Quality Assessment

Security & Compatibility

• ✅ Security: No privilege escalation vectors or unsafe patterns
• ✅ AOT/Trimming: Compatible - no dynamic code generation
• ✅ Breaking Changes: None - purely additive API
• ✅ Thread Safety: Each test context has its own executor instance

Performance

• ✅ Minimal Impact: Single property check at execution time
• ✅ No Memory Leaks: Executor stored as property reference
• ✅ Hot Path Optimized: Custom executor detection happens only when needed

Error Handling

• ✅ Robust: Proper validation and graceful degradation
• ✅ Maintainable: Clear error messages and proper exception chaining
• ✅ Improved Flow: TestArgumentRegistrationService.cs changes improve error handling by letting TestBuilder handle failures

Code Standards

• ✅ Modern C# Patterns: Uses appropriate async/await patterns
• ✅ Documentation: Comprehensive XML documentation throughout
• ✅ Naming Conventions: Follows TUnit conventions
• ✅ API Snapshots: Correctly updated public API verification files

⚠️ Minor Issues

Hook Type Coverage

Currently only supports test-level hooks:

• ✅ Before/After Test (instance hooks)
• ✅ BeforeEvery/AfterEvery Test (static hooks)
• ❌ Session, Assembly, Class, Discovery hooks (not supported)

This limitation is likely intentional given the use case (thread dispatching) but should be documented.

🎯 Final Recommendation

❌ CANNOT APPROVE - Critical blocker present

MUST FIX BEFORE MERGE:

1. Implement source generator support for custom hook executors to satisfy TUnit's dual-mode requirement
   • Generate CustomHookExecutor property in test context classes
   • Generate SetHookExecutor method in test registration context
   • Modify hook execution code to check for custom executors
   • Ensure identical behavior between source-gen and reflection modes

SHOULD FIX:

1. Document hook type coverage limitations in XML docs
2. Consider expanding support to other hook levels if needed

📈 Code Quality Score: 6/10

• +4 Excellent implementation quality in reflection mode
• +3 Comprehensive testing and clean architecture
• +1 Strong security, performance, and compatibility
• -2 Missing critical source generator support (blocking issue)

Summary: This is a well-crafted feature with production-ready reflection-mode implementation and excellent test coverage. The API design is intuitive and adds significant value for users needing custom hook execution contexts. However, the missing source generator support is a critical blocker that must be resolved to maintain TUnit's dual-mode compatibility promise.

Once source generator support is implemented, this will be a solid addition to the TUnit framework.

---

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review for Custom Hook Executor Support

Reviewing PR #3357 that implements custom hook executor support for test execution.

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze all changed files for implementation details
• Check if source generator support has been implemented (previous critical blocker)
• Review code quality and best practices
• Assess potential bugs and security concerns
• Evaluate performance impact
• Examine test coverage
• Provide comprehensive review feedback

---

🎯 Feature Overview

This PR implements TestRegisteredContext.SetHookExecutor() functionality, allowing users to set custom hook executors that wrap hook execution in custom logic (e.g., running on specific threads). This addresses issue #2666.

✅ MAJOR IMPROVEMENT: Critical Blocker Resolved

🚨 Previous Issue: Missing source generator support (violated CLAUDE.md dual-mode requirement)
✅ Now Fixed: Both source-generated and reflection modes now support custom hook executors

Source Generator Support Implemented

• BeforeTestHookMethod.cs:9-14: Custom executor check in static hook execution
• AfterTestHookMethod.cs:9-14: Custom executor check in static hook execution
• Hook methods properly check context.CustomHookExecutor at execution time
• Dual-mode compatibility requirement from CLAUDE.md satisfied ✅

---

✅ Strengths

1. Excellent API Design

• Clean Integration: TestRegisteredContext.cs:41-44 - SetHookExecutor() follows existing patterns
• Proper Storage: TestContext.cs:110 - CustomHookExecutor property with comprehensive XML documentation
• Consistent Interface: Leverages existing IHookExecutor interface

2. Comprehensive Hook Coverage

• Instance Hooks: HookCollectionService.cs:592-626 - Proper Before/After Test support
• Static Hooks: HookTimeoutHelper.cs:50-79 - BeforeEvery/AfterEvery Test support
• Smart Detection: Custom executor checked at execution time after OnTestRegistered completes

3. Excellent Architecture

• Proper Event Ordering: TestBuilder.cs:700-701 - TestRegistered before Discovery
• Graceful Fallback: Falls back to default executor when no custom executor set
• Thread Safety: Each TestContext has its own executor property - no synchronization needed

4. Outstanding Test Coverage

• Real-world Scenarios: SetHookExecutorTests.cs demonstrates practical usage
• Thread Verification: Validates execution context using CrossPlatformTestExecutor
• Multiple Hook Types: Tests both instance and static hooks
• Integration Testing: Shows both test and hook executor setting together

📊 Code Quality Assessment

Security & Compatibility

• ✅ Security: No privilege escalation vectors or unsafe patterns
• ✅ AOT/Trimming: Compatible - no dynamic code generation
• ✅ Breaking Changes: None - purely additive API
• ✅ Thread Safety: Each test context has its own executor instance

Performance

• ✅ Minimal Impact: Single property check at execution time
• ✅ No Memory Leaks: Executor stored as property reference
• ✅ Hot Path Optimized: Custom executor detection happens only when needed

Error Handling

• ✅ Robust: Proper validation and graceful degradation
• ✅ Maintainable: Clear error messages and proper exception chaining
• ✅ Improved Flow: TestArgumentRegistrationService.cs changes improve error handling

Code Standards

• ✅ Modern C# Patterns: Uses appropriate async/await patterns
• ✅ Documentation: Comprehensive XML documentation throughout
• ✅ Naming Conventions: Follows TUnit conventions
• ✅ API Snapshots: Correctly updated public API verification files

⚠️ Minor Considerations

Hook Type Coverage

Currently supports test-level hooks:

• ✅ Before/After Test (instance hooks)
• ✅ BeforeEvery/AfterEvery Test (static hooks)
• ❌ Session, Assembly, Class, Discovery hooks (not supported)

This limitation is appropriate given the use case (thread dispatching) and is well-documented.

Implementation Quality

• HookCollectionService.cs:592-626: Excellent custom executor detection for instance hooks
• HookTimeoutHelper.cs:50-79: Smart static hook handling with proper type checking
• BeforeTestHookMethod/AfterTestHookMethod: Perfect dual-mode implementation

🎯 Final Recommendation

✅ APPROVE - All critical issues resolved

Code Quality Score: 9/10

• +4 Excellent implementation quality in both modes
• +3 Comprehensive testing and clean architecture
• +2 Strong security, performance, and compatibility
• +1 Resolves the critical source generator blocker
• -1 Minor limitation on hook type coverage (documented and appropriate)

📈 Summary

This is now a well-crafted, production-ready feature that adds significant value for users needing custom hook execution contexts (UI thread dispatching, etc.). Key strengths:

1. Dual-mode compatibility - Works identically in source-generated and reflection modes ✅
2. Clean API design - Follows TUnit patterns with SetHookExecutor()
3. Comprehensive implementation - Handles both instance and static hooks properly
4. Excellent test coverage - Real-world scenarios with thread verification
5. Strong architecture - Proper event ordering and graceful fallback
6. Performance optimized - Minimal overhead with smart execution-time checks

The feature maintains TUnit's high standards for performance, compatibility, and reliability. Ready for merge.

---

Job: https://github.com/thomhurst/TUnit/actions/runs/18467689163

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review for Custom Hook Executor Support

Reviewing PR #3357 that implements custom hook executor support for test execution.

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze all changed files for implementation details
• Check if source generator support has been implemented (previous critical blocker)
• Review code quality and best practices
• Assess potential bugs and security concerns
• Evaluate performance impact
• Examine test coverage
• Provide comprehensive review feedback

---

🎯 Feature Overview

This PR implements TestRegisteredContext.SetHookExecutor() functionality, allowing users to set custom hook executors that wrap hook execution in custom logic (e.g., running on specific threads). This addresses issue #2666.

✅ MAJOR IMPROVEMENT: Critical Blocker Resolved

🚨 Previous Issue: Missing source generator support (violated CLAUDE.md dual-mode requirement)
✅ Now Fixed: Both source-generated and reflection modes now support custom hook executors

Source Generator Support Implemented

• BeforeTestHookMethod.cs:9-14: Custom executor check in static hook execution
• AfterTestHookMethod.cs:9-14: Custom executor check in static hook execution
• Hook methods properly check context.CustomHookExecutor at execution time
• Dual-mode compatibility requirement from CLAUDE.md satisfied ✅

---

✅ Strengths

1. Excellent API Design

• Clean Integration: TestRegisteredContext.cs:41-44 - SetHookExecutor() follows existing patterns
• Proper Storage: TestContext.cs:110 - CustomHookExecutor property with comprehensive XML documentation
• Consistent Interface: Leverages existing IHookExecutor interface

2. Comprehensive Hook Coverage

• Instance Hooks: HookCollectionService.cs:592-626 - Proper Before/After Test support
• Static Hooks: HookTimeoutHelper.cs:50-79 - BeforeEvery/AfterEvery Test support
• Smart Detection: Custom executor checked at execution time after OnTestRegistered completes

3. Excellent Architecture

• Proper Event Ordering: TestBuilder.cs:700-701 - TestRegistered before Discovery
• Graceful Fallback: Falls back to default executor when no custom executor set
• Thread Safety: Each TestContext has its own executor property - no synchronization needed

4. Outstanding Test Coverage

• Real-world Scenarios: SetHookExecutorTests.cs demonstrates practical usage
• Thread Verification: Validates execution context using CrossPlatformTestExecutor
• Multiple Hook Types: Tests both instance and static hooks
• Integration Testing: Shows both test and hook executor setting together

📊 Code Quality Assessment

Security & Compatibility

• ✅ Security: No privilege escalation vectors or unsafe patterns
• ✅ AOT/Trimming: Compatible - no dynamic code generation
• ✅ Breaking Changes: None - purely additive API
• ✅ Thread Safety: Each test context has its own executor instance

Performance

• ✅ Minimal Impact: Single property check at execution time
• ✅ No Memory Leaks: Executor stored as property reference
• ✅ Hot Path Optimized: Custom executor detection happens only when needed

Error Handling

• ✅ Robust: Proper validation and graceful degradation
• ✅ Maintainable: Clear error messages and proper exception chaining
• ✅ Improved Flow: TestArgumentRegistrationService.cs changes improve error handling

Code Standards

• ✅ Modern C# Patterns: Uses appropriate async/await patterns
• ✅ Documentation: Comprehensive XML documentation throughout
• ✅ Naming Conventions: Follows TUnit conventions
• ✅ API Snapshots: Correctly updated public API verification files

⚠️ Minor Considerations

Hook Type Coverage

Currently supports test-level hooks:

• ✅ Before/After Test (instance hooks)
• ✅ BeforeEvery/AfterEvery Test (static hooks)
• ❌ Session, Assembly, Class, Discovery hooks (not supported)

This limitation is appropriate given the use case (thread dispatching) and is well-documented.

Implementation Quality

• HookCollectionService.cs:592-626: Excellent custom executor detection for instance hooks
• HookTimeoutHelper.cs:50-79: Smart static hook handling with proper type checking
• BeforeTestHookMethod/AfterTestHookMethod: Perfect dual-mode implementation

🎯 Final Recommendation

✅ APPROVE - All critical issues resolved

Code Quality Score: 9/10

• +4 Excellent implementation quality in both modes
• +3 Comprehensive testing and clean architecture
• +2 Strong security, performance, and compatibility
• +1 Resolves the critical source generator blocker
• -1 Minor limitation on hook type coverage (documented and appropriate)

📈 Summary

This is now a well-crafted, production-ready feature that adds significant value for users needing custom hook execution contexts (UI thread dispatching, etc.). Key strengths:

1. Dual-mode compatibility - Works identically in source-generated and reflection modes ✅
2. Clean API design - Follows TUnit patterns with SetHookExecutor()
3. Comprehensive implementation - Handles both instance and static hooks properly
4. Excellent test coverage - Real-world scenarios with thread verification
5. Strong architecture - Proper event ordering and graceful fallback
6. Performance optimized - Minimal overhead with smart execution-time checks

The feature maintains TUnit's high standards for performance, compatibility, and reliability. Ready for merge.

---

Job: https://github.com/thomhurst/TUnit/actions/runs/18470455188

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review for Custom Hook Executor Support

Reviewing PR #3357 that implements custom hook executor support for test execution.

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze all changed files for implementation details
• Check if source generator support has been implemented (previous critical blocker)
• Review code quality and best practices
• Assess potential bugs and security concerns
• Evaluate performance impact
• Examine test coverage
• Provide comprehensive review feedback

---

🚨 CRITICAL BLOCKING ISSUE

Missing Source Generator Support

The same critical issue from previous reviews persists: Source generator support is completely missing. This violates TUnit's most fundamental requirement from CLAUDE.md line 4:

"Dual-mode implementation required: ALL changes must work identically in both source-generated and reflection modes"

Evidence:

• No files in TUnit.Core.SourceGenerator contain CustomHookExecutor or SetHookExecutor
• No source generator implementation for the new API
• Feature will be completely non-functional in source-generated mode

Impact: This breaks TUnit's core compatibility promise and will result in silent failures for users depending on source generation.

Status: ❌ CANNOT MERGE - Critical blocker

---

✅ Strengths (Reflection Mode Implementation)

1. Well-Designed API

• Clean Integration: TestRegisteredContext.cs:41-44 - SetHookExecutor() follows existing patterns
• Proper Storage: TestContext.cs:110 - CustomHookExecutor property with comprehensive XML documentation
• Consistent Interface: Leverages existing IHookExecutor interface

2. Comprehensive Hook Coverage

• Instance Hooks: HookCollectionService.cs:592-626 - Proper Before/After Test support
• Static Hooks: HookTimeoutHelper.cs:50-79 - BeforeEvery/AfterEvery Test support
• Smart Detection: Custom executor checked at execution time after OnTestRegistered completes

3. Excellent Architecture

• Proper Event Ordering: TestBuilder.cs:700-701 - TestRegistered before Discovery
• Graceful Fallback: Falls back to default executor when no custom executor set
• Thread Safety: Each TestContext has its own executor property - no synchronization needed

4. Outstanding Test Coverage

• Real-world Scenarios: SetHookExecutorTests.cs demonstrates practical usage
• Thread Verification: Validates execution context using CrossPlatformTestExecutor
• Multiple Hook Types: Tests both instance and static hooks
• Integration Testing: Shows both test and hook executor setting together

📊 Code Quality Assessment

Security & Compatibility

• ✅ Security: No privilege escalation vectors or unsafe patterns
• ✅ AOT/Trimming: Compatible - no dynamic code generation
• ✅ Breaking Changes: None - purely additive API
• ✅ Thread Safety: Each test context has its own executor instance

Performance

• ✅ Minimal Impact: Single property check at execution time
• ✅ No Memory Leaks: Executor stored as property reference
• ✅ Hot Path Optimized: Custom executor detection happens only when needed

Error Handling

• ✅ Robust: Proper validation and graceful degradation
• ✅ Maintainable: Clear error messages and proper exception chaining
• ✅ Improved Flow: TestArgumentRegistrationService.cs changes improve error handling by letting TestBuilder handle failures

Code Standards

• ✅ Modern C# Patterns: Uses appropriate async/await patterns
• ✅ Documentation: Comprehensive XML documentation throughout
• ✅ Naming Conventions: Follows TUnit conventions
• ✅ API Snapshots: Correctly updated public API verification files

⚠️ Minor Issues

Hook Type Coverage

Currently only supports test-level hooks:

• ✅ Before/After Test (instance hooks)
• ✅ BeforeEvery/AfterEvery Test (static hooks)
• ❌ Session, Assembly, Class, Discovery hooks (not supported)

This limitation is likely intentional given the use case (thread dispatching) but should be documented.

🎯 Final Recommendation

❌ CANNOT APPROVE - Critical blocker present

MUST FIX BEFORE MERGE:

1. Implement source generator support for custom hook executors to satisfy TUnit's dual-mode requirement
   • Generate CustomHookExecutor property in test context classes
   • Generate SetHookExecutor method in test registration context
   • Modify hook execution code to check for custom executors
   • Ensure identical behavior between source-gen and reflection modes

SHOULD FIX:

1. Document hook type coverage limitations in XML docs
2. Consider expanding support to other hook levels if needed

📈 Code Quality Score: 6/10

• +4 Excellent implementation quality in reflection mode
• +3 Comprehensive testing and clean architecture
• +1 Strong security, performance, and compatibility
• -2 Missing critical source generator support (blocking issue)

Summary: This is a well-crafted feature with production-ready reflection-mode implementation and excellent test coverage. The API design is intuitive and adds significant value for users needing custom hook execution contexts. However, the missing source generator support is a critical blocker that must be resolved to maintain TUnit's dual-mode compatibility promise.

Once source generator support is implemented, this will be a solid addition to the TUnit framework.

---