Conversation
…cuted The HookExecutor was missing invocation of BeforeEvery(Class), AfterEvery(Class), BeforeEvery(Assembly), and AfterEvery(Assembly) hooks. While the Test-level hooks had the correct pattern of executing both BeforeEvery and Before hooks, the same pattern was missing for Class and Assembly levels. Updated ExecuteBeforeClassHooksAsync, ExecuteAfterClassHooksAsync, ExecuteBeforeAssemblyHooksAsync, and ExecuteAfterAssemblyHooksAsync to also collect and execute the corresponding BeforeEvery/AfterEvery hooks, following the same pattern already used for Test-level hooks. Co-authored-by: thomhurst <30480171+thomhurst@users.noreply.github.com> Agent-Logs-Url: https://github.com/thomhurst/TUnit/sessions/89836764-b81e-472f-9f00-c6e7e61c02ca
|
@copilot we need tests to ensure this never regresses |
Test targets in TUnit.TestProject: - BeforeEveryClassTests: [BeforeEvery(Class)] hook sets StateBag flag, test verifies - BeforeEveryAssemblyTests: [BeforeEvery(Assembly)] hook sets StateBag flag, test verifies - AfterEveryClassTests: [AfterEvery(Class)] hook writes file as proof of execution - AfterEveryAssemblyTests: [AfterEvery(Assembly)] hook writes file as proof of execution Integration test runners in TUnit.Engine.Tests: - BeforeEveryClassHookTests: Runs filtered tests, asserts 1 passed / 0 failed - BeforeEveryAssemblyHookTests: Same pattern for assembly-level hooks - AfterEveryClassHookTests: Validates test passes and hook file was created - AfterEveryAssemblyHookTests: Validates test passes and hook file was created Co-authored-by: thomhurst <30480171+thomhurst@users.noreply.github.com> Agent-Logs-Url: https://github.com/thomhurst/TUnit/sessions/3e2fe83b-5e07-41db-9d58-975349f40a31
Added 8 regression test files in TUnit.TestProject (test targets):
TUnit.Engine.Tests (integration test runners):
These follow the same patterns as the existing |
There was a problem hiding this comment.
Review: Fix BeforeEvery/AfterEvery hooks for Class and Assembly
The fix is correct and follows the existing pattern established for ExecuteBeforeTestHooksAsync / ExecuteAfterTestHooksAsync. The root cause was clearly identified — missing CollectBeforeEvery* / CollectAfterEvery* call sites — and the regression tests adequately cover the previously broken paths.
Primary Concern: Code Duplication in HookExecutor.cs
The main architectural issue is that this PR copies the same 12-line try/catch block four times across ExecuteBeforeClassHooksAsync and ExecuteBeforeAssemblyHooksAsync:
catch (Exception ex)
{
if (ex is SkipTestException) { throw; }
if (ex.InnerException is SkipTestException skipEx)
{
ExceptionDispatchInfo.Capture(skipEx).Throw();
}
throw new BeforeClassException($"... hook failed: {ex.Message}", ex);
}This pattern already existed once in the BeforeTest variant and is now duplicated three more times. The concern is maintainability: any future change to this pattern (e.g., adding a new exception type, changing wrapping behaviour) must be made in all four places. A private helper method would consolidate this:
private static async ValueTask ExecuteBeforeHookWithSkipHandling<TException>(
Func<ValueTask> execute,
Func<string, Exception, TException> wrapException)
where TException : Exception
{
try { await execute().ConfigureAwait(false); }
catch (Exception ex)
{
if (ex is SkipTestException) throw;
if (ex.InnerException is SkipTestException skipEx)
ExceptionDispatchInfo.Capture(skipEx).Throw();
throw wrapException($"hook failed: {ex.Message}", ex);
}
}This isn't blocking — the current duplication was already present for the Test variant before this PR — but it's worth addressing either here or as a follow-up to prevent the pattern from spreading further.
Minor Observations
Test isolation for BeforeEveryAssembly: The BeforeEveryAssemblyHooks iterates context.AllTests (all tests in the assembly) and filters by exact test name. This works today but is brittle if the test is renamed without updating the hook. A better design in tests is to scope the filter by class name or use a dedicated attribute/tag rather than string-matching test names.
AfterEvery file-system side-channel: Using File.WriteAllTextAsync with a GUID filename is an acceptable trade-off for verifying AfterEvery execution (since the hook runs after the test's assertion window closes). It would be worth noting that these files are never cleaned up — if this is a concern in CI environments, a cleanup step may be needed.
Removal of the empty-collection early return in ExecuteAfterClassHooksAsync / ExecuteAfterAssemblyHooksAsync: The original code returned early with FinishClassActivity(hasErrors: false) when the hooks collection was empty. The new code doesn't have this early return, but FinishClassActivity / FinishAssemblyActivity is still called at the bottom unconditionally — so this is fine and actually cleaner.
Summary
The bug fix is correct and well-tested. The main suggestion is to extract the repeated SkipTestException-handling try/catch into a helper to prevent further duplication, ideally as a follow-up since this pattern predates the PR.
Updated [TUnit](https://github.com/thomhurst/TUnit) from 1.21.6 to 1.21.20. <details> <summary>Release notes</summary> _Sourced from [TUnit's releases](https://github.com/thomhurst/TUnit/releases)._ ## 1.21.20 <!-- Release notes generated using configuration in .github/release.yml at v1.21.20 --> ## What's Changed ### Other Changes * fix: respect TUnitImplicitUsings set in Directory.Build.props by @thomhurst in thomhurst/TUnit#5225 * feat: covariant assertions for interfaces and non-sealed classes by @thomhurst in thomhurst/TUnit#5226 * feat: support string-to-parseable type conversions in [Arguments] by @thomhurst in thomhurst/TUnit#5227 * feat: add string length range assertions by @thomhurst in thomhurst/TUnit#4935 * Fix BeforeEvery/AfterEvery hooks for Class and Assembly not being executed by @Copilot in thomhurst/TUnit#5239 ### Dependencies * chore(deps): update tunit to 1.21.6 by @thomhurst in thomhurst/TUnit#5228 * chore(deps): update dependency gitversion.msbuild to 6.7.0 by @thomhurst in thomhurst/TUnit#5229 * chore(deps): update dependency gitversion.tool to v6.7.0 by @thomhurst in thomhurst/TUnit#5230 * chore(deps): update aspire to 13.2.0 - autoclosed by @thomhurst in thomhurst/TUnit#5232 * chore(deps): update dependency typescript to v6 by @thomhurst in thomhurst/TUnit#5233 * chore(deps): update dependency polyfill to 9.23.0 by @thomhurst in thomhurst/TUnit#5235 * chore(deps): update dependency polyfill to 9.23.0 by @thomhurst in thomhurst/TUnit#5236 **Full Changelog**: thomhurst/TUnit@v1.21.6...v1.21.20 Commits viewable in [compare view](thomhurst/TUnit@v1.21.6...v1.21.20). </details> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
[BeforeEvery(Class)],[BeforeEvery(Assembly)],[AfterEvery(Class)], and[AfterEvery(Assembly)]hooks were silently never invoked, while theTestandTestSessionvariants worked fine.Root cause
HookExecutoralready had the correct pattern for Test-level hooks — collecting and executing bothBeforeEvery(Test)andBefore(Test)hooks in sequence. The Class and Assembly methods only calledCollectBefore{Class,Assembly}HooksAsyncand never called the correspondingCollectBeforeEvery*/CollectAfterEvery*methods. The hooks were registered and built correctly in both the source generator and reflection paths; only the execution callsite was missing.Changes
ExecuteBeforeClassHooksAsync/ExecuteBeforeAssemblyHooksAsync: now executeBeforeEvery(*)hooks beforeBefore(*)hooksExecuteAfterClassHooksAsync/ExecuteAfterAssemblyHooksAsync: now executeAfter(*)hooks beforeAfterEvery(*)hooks (cleanup in reverse order)Follows the identical pattern already established by
ExecuteBeforeTestHooksAsync/ExecuteAfterTestHooksAsync.Regression Tests
Added integration tests to ensure all four hook types are properly executed:
TUnit.TestProject/BeforeTests/BeforeEveryClassTests.cs:[BeforeEvery(Class)]hook sets aStateBagflag; test verifies it was setTUnit.TestProject/BeforeTests/BeforeEveryAssemblyTests.cs:[BeforeEvery(Assembly)]hook sets aStateBagflag; test verifies it was setTUnit.TestProject/AfterTests/AfterEveryClassTests.cs:[AfterEvery(Class)]hook writes a file as proof of executionTUnit.TestProject/AfterTests/AfterEveryAssemblyTests.cs:[AfterEvery(Assembly)]hook writes a file as proof of executionTUnit.Engine.Tests/BeforeEveryClassHookTests.cs/BeforeEveryAssemblyHookTests.cs: integration test runners that execute filtered tests and assert 1 passed / 0 failedTUnit.Engine.Tests/AfterEveryClassHookTests.cs/AfterEveryAssemblyHookTests.cs: integration test runners that validate tests pass and hook files were createdOriginal prompt
📍 Connect Copilot coding agent with Jira, Azure Boards or Linear to delegate work to Copilot in one click without leaving your project management tool.