Skip to content

Conversation

@thomhurst
Copy link
Owner

Fixes #3882

Repository owner deleted a comment from claude bot Nov 22, 2025
Repository owner deleted a comment from claude bot Nov 22, 2025
Repository owner deleted a comment from claude bot Nov 22, 2025
@claude
Copy link
Contributor

claude bot commented Nov 23, 2025

Claude finished @thomhurst's task —— View job


PR Review: After Hooks Execution on Cancellation

I've completed a comprehensive review of this PR that implements after hooks execution on cancellation (fixes #3882).

Todo List:

  • Read repository guidelines and requirements
  • Examine the new AfterHookPairTracker implementation
  • Check dual-mode implementation compliance (source generator + reflection)
  • Review TestExecutor changes and integration
  • Analyze test coverage and implementation
  • Assess code quality, performance, and security considerations
  • Provide comprehensive feedback and recommendations

⚠️ CRITICAL ISSUE: Dual-Mode Implementation Gap

According to TUnit's CLAUDE.md Rule 1, ALL features must work identically in both execution modes:

  • Reflection Mode: Fully implemented in TUnit.Engine
  • Source-Generated Mode: NO CORRESPONDING IMPLEMENTATION FOUND

Evidence:

  • Searched entire TUnit.Core.SourceGenerator directory - no references to AfterHookPairTracker
  • No cancellation registration logic in source generator output
  • Hook generation code exists but doesn't register for cancellation callbacks
  • The source generator generates hook execution code but lacks the cancellation registration that pairs After hooks with Before hooks

Impact: The feature only works in reflection mode, violating TUnit's mandatory dual-mode requirement.


📊 Code Quality Assessment

Strengths

  1. Excellent Architecture & Design

    • Clean Single Responsibility Principle implementation (AfterHookPairTracker.cs:8-13)
    • Thread-safe using ThreadSafeDictionary and proper locking
    • Well-separated concerns between hook tracking and execution
    • Proper resource management with IDisposable pattern
  2. Comprehensive Test Coverage

    • Integration tests in TUnit.TestProject for real scenarios
    • Engine tests verifying marker file creation (CancellationAfterHooksTests.cs:33-34)
    • Tests properly use --treenode-filter as required by CLAUDE.md
    • All hook levels covered (Test, Class, Assembly, Session)
  3. Performance Conscious

    • Uses ValueTask appropriately (AfterHookPairTracker.cs:84, 106, 117)
    • Efficient GetOrAdd caching pattern (AfterHookPairTracker.cs:108, 122)
    • Fire-and-forget cancellation callbacks to avoid blocking
  4. Proper AOT Annotations

    • Correct use of [DynamicallyAccessedMembers] (AfterHookPairTracker.cs:66, 118)
    • [UnconditionalSuppressMessage] with proper justifications (TestExecutor.cs:103-104)

⚠️ Code Quality Issues

  1. Sync-over-Async Pattern (AfterHookPairTracker.cs:34-42)

    var registration = cancellationToken.Register(() =>
    {
        // Blocks cancellation callback thread
        _ = GetOrCreateAfterTestSessionTask(afterHookExecutor);
    });

    Issue: Could block cancellation token processing
    Comment: The code has a comment explaining this limitation, but it's still a potential issue

  2. Inconsistent CancellationToken Usage (TestExecutor.cs:58, 94)

    // Sometimes CancellationToken.None, sometimes passed token
    () => new ValueTask<List<Exception>>(_hookExecutor.ExecuteAfterTestSessionHooksAsync(CancellationToken.None).AsTask())

    Issue: Inconsistent cancellation token handling between registration and execution

  3. Resource Disposal Issue

    • AfterHookPairTracker implements IDisposable (AfterHookPairTracker.cs:130-137)
    • Registered in service provider (TUnitServiceProvider.cs:148)
    • FIXED: Proper disposal is handled by the service provider's DisposeAsync method (TUnitServiceProvider.cs:291-306)
  4. Minor Issues

    • Hardcoded strings like "TUnit_3882_Tests" in tests (CancellationAfterHooksTests.cs:15)
    • Could benefit from more XML documentation on some public methods

