fix: update ExternalCancellationTests documentation and add ExcludeOn attribute for Windows

[thomhurst]

No description provided.

[claude]

Code Review - PR #4059

Summary

This PR addresses Windows CI issues by adding the [ExcludeOn(OS.Windows)] attribute to ExternalCancellationTests and improves the documentation with a detailed explanation of why these tests are skipped on Windows.

Strengths

1. Proper Platform-Specific Handling: The use of [ExcludeOn(OS.Windows)] is the correct approach for platform-specific test behavior. The attribute is properly imported via using TUnit.Core.Enums.

2. Excellent Documentation: The added remarks block provides clear explanation of WHY tests are skipped on Windows, technical details about the CliWrap limitation with GenerateConsoleCtrlEvent, and reference to the upstream issue (Send SIGINT (Ctrl+C) on cancellation Tyrrrz/CliWrap#47). This level of documentation is exemplary and will help future maintainers.

3. Consistent with Codebase Patterns: The implementation follows existing patterns found in other test files like ExcludeOnTests.cs and RunOnSkipTests.cs.

4. Addresses Real Issue: The tests validate issue After Test hook is not run when test is cancelled #3882 (After hooks executing on external cancellation). Skipping on Windows where the mechanism doesn't work reliably is pragmatic.

Code Quality

• Style Compliance: Follows TUnit's modern C# style guide
• Attribute Usage: Correct usage of [ExcludeOn] attribute
• Documentation: Excellent inline documentation explaining the technical rationale
• Imports: Proper namespace import (TUnit.Core.Enums)

Considerations

1. Test Coverage: While skipping on Windows is justified due to CliWrap limitations, consider if there's value in having alternative Windows-specific tests that verify the same After hook behavior using a different cancellation mechanism (if feasible). However, this is NOT a blocker for this PR.

2. CI Verification: Ensure that these tests now pass on Linux/macOS in CI, Windows CI no longer attempts to run these tests, and the snapshot tests don't require updates.

Verdict

Approved

This is a clean, well-documented fix that appropriately handles platform-specific limitations. The documentation is particularly well-written and provides excellent context for future developers.

---

Reviewed following TUnit's development guidelines (CLAUDE.md)

[Copilot]

Pull request overview

This PR fixes an issue with ExternalCancellationTests on Windows by adding comprehensive documentation explaining the platform limitation and applying the ExcludeOn attribute to skip these tests on Windows, where CliWrap's graceful cancellation doesn't work reliably.

Key Changes:

• Added detailed <remarks> documentation explaining why tests are skipped on Windows (CliWrap's GenerateConsoleCtrlEvent limitation with child processes)
• Applied [ExcludeOn(OS.Windows)] attribute to the test class to prevent execution on Windows
• Added necessary using TUnit.Core.Enums import

[claude]

Pull Request Review - ExternalCancellationTests Windows Exclusion

✅ Summary

This PR appropriately addresses Windows CI failures by excluding tests that rely on CliWrap's graceful cancellation mechanism, which has known issues on Windows.

---

Code Quality: ✅ Excellent

Positives:

• Clear documentation: The added XML remarks explain exactly WHY the tests are excluded (CliWrap's GenerateConsoleCtrlEvent limitation) with a reference to the upstream issue
• Appropriate attribute usage: [ExcludeOn(OS.Windows)] is correctly used and consistent with other usages in the codebase (e.g., TUnit.TestProject/ExcludeOnTests.cs)
• Proper namespace import: Added TUnit.Core.Enums for the OS enum
• Follows TUnit style: Modern C# syntax, proper XML documentation format

---

Implementation: ✅ Correct

Technical Analysis:

1. Root Cause: The issue is well-understood and documented. CliWrap's graceful cancellation on Windows uses GenerateConsoleCtrlEvent, which doesn't propagate reliably to child processes with their own console (see: Send SIGINT (Ctrl+C) on cancellation Tyrrrz/CliWrap#47)
2. Scope: All four test methods are covered by the class-level attribute:
   • TestLevel_AfterHook_Runs_OnExternalCancellation
   • SessionLevel_AfterHook_Runs_OnExternalCancellation
   • AssemblyLevel_AfterHook_Runs_OnExternalCancellation
   • ClassLevel_AfterHook_Runs_OnExternalCancellation
3. Test Coverage: These tests will still run on Linux/macOS in CI, maintaining validation of the critical functionality (After hooks executing on external cancellation - Issue After Test hook is not run when test is cancelled #3882)

---

Potential Concerns: ⚠️ Minor Considerations

1. Windows Coverage Gap: While the exclusion is justified, Windows users lose test coverage for this critical scenario (After hooks on external cancellation). Consider:

   • Future work: Investigate Windows-specific alternatives (e.g., Ctrl+Break, taskkill /t, or other process termination mechanisms)
   • Documentation: The issue reference is helpful, but consider tracking this as a known limitation in documentation

2. Alternative Approaches (for future consideration):

   • Could use conditional compilation/runtime checks to use different cancellation strategies per OS
   • Could test the underlying After hook execution logic separately on Windows (without relying on CliWrap)

---

Best Practices Alignment: ✅

Per CLAUDE.md guidelines:

• ✅ Modern C# syntax and style
• ✅ Clear, concise documentation
• ✅ Appropriate use of existing TUnit attributes
• ✅ No breaking changes
• ✅ No performance impact

---

Recommendation: APPROVE ✅

This is a pragmatic fix that:

1. Resolves Windows CI failures
2. Maintains test coverage on platforms where the functionality works
3. Documents the limitation clearly for future reference
4. Follows TUnit conventions

Suggested follow-up (optional, not blocking):

• Track the CliWrap Windows limitation as a known issue
• Consider alternative Windows testing approaches when time permits

---

Great work addressing the CI issue with proper documentation! 🚀