-
-
Notifications
You must be signed in to change notification settings - Fork 108
Fix TimeoutAttribute not being applied to non-test hooks #4115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The TimeoutAttribute on [Before(Assembly)], [Before(Class)], [Before(TestSession)], and [Before(TestDiscovery)] hooks was being ignored because ProcessHookRegistrationAsync was not being called for these hooks. This commit: - Adds async versions of hook delegate creation methods that call ProcessHookRegistrationAsync - Updates InitializeAsync to await the async hook building methods - Converts CollectBeforeAssemblyHooksAsync, CollectAfterAssemblyHooksAsync, CollectBeforeClassHooksAsync, and CollectAfterClassHooksAsync to properly process hook registration events - Adds HookTimeoutTests to verify the fix works for class and assembly hooks Co-authored-by: thomhurst <[email protected]>
Co-authored-by: thomhurst <[email protected]>
4dfaece to
61eede3
Compare
SummaryThis PR fixes a bug where the Critical IssuesNone found ✅ AnalysisRoot CauseThe bug occurred because SolutionThe PR correctly implements the fix by:
Dual-Mode Compliance (Rule 1) ✅The changes only affect TUnit.Engine (reflection mode). Source-generated mode works differently but produces the same runtime behavior:
Both modes converge at the same runtime path in Test Coverage ✅The test class
Performance ConsiderationsThe conversion from synchronous to async delegate creation is appropriate here because:
Verdict✅ APPROVE - No critical issues The fix correctly addresses the root cause, maintains dual-mode compatibility, includes proper test coverage, and follows TUnit's architectural patterns. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a bug where the [Timeout] attribute was being ignored on non-test hooks ([Before(Assembly)], [Before(Class)], [Before(TestSession)], and [Before(TestDiscovery)]), causing them to always use the default 5-minute timeout regardless of the specified value.
Key Changes:
- Added async delegate creation methods in the reflection engine to invoke
ProcessHookRegistrationAsync, which triggersTimeoutAttribute.OnHookRegistered - Updated all hook building methods to use the new async creators instead of synchronous ones
- Added integration and unit tests to verify timeout behavior for class and assembly hooks
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| TUnit.Engine/Services/HookCollectionService.cs | Added async delegate creation methods (CreateClassHookDelegateAsync, CreateAssemblyHookDelegateAsync, etc.) that call ProcessHookRegistrationAsync to ensure timeout attributes are processed; updated all hook collection methods to await these async creators |
| TUnit.TestProject/HookTimeoutTests.cs | Added integration tests with class and assembly hooks that have timeouts to verify the fix works in both timeout success and failure scenarios |
| TUnit.Engine.Tests/HookTimeoutTests.cs | Added unit tests that verify hook timeout behavior across both source-generated and reflection execution modes |
| public class HookTimeoutTests | ||
| { | ||
| /// <summary> | ||
| /// A 100ms timeout on the hook - it should fail because the hook takes 500ms | ||
| /// </summary> | ||
| [Test] | ||
| public void Test_WithTimeoutHook() | ||
| { | ||
| // This test exists to verify that the hook timeout was applied correctly | ||
| } | ||
| } |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test class HookTimeoutTests contains a test method but has no hooks defined. The test Test_WithTimeoutHook doesn't verify anything meaningful since there are no hooks with timeouts in this class. Either:
- Add a test-level hook with a timeout to this class (e.g.,
[Timeout(100)] [Before(Test)]), or - Remove this class entirely since the other test classes (
ClassHookTimeoutTestsandAssemblyHookTimeoutPassTests) already provide coverage for hook timeouts.
The comment on line 10 suggests this test should verify hook timeout behavior, but without an actual hook, it cannot fulfill that purpose.
[Timeout]attribute on[Before(Assembly)],[Before(Class)],[Before(TestSession)], and[Before(TestDiscovery)]hooks was ignored—always using the default 5-minute timeout regardless of the specified value.Root Cause
ProcessHookRegistrationAsync(which invokesTimeoutAttribute.OnHookRegistered) was only called for test hooks. The delegate creation methods for class/assembly/session/discovery hooks were synchronous and bypassed hook registration entirely.Changes
CreateClassHookDelegateAsync,CreateAssemblyHookDelegateAsync, etc.) that callProcessHookRegistrationAsyncCollectBefore/AfterClassHooksAsyncandCollectBefore/AfterAssemblyHooksAsyncto properly process hook registration eventsVerification
Both execution modes (Source Generated and Reflection) now respect hook timeouts:
Original prompt
This section details on the original issue you should resolve
<issue_title>[Bug]: TimeoutAttribute on [Before(Assembly)] hook is ignored - default 5 minute timeout always used</issue_title>
<issue_description>### Description
The
[Timeout]attribute placed on a[Before(Assembly)]hook method is not being applied. The hook always uses the default 5-minute timeout (300,000ms) defined inHookMethod.cs, regardless of the timeout value specified in the attribute.Expected Behavior
The hook should use the 15-minute timeout specified in the [Timeout(900_000)] attribute.
Actual Behavior
The hook fails with:
TUnit.Engine.Exceptions.TestFailedException: BeforeAssemblyException: BeforeAssembly hook failed:
Hook 'IntegrationTestsBase.SetUp()' exceeded timeout of 300000ms
The timeout is always 300,000ms (5 minutes) - the default value from HookMethod.Timeout.
Steps to Reproduce
[Before(Assembly)]hook that takes longer than 5 minutes on slow CI/CD environments[Timeout(900_000)]attribute to increase timeout to 15 minutesTUnit Version
1.5.80
.NET Version
.NET 10.0
Operating System
Linux
IDE / Test Runner
dotnet CLI (dotnet test / dotnet run)
Error Output / Stack Tra...
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.