🔧 Required Changes

1. Implement Dual-Mode Support (CRITICAL - BLOCKS MERGE)

The source generator must emit cancellation registration code. Currently, the source generator generates hook execution but doesn't register After hooks for cancellation:

// Source generator needs to emit something like:
serviceProvider.GetService<AfterHookPairTracker>()
    ?.RegisterAfterTestSessionHook(cancellationToken, () => /* after hook executor */);

2. Address Sync-over-Async Pattern

While the comment explains the limitation, consider alternatives:

// Option 1: Use Task.Run to avoid blocking
cancellationToken.Register(state =>
{
    _ = Task.Run(() => tracker.ExecuteAfterHooks());
}, this);

// Option 2: Consider using RegisterAsync if available in future .NET versions

3. Standardize CancellationToken Usage

Ensure consistent usage of CancellationToken.None vs. passed tokens throughout the codebase.


📈 Assessment Summary

Category Rating Notes
Dual-Mode Compliance CRITICAL FAILURE Source generator mode not implemented
Code Quality Good Well-designed, minor sync-over-async issue
Test Coverage Excellent Comprehensive integration and unit tests
Performance Good Efficient caching, appropriate async patterns
Security No Concerns No security issues identified
Resource Management Properly Handled Correct disposal in service provider

🚫 Recommendation: CHANGES REQUIRED

This PR cannot be merged until the dual-mode requirement is satisfied. The implementation approach is solid, but the missing source generator support makes it incomplete per TUnit's mandatory requirements.

Next Steps:

  1. CRITICAL: Implement source generator support for cancellation registration
  2. ⚠️ Optional: Address sync-over-async patterns and standardize cancellation token usage
  3. Required: Test both execution modes explicitly
  4. Required: Verify identical behavior between modes

The concept and reflection-mode implementation are well-executed, but dual-mode compliance is non-negotiable per CLAUDE.md Rule 1.


Repository owner deleted a comment from claude bot Nov 23, 2025
Repository owner deleted a comment from claude bot Nov 23, 2025
@claude
Copy link
Contributor

claude bot commented Nov 27, 2025

Claude finished @thomhurst's task —— View job


PR Review: After Hooks Execution on Cancellation

Todo List:

  • Read repository guidelines and requirements
  • Examine the AfterHookPairTracker implementation
  • Check dual-mode implementation compliance (source generator + reflection)
  • Review TestExecutor changes and integration
  • Analyze test coverage and implementation
  • Assess code quality, performance, and security considerations
  • Provide comprehensive feedback and recommendations

⚠️ CRITICAL ISSUE: Dual-Mode Implementation Gap

According to TUnit's CLAUDE.md Rule 1, ALL features must work identically in both execution modes:

  • Reflection Mode: Fully implemented in TUnit.Engine
  • Source-Generated Mode: NO CORRESPONDING IMPLEMENTATION FOUND

Evidence:

  • Searched entire TUnit.Core.SourceGenerator directory - no references to AfterHookPairTracker
  • No cancellation registration logic in source generator output
  • Hook generation code exists but doesn't register for cancellation callbacks
  • The source generator generates hook execution code but lacks the cancellation registration that pairs After hooks with Before hooks

Impact: The feature only works in reflection mode, violating TUnit's mandatory dual-mode requirement.


📊 Code Quality Assessment

