Skip to content

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Aug 3, 2025

After investigating the reported hook execution order issue, I found that the current implementation is already working correctly. The hook execution order follows the expected sequence:

  1. [BeforeEvery(Test)] hooks (global setup)
  2. [Before(Test)] hooks (test-specific setup)
  3. Actual test execution

This is exactly the behavior requested in the issue. Testing with both source generation and reflection modes confirmed that BeforeEvery(Test) hooks execute before Before(Test) hooks as expected.

Example verification:

public class Tests
{
    [Before(Test)]
    public void Setup()
    {
        Console.WriteLine("Before test setup");
    }

    [Test]
    public void Basic()
    {
        Console.WriteLine("This is a basic test");
    }
}

public sealed class Hooks
{
    [BeforeEvery(Test)]
    public static void BeforeTest(TestContext testContext)
    {
        Console.WriteLine("Before every test");
    }
}

Output:

Before every test
Before test setup
This is a basic test

Changes made:

  • Added HookExecutionOrderTests.cs with a comprehensive test that verifies the correct execution order
  • Added example test case HookOrderTest.cs that demonstrates the working behavior
  • The test captures the actual execution sequence and validates that BeforeEvery hooks run before Before hooks

This test will prevent any future regressions and documents the expected behavior.

Fixes #2606.

Warning

Firewall rules blocked me from connecting to one or more addresses

I tried to connect to the following addresses, but was blocked by firewall rules:

  • aka.ms
    • Triggering command: curl -I -sSL --retry 5 --retry-delay 2 --connect-timeout 15 REDACTED (dns block)

If you need me to access, download, or install something from one of these locations, you can either:


💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

Copilot AI and others added 2 commits August 3, 2025 21:37
Co-authored-by: thomhurst <30480171+thomhurst@users.noreply.github.com>
Co-authored-by: thomhurst <30480171+thomhurst@users.noreply.github.com>
@Copilot Copilot AI changed the title [WIP] [Before(Test)] executes before [BeforeEvery(Test)] Add test to verify correct hook execution order (BeforeEvery before Before) Aug 3, 2025
@Copilot Copilot AI requested a review from thomhurst August 3, 2025 21:49
Copilot finished work on behalf of thomhurst August 3, 2025 21:49
@thomhurst thomhurst marked this pull request as ready for review August 3, 2025 22:19
@thomhurst thomhurst enabled auto-merge (squash) August 3, 2025 22:19
Copy link
Contributor

claude bot commented Aug 6, 2025

Claude finished @Copilot's task —— View job


Code Review Complete

I've thoroughly reviewed this PR and have the following feedback:

Todo List:

  • Read repository guidelines (CLAUDE.md) - No repo-specific guidelines found
  • Analyze HookExecutionOrderTests.cs
  • Analyze HookOrderTest.cs
  • Evaluate test logic and coverage
  • Check for code quality and best practices
  • Provide comprehensive review feedback

Review Feedback

Code Quality & Best Practices

Strengths:

  • Clean, readable test implementation
  • Proper use of async/await patterns in HookExecutionOrderTests.cs:27
  • Good separation of concerns with dedicated setup classes
  • Follows TUnit testing framework conventions

Areas for Improvement:

  • Missing using statements in HookOrderTest.cs:8,23 - should include using TUnit.Core; for consistency
  • Consider making _executionOrder in HookExecutionOrderTests.cs:18 private with internal accessor methods for better encapsulation

Test Logic & Coverage

Strong Test Design:

  • The HookExecutionOrderTests.cs provides an excellent programmatic verification of execution order
  • Uses a shared static list to capture actual execution sequence
  • Verifies the exact order: BeforeEvery → Before → Test
  • Clear assertions that validate each step of the execution chain

Comprehensive Coverage:

  • HookExecutionOrderTests.cs - Automated verification with assertions
  • HookOrderTest.cs - Visual verification through console output
  • Both approaches complement each other well for different debugging scenarios

Potential Issues

