Restructure the test runner for better code coherency

[thomhurst]

Fixes #3044

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review for PR #3093

Reviewing the pull request that fixes Before(Class) and After(Class) appearing parallel when in different ParallelGroups.

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze changed files and understand the implementation
• Review code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Look for security concerns
• Assess test coverage needs
• Provide comprehensive feedback

Implementation Overview

The PR introduces a sophisticated coordination mechanism for class-level hooks to ensure proper sequential execution in different execution contexts. Key changes:

1. New TestExecutionContext model (TestExecutionContext.cs): Defines execution context types and coordination metadata
2. Execution context assignment (TestScheduler.cs:490-539): Maps tests to their execution contexts
3. Hook coordination logic (HookOrchestrator.cs:629-685): Implements sequential coordination for class hooks
4. Test metadata enhancement (AbstractExecutableTest.cs:37): Adds execution context property

✅ Strengths

Code Quality & Design

• Clean separation of concerns: New TestExecutionContext model encapsulates coordination logic well
• Modern C# patterns: Uses records, pattern matching, and nullable reference types appropriately
• Defensive programming: Proper null checks and safe dictionary operations
• Thread-safe implementation: Uses ConcurrentDictionary and proper locking mechanisms

Performance Considerations

• Efficient coordination: Uses semaphores instead of busy-waiting or polling
• Minimal overhead: Only processes coordination logic when sequential execution is required (ShouldCoordinateSequentially)
• Memory management: Proper cleanup with TryRemove operations in ReleaseSequentialCoordination

Architecture

• Maintains dual-mode compatibility: Changes work for both source-generated and reflection modes
• Non-breaking: New optional property on AbstractExecutableTest doesn't break existing tests
• Clear coordination logic: The coordination key system is well-designed and extensible

⚠️ Areas for Improvement

Potential Issues

1. Semaphore cleanup (HookOrchestrator.cs:681-684):

   if (_sequentialGroupSemaphores.TryGetValue(groupKey, out var semaphore))
   {
       semaphore.Release();
   }

   Issue: Semaphores are never disposed or removed from the dictionary, potentially causing memory leaks in long-running test sessions.
   Suggestion: Consider cleanup after coordination groups complete.

2. Error handling in coordination (HookOrchestrator.cs:647-653):

   await semaphore.WaitAsync(cancellationToken).ConfigureAwait(false);
   var releaseTask = new TaskCompletionSource<bool>();
   _pendingAfterClassTasks[(testClassType, executionContext.GroupKey)] = releaseTask;

   Issue: If cancellation occurs between acquiring semaphore and storing task completion source, semaphore won't be released.
   Suggestion: Use try/finally or consider using SemaphoreSlim.Release() in exception handling.

3. Coordination key uniqueness (HookOrchestrator.cs:658-664):

   ExecutionContextType.ParallelGroup => $"ParallelGroup:{executionContext.GroupKey}:{executionContext.Order}"

   Question: Should parallel group coordination be per-order or per-group? The current implementation coordinates per-order within a group, which may be overly restrictive.

Minor Improvements

4. Magic string usage (TestScheduler.cs:503-507):

   test.ExecutionContext = new TestExecutionContext
   {
       ContextType = ExecutionContextType.NotInParallel
   };

   Suggestion: Consider factory methods or builder pattern for consistent context creation.

5. Documentation: The new coordination logic is complex. Consider adding more detailed XML documentation explaining the coordination behavior.

🔒 Security Assessment

• No security concerns identified
• No sensitive data exposure
• Proper resource management patterns

🧪 Test Coverage Considerations

The coordination logic is complex and should be thoroughly tested:

• Sequential coordination edge cases: Test cancellation during coordination
• Memory leak prevention: Verify semaphore cleanup in long-running scenarios
• Mixed execution contexts: Test classes with different parallel group configurations
• Error propagation: Ensure coordination failures are properly handled

Recommended Actions

1. High Priority: Fix potential semaphore resource leak
2. Medium Priority: Improve error handling in coordination setup
3. Low Priority: Add comprehensive integration tests for the coordination scenarios
4. Documentation: Add detailed XML docs for the coordination behavior