Strengths

  1. Excellent Architecture & Design

    • Clean Single Responsibility Principle implementation (AfterHookPairTracker.cs:8-13)
    • Thread-safe using ThreadSafeDictionary and proper locking
    • Well-separated concerns between hook tracking and execution
    • Proper resource management with IDisposable pattern
  2. Comprehensive Test Coverage

    • Integration tests in TUnit.TestProject for real scenarios
    • Engine tests verifying marker file creation (CancellationAfterHooksTests.cs:33-34)
    • External cancellation tests simulating real-world scenarios (ExternalCancellationTests.cs:83-97)
    • Tests properly use --treenode-filter as required by CLAUDE.md
    • All hook levels covered (Test, Class, Assembly, Session)
  3. Performance Conscious

    • Uses ValueTask appropriately (AfterHookPairTracker.cs:85, 107, 118)
    • Efficient GetOrAdd caching pattern (AfterHookPairTracker.cs:109, 139)
    • Fire-and-forget cancellation callbacks to avoid blocking
  4. Proper AOT Annotations

    • Correct use of [DynamicallyAccessedMembers] (AfterHookPairTracker.cs:67, 119)
    • Proper trim/AOT compatibility considerations
  5. Enhanced Cancellation Support

    • Process exit handling in EngineCancellationToken.cs:75-88
    • Ctrl+C handling with graceful shutdown period
    • Forceful termination prevention for 10 seconds (EngineCancellationToken.cs:61-68)

⚠️ Code Quality Issues

  1. Sync-over-Async Pattern (AfterHookPairTracker.cs:35-42)

    var registration = cancellationToken.Register(() =>
    {
        // Blocks cancellation callback thread
        _ = GetOrCreateAfterTestSessionTask(afterHookExecutor);
    });

    Issue: Could block cancellation token processing
    Note: The code has a comment explaining this limitation

  2. Inconsistent CancellationToken Usage (TestExecutor.cs:58, 94)

    // Sometimes CancellationToken.None, sometimes passed token
    () => new ValueTask<List<Exception>>(_hookExecutor.ExecuteAfterTestSessionHooksAsync(CancellationToken.None).AsTask())

    Issue: Inconsistent cancellation token handling between registration and execution

  3. Resource Disposal

    • AfterHookPairTracker implements IDisposable (AfterHookPairTracker.cs:148-154)
    • Properly handled: Service provider manages disposal (TUnitServiceProvider.cs:148)
  4. Minor Issues

    • Hardcoded strings like "TUnit_3882_Tests" in tests (CancellationAfterHooksTests.cs:15)
    • Could benefit from more XML documentation on some public methods

🔧 Required Changes

1. Implement Dual-Mode Support (CRITICAL - BLOCKS MERGE)

The source generator must emit cancellation registration code. Currently, the source generator generates hook execution but doesn't register After hooks for cancellation:

// Source generator needs to emit something like:
serviceProvider.GetService<AfterHookPairTracker>()
    ?.RegisterAfterTestSessionHook(cancellationToken, () => /* after hook executor */);

2. Address Sync-over-Async Pattern (Optional)

While the comment explains the limitation, consider alternatives:

// Option 1: Use Task.Run to avoid blocking
cancellationToken.Register(state =>
{
    _ = Task.Run(() => tracker.ExecuteAfterHooks());
}, this);

3. Standardize CancellationToken Usage (Optional)

Ensure consistent usage of CancellationToken.None vs. passed tokens throughout the codebase.


📈 Assessment Summary

Category Rating Notes
Dual-Mode Compliance CRITICAL FAILURE Source generator mode not implemented
Code Quality Good Well-designed, minor sync-over-async issue
Test Coverage Excellent Comprehensive integration and unit tests
Performance Good Efficient caching, appropriate async patterns
Security No Concerns No security issues identified
Resource Management Properly Handled Correct disposal in service provider

🚫 Recommendation: CHANGES REQUIRED

This PR cannot be merged until the dual-mode requirement is satisfied. The implementation approach is solid, but the missing source generator support makes it incomplete per TUnit's mandatory requirements.

Next Steps:

  1. CRITICAL: Implement source generator support for cancellation registration
  2. ⚠️ Optional: Address sync-over-async patterns and standardize cancellation token usage
  3. Required: Test both execution modes explicitly
  4. Required: Verify identical behavior between modes

The concept and reflection-mode implementation are well-executed, but dual-mode compliance is non-negotiable per CLAUDE.md Rule 1.


This was referenced Dec 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

After Test hook is not run when test is cancelled

2 participants