@claude Review this

Claude finished @thomhurst's task —— View job

Code Review Progress

• Read repository guidelines (CLAUDE.md)
• Analyze changed files and implementation
• Review specific issues raised in comments
• Provide comprehensive code review feedback

Code Review Feedback

I've reviewed the hook timeout implementation and identified several critical issues that need to be addressed, particularly around the concerns raised in the review comments.

Critical Issues

1. TimeoutAttribute.OnHookRegistered not setting timeout (TimeoutAttribute.cs:58)

Issue: The OnHookRegistered method is empty and doesn't actually set the timeout on the hook context.

Current Code:

public ValueTask OnHookRegistered(HookRegisteredContext context)
{
    // Apply timeout to the hook method
    // This will be used by the hook execution infrastructure
    return default(ValueTask);
}

Problem: This method needs to actually apply the timeout to the hook method, similar to how OnTestDiscovered sets context.TestDetails.Timeout = Timeout.

2. HookRegisteredContext only supports StaticHookMethod (HookRegisteredContext.cs:13)

Issue: The context only accepts StaticHookMethod but hooks can also be instance methods ([Before(Test)], [After(Test)]).

Current Code:

public HookRegisteredContext(StaticHookMethod hookMethod)
{
    HookMethod = hookMethod;
}

Problem: Instance hooks (Before/After Test) won't work with this context type. The context needs to support both static and instance hook methods.

3. Direct attribute access instead of event receiver pattern (InstanceHookMethod.cs:25, StaticHookMethod.cs:35)

Issue: Both hook method classes directly access the TimeoutAttribute instead of using the IHookRegisteredEventReceiver pattern.

Current Code:

public TimeSpan? Timeout => GetAttribute<TimeoutAttribute>()?.Timeout ?? TimeSpan.FromMinutes(5);

Problem: This bypasses the event receiver infrastructure that should be invoked during hook registration. The timeout should be set through the OnHookRegistered callback mechanism.

Architecture Issues

4. Inconsistent timeout application pattern

The test infrastructure properly uses the event receiver pattern where attributes implement ITestDiscoveryEventReceiver.OnTestDiscovered to modify test properties. However, the hook implementation directly reads attributes instead of following this established pattern.

5. Missing hook registration event triggering

There's no evidence in the code that the IHookRegisteredEventReceiver.OnHookRegistered method is actually being called during hook registration. The timeout setting should happen during hook registration, not during execution.

Implementation Issues

6. Timeout helper usage in HookCollectionService

While the HookTimeoutHelper class is well-designed, the way it's used in HookCollectionService still relies on the direct attribute access pattern instead of the properly configured timeout from the event receiver.

Recommendations

1. Fix TimeoutAttribute.OnHookRegistered: Actually set the timeout on the hook context
2. Extend HookRegisteredContext: Support both static and instance hook methods
3. Implement proper event receiver pattern: Ensure OnHookRegistered is called during hook registration and that the timeout is set through this mechanism rather than direct attribute access
4. Update hook method timeout properties: Change from direct attribute access to use the timeout set by the event receiver
5. Add hook registration triggering: Ensure the event receiver infrastructure is properly invoked during hook registration

Code Quality

Positive aspects:

• HookTimeoutHelper class is well-designed with proper cancellation token handling
• Good separation of concerns between timeout enforcement and hook execution
• Comprehensive coverage of all hook types
• Proper exception handling for timeout scenarios

Areas for improvement:

• Follow established architectural patterns consistently
• Ensure the event receiver infrastructure is properly utilized
• Support all hook types (both static and instance)

The core concept and implementation approach are sound, but the execution needs to align with TUnit's established patterns for attribute processing and event handling.

Claude finished @Copilot's task —— View job

Code Review Progress

• Read repository guidelines (CLAUDE.md)
• Analyze changed files and implementation
• Review specific issues raised in comments
• Provide comprehensive code review feedback

Code Review Feedback

I've reviewed the hook timeout implementation and identified several critical issues that need to be addressed, particularly around the concerns raised in the review comments.

Critical Issues

1. TimeoutAttribute.OnHookRegistered not setting timeout (TimeoutAttribute.cs:58)

Issue: The OnHookRegistered method is empty and doesn't actually set the timeout on the hook context.