Overall Assessment

This is a well-implemented solution that addresses a complex coordination problem. The design is sound, performance-conscious, and maintains backward compatibility. The main concerns are around resource cleanup and error handling edge cases, which should be addressed before merging.

Recommendation: ✅ Approve with minor changes - Address the semaphore cleanup issue and consider the error handling improvements.

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review for PR #3093

Reviewing the pull request that fixes Before(Class) and After(Class) appearing parallel when in different ParallelGroups.

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze new TestExecutionContext model
• Review AbstractExecutableTest changes
• Examine TestScheduler modifications
• Analyze HookOrchestrator updates
• Review public API changes
• Check for potential bugs or issues
• Evaluate performance considerations
• Look for security concerns
• Assess test coverage needs
• Provide comprehensive feedback summary

Code Review Summary

This PR introduces a solid architectural solution to coordinate class hooks properly across different execution contexts. The implementation demonstrates good understanding of the problem and provides a comprehensive fix.

✅ Strengths

Architecture & Design

• Clean abstraction: The TestExecutionContext model elegantly captures the different execution modes (Parallel, NotInParallel, KeyedNotInParallel, ParallelGroup)
• Proper separation: Context assignment is centralized in TestScheduler.AssignExecutionContexts() while coordination logic is handled in HookOrchestrator
• Minimal API surface: New public APIs are well-designed and follow existing patterns

Implementation Quality

• Thread safety: Proper use of ConcurrentDictionary, SemaphoreSlim, and TaskCompletionSource for coordination
• Performance optimization: Fast-path checks for hook existence prevent unnecessary work (_hasBeforeEveryTestHooks, _hasAfterEveryTestHooks)
• Memory management: Proper cleanup with TryRemove calls to prevent memory leaks
• Error handling: Comprehensive exception handling with proper aggregation

Code Quality

• Modern C#: Good use of records, pattern matching, and nullable types
• Documentation: Well-documented public APIs with XML comments
• Naming: Clear, descriptive method and property names

⚠️ Areas for Consideration

Complexity & Maintainability

• HookOrchestrator complexity: The orchestrator has grown significantly (686 lines). Consider splitting coordination logic into a separate class like SequentialExecutionCoordinator
• Multiple coordination mechanisms: The mix of semaphores, task completion sources, and dictionaries makes the coordination logic complex to follow

Performance Considerations

• Semaphore overhead: Creating semaphores per coordination group may add overhead for large test suites
• Dictionary lookups: Multiple concurrent dictionary operations in hot paths could impact performance under high concurrency

Potential Edge Cases

• Cancellation handling: The WaitForPreviousAfterClassTasks method doesn't propagate cancellation to the semaphore wait, potentially causing hanging on shutdown
• Exception scenarios: If ReleaseSequentialCoordination is never called due to exceptions, semaphores might not be released properly

🔧 Specific Issues Found

HookOrchestrator.cs:648

await semaphore.WaitAsync(cancellationToken).ConfigureAwait(false);

The cancellation token should be used consistently. The wait call uses it, but the coordination logic doesn't handle cancellation gracefully.

TestScheduler.cs:490-539

The AssignExecutionContexts method could benefit from extracting the context creation logic into separate methods for better readability.

🧪 Test Coverage Assessment

Missing Test Coverage:

• No visible tests specifically validating the Before/After class coordination behavior
• Edge cases like cancellation during coordination need testing
• Mixed execution context scenarios (classes with different parallel configurations)

Recommendation: Add integration tests that verify:

1. Before(Class) in different ParallelGroups don't run concurrently
2. After(Class) coordination works correctly
3. Cancellation during coordination is handled gracefully
4. Performance under high concurrency

🔒 Security Assessment

No security concerns identified. The implementation:

• Uses standard .NET concurrency primitives appropriately
• Doesn't expose sensitive information
• Follows defensive programming practices with null checks and proper disposal

🚀 Performance Considerations

Positive:

• Fast-path optimizations for hook existence checks
• Efficient coordination using semaphores instead of polling
• Proper memory cleanup to prevent leaks

