-
-
Notifications
You must be signed in to change notification settings - Fork 84
Add test to verify correct hook execution order (BeforeEvery before Before) #2718
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
Co-authored-by: thomhurst <30480171+thomhurst@users.noreply.github.com>
Co-authored-by: thomhurst <30480171+thomhurst@users.noreply.github.com>
Claude finished @Copilot's task —— View job Code Review CompleteI've thoroughly reviewed this PR and have the following feedback: Todo List:
Review Feedback✅ Code Quality & Best PracticesStrengths:
Areas for Improvement:
✅ Test Logic & CoverageStrong Test Design:
Comprehensive Coverage:
✅ Potential IssuesThread Safety Concern:
Test Isolation:
✅ Performance & SecurityPerformance:
Security:
✅ Overall AssessmentThis is a well-implemented solution that effectively addresses issue #2606. The tests clearly demonstrate and verify that Key Strengths:
Minor Suggestions:
Verdict: ✅ Ready to merge - This PR effectively demonstrates the correct hook execution order and provides valuable regression protection. |
Claude finished @Copilot's task —— View job Code Review CompleteI've thoroughly reviewed this PR and have the following feedback: Todo List:
Review Feedback✅ Code Quality & Best PracticesStrengths:
Areas for Improvement:
✅ Test Logic & CoverageStrong Test Design:
Comprehensive Coverage:
✅ Potential IssuesThread Safety Concern:
Test Isolation:
✅ Performance & SecurityPerformance:
Security:
✅ Overall AssessmentThis is a well-implemented solution that effectively addresses issue #2606. The tests clearly demonstrate and verify that Key Strengths:
Minor Suggestions:
Verdict: ✅ Ready to merge - This PR effectively demonstrates the correct hook execution order and provides valuable regression protection. |
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:
[BeforeEvery(Test)]
hooks (global setup)[Before(Test)]
hooks (test-specific setup)This is exactly the behavior requested in the issue. Testing with both source generation and reflection modes confirmed that
BeforeEvery(Test)
hooks execute beforeBefore(Test)
hooks as expected.Example verification:
Output:
Changes made:
HookExecutionOrderTests.cs
with a comprehensive test that verifies the correct execution orderHookOrderTest.cs
that demonstrates the working behaviorThis 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
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.