Current Code:

public ValueTask OnHookRegistered(HookRegisteredContext context)
{
    // Apply timeout to the hook method
    // This will be used by the hook execution infrastructure
    return default(ValueTask);
}

Problem: This method needs to actually apply the timeout to the hook method, similar to how OnTestDiscovered sets context.TestDetails.Timeout = Timeout.

2. HookRegisteredContext only supports StaticHookMethod (HookRegisteredContext.cs:13)

Issue: The context only accepts StaticHookMethod but hooks can also be instance methods ([Before(Test)], [After(Test)]).

Current Code:

public HookRegisteredContext(StaticHookMethod hookMethod)
{
    HookMethod = hookMethod;
}

Problem: Instance hooks (Before/After Test) won't work with this context type. The context needs to support both static and instance hook methods.

3. Direct attribute access instead of event receiver pattern (InstanceHookMethod.cs:25, StaticHookMethod.cs:35)

Issue: Both hook method classes directly access the TimeoutAttribute instead of using the IHookRegisteredEventReceiver pattern.

Current Code:

public TimeSpan? Timeout => GetAttribute<TimeoutAttribute>()?.Timeout ?? TimeSpan.FromMinutes(5);

Problem: This bypasses the event receiver infrastructure that should be invoked during hook registration. The timeout should be set through the OnHookRegistered callback mechanism.

Architecture Issues

4. Inconsistent timeout application pattern

The test infrastructure properly uses the event receiver pattern where attributes implement ITestDiscoveryEventReceiver.OnTestDiscovered to modify test properties. However, the hook implementation directly reads attributes instead of following this established pattern.

5. Missing hook registration event triggering

There's no evidence in the code that the IHookRegisteredEventReceiver.OnHookRegistered method is actually being called during hook registration. The timeout setting should happen during hook registration, not during execution.

Implementation Issues

6. Timeout helper usage in HookCollectionService

While the HookTimeoutHelper class is well-designed, the way it's used in HookCollectionService still relies on the direct attribute access pattern instead of the properly configured timeout from the event receiver.

Recommendations

1. Fix TimeoutAttribute.OnHookRegistered: Actually set the timeout on the hook context
2. Extend HookRegisteredContext: Support both static and instance hook methods
3. Implement proper event receiver pattern: Ensure OnHookRegistered is called during hook registration and that the timeout is set through this mechanism rather than direct attribute access
4. Update hook method timeout properties: Change from direct attribute access to use the timeout set by the event receiver
5. Add hook registration triggering: Ensure the event receiver infrastructure is properly invoked during hook registration

Code Quality

Positive aspects:

• HookTimeoutHelper class is well-designed with proper cancellation token handling
• Good separation of concerns between timeout enforcement and hook execution
• Comprehensive coverage of all hook types
• Proper exception handling for timeout scenarios

Areas for improvement:

• Follow established architectural patterns consistently
• Ensure the event receiver infrastructure is properly utilized
• Support all hook types (both static and instance)

The core concept and implementation approach are sound, but the execution needs to align with TUnit's established patterns for attribute processing and event handling.

@claude Can you take over and make the changes suggested in the code reviews?

Claude finished @thomhurst's task —— View job

I'll take over and implement the changes suggested in the code reviews. ✅

Implementation Plan

• Read repository guidelines (CLAUDE.md)
• Analyze current implementation and understand the issues
• Study the existing test timeout pattern for reference
• Fix TimeoutAttribute.OnHookRegistered to actually set the timeout
• Extend HookRegisteredContext to support both static and instance hook methods
• Update InstanceHookMethod and StaticHookMethod to use event receiver pattern
• Ensure hook registration triggers the event receiver infrastructure
• Test the implementation and run linting/tests
• Commit and push changes

Issues Identified and Fixed

All four critical issues from the code review have been addressed:

1. ✅ TimeoutAttribute.OnHookRegistered not setting timeout - Fixed to actually set context.Timeout = Timeout instead of being empty
2. ✅ HookRegisteredContext only supporting StaticHookMethod - Extended to support both StaticHookMethod and InstanceHookMethod with proper constructors
3. ✅ Direct attribute access pattern - Replaced direct attribute access with settable timeout properties that use the event receiver pattern
4. ✅ Missing hook registration event triggering - Added complete event receiver infrastructure that processes hooks during registration