Watch for:

• Scalability with many parallel groups (each gets its own semaphore)
• Dictionary contention under high concurrency
• Memory overhead of coordination data structures

📋 Recommendations

1. Consider refactoring: Extract coordination logic from HookOrchestrator into a dedicated SequentialExecutionCoordinator class
2. Add comprehensive tests: Focus on the coordination scenarios this PR fixes
3. Improve cancellation: Ensure proper cancellation handling throughout the coordination logic
4. Performance testing: Validate performance with large test suites having many parallel groups
5. Documentation: Add architectural docs explaining the coordination mechanism

Final Assessment

This is a well-implemented solution that addresses the core issue effectively. The architectural approach is sound, and the code quality is high. The main concerns are around complexity management and comprehensive testing. The changes maintain backward compatibility and follow TUnit's performance-first principles.

Status: ✅ Approved with minor suggestions

The fix correctly addresses the Before(Class)/After(Class) parallelism issue while maintaining good performance characteristics. Consider the refactoring suggestions for long-term maintainability.

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review for PR #3093

Reviewing the pull request that fixes Before(Class) and After(Class) appearing parallel when in different ParallelGroups.

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze new TestExecutionContext model
• Review AbstractExecutableTest changes
• Examine TestScheduler modifications
• Analyze HookOrchestrator updates
• Review public API changes
• Check for potential bugs or issues
• Evaluate performance considerations
• Look for security concerns
• Assess test coverage needs
• Provide comprehensive feedback summary

Code Review Summary

This PR introduces a solid architectural solution to coordinate class hooks properly across different execution contexts. The implementation demonstrates good understanding of the problem and provides a comprehensive fix.

✅ Strengths

Architecture & Design

• Clean abstraction: The TestExecutionContext model elegantly captures the different execution modes (Parallel, NotInParallel, KeyedNotInParallel, ParallelGroup)
• Proper separation: Context assignment is centralized in TestScheduler.AssignExecutionContexts() while coordination logic is handled in HookOrchestrator
• Minimal API surface: New public APIs are well-designed and follow existing patterns

Implementation Quality

• Thread safety: Proper use of ConcurrentDictionary, SemaphoreSlim, and TaskCompletionSource for coordination
• Performance optimization: Fast-path checks for hook existence prevent unnecessary work (_hasBeforeEveryTestHooks, _hasAfterEveryTestHooks)
• Memory management: Proper cleanup with TryRemove calls to prevent memory leaks
• Error handling: Comprehensive exception handling with proper aggregation

Code Quality

• Modern C#: Good use of records, pattern matching, and nullable types
• Documentation: Well-documented public APIs with XML comments
• Naming: Clear, descriptive method and property names

⚠️ Areas for Consideration

Complexity & Maintainability

• HookOrchestrator complexity: The orchestrator has grown significantly (686 lines). Consider splitting coordination logic into a separate class like SequentialExecutionCoordinator
• Multiple coordination mechanisms: The mix of semaphores, task completion sources, and dictionaries makes the coordination logic complex to follow

Performance Considerations

• Semaphore overhead: Creating semaphores per coordination group may add overhead for large test suites
• Dictionary lookups: Multiple concurrent dictionary operations in hot paths could impact performance under high concurrency

Potential Edge Cases

• Cancellation handling: The WaitForPreviousAfterClassTasks method doesn't propagate cancellation to the semaphore wait, potentially causing hanging on shutdown
• Exception scenarios: If ReleaseSequentialCoordination is never called due to exceptions, semaphores might not be released properly

🔧 Specific Issues Found

HookOrchestrator.cs:648

await semaphore.WaitAsync(cancellationToken).ConfigureAwait(false);

The cancellation token should be used consistently. The wait call uses it, but the coordination logic doesn't handle cancellation gracefully.

TestScheduler.cs:490-539

The AssignExecutionContexts method could benefit from extracting the context creation logic into separate methods for better readability.

🧪 Test Coverage Assessment

Missing Test Coverage:

• No visible tests specifically validating the Before/After class coordination behavior
• Edge cases like cancellation during coordination need testing
• Mixed execution context scenarios (classes with different parallel configurations)