Thread Safety Concern:

  • _executionOrder static list in HookExecutionOrderTests.cs:18 could have race conditions if tests run in parallel
  • Recommendation: Consider using ConcurrentBag<string> or ensure tests run sequentially

Test Isolation:

  • The Clear() call in GlobalHookExecutionOrderSetup.GlobalSetup() at line 11 ensures proper test isolation - good practice

Performance & Security

Performance:

  • Minimal overhead - simple string operations and list management
  • No performance concerns for test code

Security:

  • No security vulnerabilities identified
  • Standard test code with no external dependencies or unsafe operations

Overall Assessment

This is a well-implemented solution that effectively addresses issue #2606. The tests clearly demonstrate and verify that BeforeEvery(Test) hooks execute before Before(Test) hooks as expected.

Key Strengths:

  • Solves the core problem with both automated and visual verification
  • Clean, maintainable code structure
  • Good test practices with proper setup/teardown
  • Will prevent future regressions effectively

Minor Suggestions:

  1. Add missing using TUnit.Core; to HookOrderTest.cs
  2. Consider thread safety for the shared execution order list
  3. Could add XML documentation comments for the test methods

Verdict:Ready to merge - This PR effectively demonstrates the correct hook execution order and provides valuable regression protection.


Copy link
Contributor

claude bot commented Aug 6, 2025

Claude finished @Copilot's task —— View job


Code Review Complete

I've thoroughly reviewed this PR and have the following feedback:

Todo List:

  • Read repository guidelines (CLAUDE.md) - No repo-specific guidelines found
  • Analyze HookExecutionOrderTests.cs
  • Analyze HookOrderTest.cs
  • Evaluate test logic and coverage
  • Check for code quality and best practices
  • Provide comprehensive review feedback

Review Feedback

Code Quality & Best Practices

Strengths:

  • Clean, readable test implementation
  • Proper use of async/await patterns in HookExecutionOrderTests.cs:27
  • Good separation of concerns with dedicated setup classes
  • Follows TUnit testing framework conventions

Areas for Improvement:

  • Missing using statements in HookOrderTest.cs:8,23 - should include using TUnit.Core; for consistency
  • Consider making _executionOrder in HookExecutionOrderTests.cs:18 private with internal accessor methods for better encapsulation

Test Logic & Coverage

Strong Test Design:

  • The HookExecutionOrderTests.cs provides an excellent programmatic verification of execution order
  • Uses a shared static list to capture actual execution sequence
  • Verifies the exact order: BeforeEvery → Before → Test
  • Clear assertions that validate each step of the execution chain

Comprehensive Coverage:

  • HookExecutionOrderTests.cs - Automated verification with assertions
  • HookOrderTest.cs - Visual verification through console output
  • Both approaches complement each other well for different debugging scenarios

Potential Issues

Thread Safety Concern:

  • _executionOrder static list in HookExecutionOrderTests.cs:18 could have race conditions if tests run in parallel
  • Recommendation: Consider using ConcurrentBag<string> or ensure tests run sequentially

Test Isolation:

  • The Clear() call in GlobalHookExecutionOrderSetup.GlobalSetup() at line 11 ensures proper test isolation - good practice

Performance & Security

Performance:

  • Minimal overhead - simple string operations and list management
  • No performance concerns for test code

Security:

  • No security vulnerabilities identified
  • Standard test code with no external dependencies or unsafe operations

Overall Assessment

This is a well-implemented solution that effectively addresses issue #2606. The tests clearly demonstrate and verify that BeforeEvery(Test) hooks execute before Before(Test) hooks as expected.

Key Strengths:

  • Solves the core problem with both automated and visual verification
  • Clean, maintainable code structure
  • Good test practices with proper setup/teardown
  • Will prevent future regressions effectively

Minor Suggestions:

  1. Add missing using TUnit.Core; to HookOrderTest.cs
  2. Consider thread safety for the shared execution order list
  3. Could add XML documentation comments for the test methods

Verdict:Ready to merge - This PR effectively demonstrates the correct hook execution order and provides valuable regression protection.


This was referenced Aug 20, 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.

[Before(Test)] executes before [BeforeEvery(Test)]
2 participants