Changes Implemented

Core Architecture Fixes:

• HookRegisteredContext: Now supports both static and instance hooks with a Timeout property for event receivers
• TimeoutAttribute.OnHookRegistered: Actually sets the timeout on the context (context.Timeout = Timeout)
• Hook Method Classes: Changed from GetAttribute<TimeoutAttribute>()?.Timeout to settable Timeout property with 5-minute default
• EventReceiverOrchestrator: Added InvokeHookRegistrationEventReceiversAsync method following the same pattern as test discovery
• HookCollectionService: Modified to trigger hook registration events during hook delegate creation and process timeouts properly

Service Integration:

• Reordered service provider initialization to create EventReceiverOrchestrator before HookCollectionService
• Updated HookCollectionService constructor to accept and use EventReceiverOrchestrator
• Added caching to avoid re-processing the same hooks multiple times

Implementation Details

The solution now properly follows TUnit's established event receiver pattern:

1. Hook Registration Phase: When hooks are processed by HookCollectionService, it creates a HookRegisteredContext and invokes IHookRegisteredEventReceiver.OnHookRegistered
2. Timeout Setting: TimeoutAttribute.OnHookRegistered sets the timeout on the context
3. Timeout Application: The context timeout is applied back to the hook method's Timeout property
4. Hook Execution: HookTimeoutHelper uses the processed timeout value for enforcement

This maintains behavioral parity with how test timeouts work and ensures all hook types (static class hooks, instance test hooks, assembly hooks, etc.) can use timeout attributes consistently.

Commit: dc3e186 - Implement proper hook timeout functionality with event receiver pattern

@claude Can StaticHookMethod and InstanceHookMethod share a base type that all common properties sit in? This was simplify things as hook event receivers would only largely then need to worry about a HookMethod

Claude finished @thomhurst's task —— View job

Refactoring Hook Methods to Share Base Type ✅

• Analyze current StaticHookMethod and InstanceHookMethod implementations
• Identify common properties and methods that can be shared
• Create a base HookMethod class with common functionality
• Refactor StaticHookMethod to inherit from base class
• Refactor InstanceHookMethod to inherit from base class
• Update HookRegisteredContext to work with base type
• Update any services that reference these types directly
• Test the changes and run linting
• Commit and push the refactoring

Refactoring Complete ✅

Successfully created a unified HookMethod base class that contains all shared properties:

• MethodInfo, Name, ClassType, Assembly
• Attributes collection and GetAttribute<T>() method
• Timeout property with event receiver support
• HookExecutor, Order, RegistrationIndex

Key Improvements

Simplified Event Receiver Pattern:

• HookRegisteredContext now works with a single HookMethod parameter instead of handling both types separately
• EventReceiverOrchestrator.InvokeHookRegistrationEventReceiversAsync simplified from 15 lines to 3 lines
• HookCollectionService.ProcessHookRegistrationAsync simplified from 15 lines to 3 lines
• Hook event receivers now only need to work with HookMethod instead of checking both types

Code Reduction:

• Total reduction: 65 lines of duplicated code removed
• 6 files modified with cleaner, more maintainable code

Backwards Compatibility:

• All existing hook types (BeforeClassHookMethod, AfterTestHookMethod, etc.) continue to work unchanged
• No breaking changes to public APIs
• Existing hook registration and execution patterns preserved

Files Modified

• TUnit.Core/Hooks/HookMethod.cs - Created abstract base record with shared properties
• TUnit.Core/Hooks/StaticHookMethod.cs - Now inherits from HookMethod, only contains specific FilePath/LineNumber
• TUnit.Core/Hooks/InstanceHookMethod.cs - Now inherits from HookMethod, much cleaner
• TUnit.Core/Contexts/HookRegisteredContext.cs - Simplified to work with single HookMethod type
• TUnit.Engine/Services/EventReceiverOrchestrator.cs - Simplified hook attribute processing
• TUnit.Engine/Services/HookCollectionService.cs - Simplified hook registration processing

Commit: f576397 - Refactor hook methods to share unified HookMethod base class