Recommendation: Add integration tests that verify:

1. Before(Class) in different ParallelGroups don't run concurrently
2. After(Class) coordination works correctly
3. Cancellation during coordination is handled gracefully
4. Performance under high concurrency

🔒 Security Assessment

No security concerns identified. The implementation:

• Uses standard .NET concurrency primitives appropriately
• Doesn't expose sensitive information
• Follows defensive programming practices with null checks and proper disposal

🚀 Performance Considerations

Positive:

• Fast-path optimizations for hook existence checks
• Efficient coordination using semaphores instead of polling
• Proper memory cleanup to prevent leaks

Watch for:

• Scalability with many parallel groups (each gets its own semaphore)
• Dictionary contention under high concurrency
• Memory overhead of coordination data structures

📋 Recommendations

1. Consider refactoring: Extract coordination logic from HookOrchestrator into a dedicated SequentialExecutionCoordinator class
2. Add comprehensive tests: Focus on the coordination scenarios this PR fixes
3. Improve cancellation: Ensure proper cancellation handling throughout the coordination logic
4. Performance testing: Validate performance with large test suites having many parallel groups
5. Documentation: Add architectural docs explaining the coordination mechanism

Final Assessment

This is a well-implemented solution that addresses the core issue effectively. The architectural approach is sound, and the code quality is high. The main concerns are around complexity management and comprehensive testing. The changes maintain backward compatibility and follow TUnit's performance-first principles.

Status: ✅ Approved with minor suggestions

The fix correctly addresses the Before(Class)/After(Class) parallelism issue while maintaining good performance characteristics. Consider the refactoring suggestions for long-term maintainability.

---

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review for PR #3093

Reviewing the pull request that fixes Before(Class) and After(Class) appearing parallel when in different ParallelGroups.

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze new TestExecutionContext model
• Review AbstractExecutableTest changes
• Examine TestScheduler modifications
• Analyze HookOrchestrator updates
• Review SequentialCoordinator implementation
• Check public API changes
• Evaluate code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Look for security concerns
• Assess test coverage needs
• Provide comprehensive feedback summary

Code Review Summary

This PR introduces a well-architected solution to fix the coordination of Before(Class) and After(Class) hooks when running in different ParallelGroups. The implementation follows TUnit's principles and maintains both source-generated and reflection mode compatibility.

✅ Strengths

Architecture & Design

• Clean abstraction: The TestExecutionContext model elegantly captures different execution modes (Parallel, NotInParallel, KeyedNotInParallel, ParallelGroup)
• Proper separation: Context assignment is centralized in TestScheduler.AssignExecutionContexts() while coordination logic is handled in HookOrchestrator
• New SequentialCoordinator class: Well-designed dedicated coordinator that manages semaphores and provides clean disposal pattern

Code Quality

• Modern C# usage: Proper use of records, file-scoped namespaces, and nullable reference types
• Thread safety: Appropriate use of ConcurrentDictionary, SemaphoreSlim, and proper coordination mechanisms
• Resource management: Excellent disposal patterns with proper cleanup in both SequentialCoordinator and HookOrchestrator
• Documentation: Well-documented APIs with clear XML comments

Performance Considerations

• Efficient coordination: Uses semaphores instead of busy-waiting or polling
• Fast-path optimization: ShouldCoordinateSequentially() prevents unnecessary coordination overhead
• Memory management: Proper cleanup with TryRemove operations and disposal

⚠️ Areas for Consideration

Potential Issues

1. HookOrchestrator.cs:114 - Coordination key generation for ParallelGroup:

   ExecutionContextType.ParallelGroup => $"ParallelGroup:{executionContext.GroupKey}:{executionContext.Order}"

   Question: Should coordination be per-order within a group or per-group? Current implementation coordinates per-order, which ensures hooks run sequentially within each order level but may be overly restrictive if the intention is to coordinate across the entire group.

2. Exception handling during coordination: While the SequentialCoordinator properly handles cancellation, there could be edge cases where coordination tokens aren't properly released if exceptions occur during the coordination setup phase.

3. Public API surface: The new TestExecutionContext is added to the public API. While well-designed, this expands the API surface that needs to be maintained.

Minor Improvements

4. TestScheduler.cs:490-539 - The AssignExecutionContexts method could benefit from extracting context creation into factory methods for better readability and testability.

5. Coordination complexity: The coordination logic spans multiple classes (HookOrchestrator, SequentialCoordinator, TestScheduler). While well-separated, the overall flow could benefit from additional documentation.

🧪 Test Coverage Assessment

Missing Test Scenarios:

• No visible integration tests specifically validating the Before/After class coordination behavior
• Edge cases like cancellation during coordination need testing
• Mixed execution context scenarios (classes with different parallel configurations)

Recommended Tests:

1. Before(Class) in different ParallelGroups don't run concurrently
2. After(Class) coordination works correctly across different execution contexts
3. Cancellation during coordination is handled gracefully
4. Performance validation with many parallel groups

🔒 Security Assessment

No security concerns identified:

• Uses standard .NET concurrency primitives appropriately
• Proper resource disposal patterns
• No sensitive information exposure
• Follows defensive programming practices

🚀 Performance Implications

Positive:

• Minimal overhead for parallel tests (fast-path through coordination check)
• Efficient semaphore-based coordination
• Proper cleanup prevents memory leaks

Considerations:

• Each coordination group gets its own semaphore (scales with number of groups)
• Dictionary contention under high concurrency could impact performance
• Memory overhead of coordination data structures

📋 Recommendations

1. Consider documentation: Add architectural documentation explaining the coordination mechanism and when it applies
2. Integration testing: Add comprehensive tests for the coordination scenarios this PR fixes
3. Performance testing: Validate performance impact with large test suites having many parallel groups
4. API stability: Ensure the new TestExecutionContext API is stable since it's now public

Final Assessment

This is a well-implemented solution that correctly addresses the Before(Class)/After(Class) parallelism issue. The code quality is high, follows TUnit's performance-first principles, and maintains backward compatibility. The architectural approach with the dedicated SequentialCoordinator is clean and maintainable.

The implementation properly handles:

• ✅ Sequential coordination for class hooks
• ✅ Thread-safe coordination mechanisms
• ✅ Proper resource cleanup and disposal
• ✅ Minimal performance impact for parallel tests
• ✅ Dual-mode compatibility (source-gen and reflection)

Status: ✅ Approved

The fix correctly resolves the coordination issue while maintaining TUnit's core principles of being fast, modern, and reliable.

---

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review for PR #3093

Reviewing the pull request that fixes Before(Class) and After(Class) appearing parallel when in different ParallelGroups.

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze new TestExecutionContext model
• Review AbstractExecutableTest changes
• Examine TestScheduler modifications
• Analyze HookOrchestrator updates
• Review SequentialCoordinator implementation
• Check public API changes
• Evaluate code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Look for security concerns
• Assess test coverage needs
• Provide comprehensive feedback summary

Code Review Summary

This PR introduces a well-architected solution to fix the coordination of Before(Class) and After(Class) hooks when running in different ParallelGroups. The implementation follows TUnit's principles and maintains both source-generated and reflection mode compatibility.

✅ Strengths

Architecture & Design

• Clean abstraction: The TestExecutionContext model elegantly captures different execution modes (Parallel, NotInParallel, KeyedNotInParallel, ParallelGroup)
• Proper separation: Context assignment is centralized in TestScheduler.AssignExecutionContexts() while coordination logic is handled in HookOrchestrator
• New SequentialCoordinator class: Well-designed dedicated coordinator that manages semaphores and provides clean disposal pattern

Code Quality

• Modern C# usage: Proper use of records, file-scoped namespaces, and nullable reference types
• Thread safety: Appropriate use of ConcurrentDictionary, SemaphoreSlim, and proper coordination mechanisms
• Resource management: Excellent disposal patterns with proper cleanup in both SequentialCoordinator and HookOrchestrator
• Documentation: Well-documented APIs with clear XML comments

Performance Considerations

• Efficient coordination: Uses semaphores instead of busy-waiting or polling
• Fast-path optimization: ShouldCoordinateSequentially() prevents unnecessary coordination overhead
• Memory management: Proper cleanup with TryRemove operations and disposal

⚠️ Areas for Consideration

Potential Issues

1. HookOrchestrator.cs:38 - Coordination key generation for ParallelGroup:

   ExecutionContextType.ParallelGroup => $"ParallelGroup:{executionContext.GroupKey}:{executionContext.Order}"

   Question: Should coordination be per-order within a group or per-group? Current implementation coordinates per-order, which ensures hooks run sequentially within each order level but may be overly restrictive if the intention is to coordinate across the entire group.

2. Exception handling during coordination: While the SequentialCoordinator properly handles cancellation, there could be edge cases where coordination tokens aren't properly released if exceptions occur during the coordination setup phase.

3. Public API surface: The new TestExecutionContext is added to the public API. While well-designed, this expands the API surface that needs to be maintained.

Minor Improvements

4. TestScheduler.cs:490-539 - The AssignExecutionContexts method could benefit from extracting context creation into factory methods for better readability and testability.

5. Coordination complexity: The coordination logic spans multiple classes (HookOrchestrator, SequentialCoordinator, TestScheduler). While well-separated, the overall flow could benefit from additional documentation.

🧪 Test Coverage Assessment

Missing Test Scenarios:

• No visible integration tests specifically validating the Before/After class coordination behavior
• Edge cases like cancellation during coordination need testing
• Mixed execution context scenarios (classes with different parallel configurations)

Recommended Tests:

1. Before(Class) in different ParallelGroups don't run concurrently
2. After(Class) coordination works correctly across different execution contexts
3. Cancellation during coordination is handled gracefully
4. Performance validation with many parallel groups

🔒 Security Assessment

No security concerns identified:

• Uses standard .NET concurrency primitives appropriately
• Proper resource disposal patterns
• No sensitive information exposure
• Follows defensive programming practices

🚀 Performance Implications

Positive:

• Minimal overhead for parallel tests (fast-path through coordination check)
• Efficient semaphore-based coordination
• Proper cleanup prevents memory leaks

Considerations:

• Each coordination group gets its own semaphore (scales with number of groups)
• Dictionary contention under high concurrency could impact performance
• Memory overhead of coordination data structures

📋 Recommendations

1. Consider documentation: Add architectural documentation explaining the coordination mechanism and when it applies
2. Integration testing: Add comprehensive tests for the coordination scenarios this PR fixes
3. Performance testing: Validate performance impact with large test suites having many parallel groups
4. API stability: Ensure the new TestExecutionContext API is stable since it's now public

Final Assessment

This is a well-implemented solution that correctly addresses the Before(Class)/After(Class) parallelism issue. The code quality is high, follows TUnit's performance-first principles, and maintains backward compatibility. The architectural approach with the dedicated SequentialCoordinator is clean and maintainable.

The implementation properly handles:

• ✅ Sequential coordination for class hooks
• ✅ Thread-safe coordination mechanisms
• ✅ Proper resource cleanup and disposal
• ✅ Minimal performance impact for parallel tests
• ✅ Dual-mode compatibility (source-gen and reflection)

Status: ✅ Approved

The fix correctly resolves the coordination issue while maintaining TUnit's core principles of being fast, modern, and reliable.

---

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review for PR #3093

Reviewing the pull request that fixes Before(Class) and After(Class) appearing parallel when in different ParallelGroups.

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze new TestExecutionContext model
• Review AbstractExecutableTest changes
• Examine TestScheduler modifications
• Analyze HookOrchestrator updates
• Review SequentialCoordinator implementation
• Check public API changes
• Evaluate code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Look for security concerns
• Assess test coverage needs
• Provide comprehensive feedback summary

Code Review Summary

This PR introduces a well-architected solution to fix the coordination of Before(Class) and After(Class) hooks when running in different ParallelGroups. The implementation follows TUnit's principles and maintains both source-generated and reflection mode compatibility.

✅ Strengths

Architecture & Design

• Clean abstraction: The TestExecutionContext model elegantly captures different execution modes (Parallel, NotInParallel, KeyedNotInParallel, ParallelGroup)
• Proper separation: Context assignment is centralized in TestScheduler.AssignExecutionContexts() while coordination logic is handled in HookOrchestrator
• New SequentialCoordinator class: Well-designed dedicated coordinator that manages semaphores and provides clean disposal pattern

Code Quality

• Modern C# usage: Proper use of records, file-scoped namespaces, and nullable reference types
• Thread safety: Appropriate use of ConcurrentDictionary, SemaphoreSlim, and proper coordination mechanisms
• Resource management: Excellent disposal patterns with proper cleanup in both SequentialCoordinator and HookOrchestrator
• Documentation: Well-documented APIs with clear XML comments

Performance Considerations

• Efficient coordination: Uses semaphores instead of busy-waiting or polling
• Fast-path optimization: ShouldCoordinateSequentially() prevents unnecessary coordination overhead
• Memory management: Proper cleanup with TryRemove operations and disposal

⚠️ Areas for Consideration

Potential Issues

1. SequentialCoordinator.cs:38 - Coordination key generation for ParallelGroup:

   ExecutionContextType.ParallelGroup => $"ParallelGroup:{executionContext.GroupKey}:{executionContext.Order}"

   Question: Should coordination be per-order within a group or per-group? Current implementation coordinates per-order, which ensures hooks run sequentially within each order level but may be overly restrictive if the intention is to coordinate across the entire group.

2. Exception handling during coordination: While the SequentialCoordinator properly handles cancellation, there could be edge cases where coordination tokens aren't properly released if exceptions occur during the coordination setup phase.

3. Public API surface: The new TestExecutionContext is added to the public API. While well-designed, this expands the API surface that needs to be maintained.

Minor Improvements

4. TestScheduler.cs:490-539 - The AssignExecutionContexts method could benefit from extracting context creation into factory methods for better readability and testability.

5. Coordination complexity: The coordination logic spans multiple classes (HookOrchestrator, SequentialCoordinator, TestScheduler). While well-separated, the overall flow could benefit from additional documentation.

🧪 Test Coverage Assessment

Missing Test Scenarios:

• No visible integration tests specifically validating the Before/After class coordination behavior
• Edge cases like cancellation during coordination need testing
• Mixed execution context scenarios (classes with different parallel configurations)

Recommended Tests:

1. Before(Class) in different ParallelGroups don't run concurrently
2. After(Class) coordination works correctly across different execution contexts
3. Cancellation during coordination is handled gracefully
4. Performance validation with many parallel groups

🔒 Security Assessment

No security concerns identified:

• Uses standard .NET concurrency primitives appropriately
• Proper resource disposal patterns
• No sensitive information exposure
• Follows defensive programming practices

🚀 Performance Implications

Positive:

• Minimal overhead for parallel tests (fast-path through coordination check)
• Efficient semaphore-based coordination
• Proper cleanup prevents memory leaks

Considerations:

• Each coordination group gets its own semaphore (scales with number of groups)
• Dictionary contention under high concurrency could impact performance
• Memory overhead of coordination data structures

📋 Recommendations

1. Consider documentation: Add architectural documentation explaining the coordination mechanism and when it applies
2. Integration testing: Add comprehensive tests for the coordination scenarios this PR fixes
3. Performance testing: Validate performance impact with large test suites having many parallel groups
4. API stability: Ensure the new TestExecutionContext API is stable since it's now public

Final Assessment

This is a well-implemented solution that correctly addresses the Before(Class)/After(Class) parallelism issue. The code quality is high, follows TUnit's performance-first principles, and maintains backward compatibility. The architectural approach with the dedicated SequentialCoordinator is clean and maintainable.

The implementation properly handles:

• ✅ Sequential coordination for class hooks
• ✅ Thread-safe coordination mechanisms
• ✅ Proper resource cleanup and disposal
• ✅ Minimal performance impact for parallel tests
• ✅ Dual-mode compatibility (source-gen and reflection)

Status: ✅ Approved

The fix correctly resolves the coordination issue while maintaining TUnit's core principles of being fast, modern, and reliable.

---