Fix SkipTestException handling to properly mark tests as skipped #2907

Copilot · 2025-08-14T13:10:01Z

When a test throws a SkipTestException, it should mark the test as skipped, but currently it's being marked as failed. This regression was introduced after version 0.25.21.

The issue occurs in the SingleTestExecutor.HandleTestFailure() method, which treated all exceptions as failures without checking for special framework exception types.

Example of the issue:

[Test]
public async Task TestA()
{
    await Task.Yield();
    throw new SkipTestException("i should be skipped");
}

Before fix: Test is marked as ❌ Failed
After fix: Test is marked as ⚠️ Skipped

Changes Made

Modified the HandleTestFailure method in SingleTestExecutor.cs to properly handle SkipTestException:

Added explicit check for SkipTestException before the generic failure handling
When caught, sets test.State = TestState.Skipped
Uses _resultFactory.CreateSkippedResult() with the exception's reason
Follows the same pattern as existing skip handling in HandleSkippedTestInternalAsync

This minimal change ensures that all three call sites of HandleTestFailure now properly handle skip exceptions:

Main test execution catch block
Test execution with hooks catch block
After hook execution catch block

The fix maintains behavioral parity with the documentation which states that SkipTestException should result in skipped tests, not failed ones.

Fixes #2904.

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Co-authored-by: thomhurst <30480171+thomhurst@users.noreply.github.com>

claude · 2025-08-14T14:58:26Z

Claude finished @Copilot's task —— View job

PR Code Review

Reviewing the SkipTestException handling fix and providing comprehensive feedback.

Todo List:

Read repository guidelines (CLAUDE.md) - No repo guidelines found
Examine the core fix in SingleTestExecutor.cs
Review added test files for coverage and quality
Analyze code quality and best practices
Check for potential bugs or issues
Evaluate performance considerations
Assess security concerns
Provide final recommendations

Code Review Feedback

✅ Positive Aspects

Core Fix Quality: The fix in SingleTestExecutor.cs:398-404 is well-implemented:

Clean, explicit handling of SkipTestException before generic failure handling
Follows existing patterns (matches HandleSkippedTestInternalAsync approach)
Properly extracts the reason from the exception using skipEx.Reason
Maintains consistency across all three call sites that use HandleTestFailure

Test Coverage: Good test coverage approach:

Engine-level integration test validates the complete flow
Project-level test provides concrete example matching the issue description
Tests verify correct counters (0 failed, 1 not executed) confirming skip behavior

🔍 Code Quality Assessment

Implementation: The fix is minimal and surgical - exactly what's needed. The logic flow is:

Check for timeout → mark as timeout
Check for SkipTestException → mark as skipped
Default → mark as failed

This ordering is correct and prevents the skip exception from being caught by the generic failure case.

Error Handling: Proper use of the existing result factory pattern maintains consistency with the codebase architecture.

🚫 No Significant Issues Found

Bugs: No logical errors or edge cases identified.

Performance: Zero performance impact - adds one type check in exception handling path.

Security: No security concerns - this is defensive exception handling code.

Thread Safety: Uses the same patterns as existing code, inheriting the same thread safety characteristics.

🔧 Minor Suggestions

Test Naming Consistency: The test files use slightly different naming patterns:
- SkipExceptionFixTest.cs vs SkipExceptionTests.cs
- Consider using a consistent naming convention
Test Documentation: The engine test at SkipTestExceptionTests.cs:9-20 could benefit from a comment explaining why NotExecuted.ShouldBe(1) validates the skip behavior (since skipped tests count as "not executed").

📋 Verification

The fix addresses all three call sites mentioned in the PR description:

Line 176: Main test execution catch block ✅
Line 279: Test execution with hooks catch block ✅
Line 294: After hook execution catch block ✅

🎯 Final Assessment

This is a high-quality, focused fix that:

✅ Solves the exact problem described in issue Throwing SkipTestException doesn't mark test as skipped anymore #2904
✅ Follows established codebase patterns
✅ Includes appropriate test coverage
✅ Has minimal risk and no side effects
✅ Maintains backward compatibility

Recommendation: Approve - This PR successfully fixes the regression and restores proper SkipTestException handling.

claude · 2025-08-14T20:58:39Z

Claude finished @Copilot's task —— View job

PR Code Review

Reviewing the SkipTestException handling fix and providing comprehensive feedback.

Todo List:

Read repository guidelines (CLAUDE.md) - No repo guidelines found
Examine the core fix in SingleTestExecutor.cs
Review added test files for coverage and quality
Analyze code quality and best practices
Check for potential bugs or issues
Evaluate performance considerations
Assess security concerns
Provide final recommendations

Code Review Feedback

✅ Positive Aspects

Core Fix Quality: The fix in SingleTestExecutor.cs:398-404 is well-implemented:

Clean, explicit handling of SkipTestException before generic failure handling
Follows existing patterns (matches HandleSkippedTestInternalAsync approach)
Properly extracts the reason from the exception using skipEx.Reason
Maintains consistency across all three call sites that use HandleTestFailure

Test Coverage: Good test coverage approach:

Engine-level integration test validates the complete flow
Project-level test provides concrete example matching the issue description
Tests verify correct counters (0 failed, 1 not executed) confirming skip behavior

🔍 Code Quality Assessment

Implementation: The fix is minimal and surgical - exactly what's needed. The logic flow is:

Check for timeout → mark as timeout
Check for SkipTestException → mark as skipped
Default → mark as failed

This ordering is correct and prevents the skip exception from being caught by the generic failure case.

Error Handling: Proper use of the existing result factory pattern maintains consistency with the codebase architecture.

🚫 No Significant Issues Found

Bugs: No logical errors or edge cases identified.

Performance: Zero performance impact - adds one type check in exception handling path.

Security: No security concerns - this is defensive exception handling code.

Thread Safety: Uses the same patterns as existing code, inheriting the same thread safety characteristics.

🔧 Minor Suggestions

Test Naming Consistency: The test files use slightly different naming patterns:
- SkipExceptionFixTest.cs vs SkipExceptionTests.cs
- Consider using a consistent naming convention
Test Documentation: The engine test at SkipTestExceptionTests.cs:9-20 could benefit from a comment explaining why NotExecuted.ShouldBe(1) validates the skip behavior (since skipped tests count as "not executed").

📋 Verification

The fix addresses all three call sites mentioned in the PR description:

Line 176: Main test execution catch block ✅
Line 279: Test execution with hooks catch block ✅
Line 294: After hook execution catch block ✅

🎯 Final Assessment

This is a high-quality, focused fix that:

✅ Solves the exact problem described in issue Throwing SkipTestException doesn't mark test as skipped anymore #2904
✅ Follows established codebase patterns
✅ Includes appropriate test coverage
✅ Has minimal risk and no side effects
✅ Maintains backward compatibility

Recommendation: Approve - This PR successfully fixes the regression and restores proper SkipTestException handling.

github-actions · 2025-08-15T23:48:58Z

@claude [Claude Auto-Fix] Detected failed modularpipeline build (check run 48126053617).

Please analyze the build failure at https://github.com/thomhurst/TUnit/actions/runs/16976317222/job/48126053617 and commit a fix for it.

Instructions:

Look at the build errors from the modularpipeline check
Identify the root cause of the failure
Implement and commit a fix directly to the PR branch: copilot/fix-2904
Ensure the fix follows the TUnit coding standards

Focus on fixing only the build errors - do not make unrelated changes.

claude · 2025-08-16T00:22:13Z

Claude finished @Copilot's task —— View job

PR Code Review

Reviewing the SkipTestException handling fix and providing comprehensive feedback.

Todo List:

Read repository guidelines (CLAUDE.md)
Examine the core fix in SingleTestExecutor.cs
Review added test files for coverage and quality
Analyze code quality and best practices
Check for potential bugs or issues
Evaluate performance considerations
Assess security concerns
Provide final recommendations

Code Review Feedback

✅ Positive Aspects

Core Fix Quality: The fix in SingleTestExecutor.cs:442-448 is well-implemented:

Clean, explicit handling of SkipTestException before generic failure handling
Follows existing patterns (matches HandleSkippedTestInternalAsync approach)
Properly extracts the reason from the exception using skipEx.Reason
Maintains consistency across all three call sites that use HandleTestFailure

Test Coverage: Good test coverage approach:

Engine-level integration test validates the complete flow
Project-level test provides concrete example matching the issue description
Tests verify correct counters (0 failed, 1 not executed) confirming skip behavior

🔍 Code Quality Assessment

Implementation: The fix is minimal and surgical - exactly what's needed. The logic flow is:

Check for timeout → mark as timeout
Check for SkipTestException → mark as skipped
Default → mark as failed

This ordering is correct and prevents the skip exception from being caught by the generic failure case.

Error Handling: Proper use of the existing result factory pattern maintains consistency with the codebase architecture.

🚫 No Significant Issues Found

Bugs: No logical errors or edge cases identified.

Performance: Zero performance impact - adds one type check in exception handling path.

Security: No security concerns - this is defensive exception handling code.

Thread Safety: Uses the same patterns as existing code, inheriting the same thread safety characteristics.

🔧 Minor Suggestions

Test Naming Consistency: The test files use slightly different naming patterns:
- SkipExceptionFixTest.cs vs SkipExceptionTests.cs
- Consider using a consistent naming convention
Test Documentation: The engine test at SkipTestExceptionTests.cs:9-20 could benefit from a comment explaining why NotExecuted.ShouldBe(1) validates the skip behavior (since skipped tests count as "not executed").

📋 Verification

The fix addresses all three call sites mentioned in the PR description:

Line 176: Main test execution catch block ✅
Line 323: Test execution with hooks catch block ✅
Line 338: After hook execution catch block ✅

🎯 Final Assessment

This is a high-quality, focused fix that:

✅ Solves the exact problem described in issue Throwing SkipTestException doesn't mark test as skipped anymore #2904
✅ Follows established codebase patterns
✅ Includes appropriate test coverage
✅ Has minimal risk and no side effects
✅ Maintains backward compatibility

Recommendation: Approve - This PR successfully fixes the regression and restores proper SkipTestException handling.

thomhurst · 2025-08-16T01:29:41Z

@claude [Claude Auto-Fix] Detected failed modularpipeline build (check run 48205300630).

Please analyze the build failure at https://github.com/thomhurst/TUnit/actions/runs/17001967683/job/48205300630 and commit a fix for it.

Instructions:

Look at the build errors from the modularpipeline check
Identify the root cause of the failure
Implement and commit a fix directly to the PR branch: copilot/fix-2904
Ensure the fix follows the TUnit coding standards

Focus on fixing only the build errors - do not make unrelated changes.

Initial plan

f2f46d1

Copilot AI assigned Copilot and thomhurst Aug 14, 2025

Copilot started work on behalf of thomhurst August 14, 2025 13:10 View session

Copilot AI and others added 2 commits August 14, 2025 13:17

Initial investigation: Identify issue with SkipTestException handling

ccd7c9e

Co-authored-by: thomhurst <30480171+thomhurst@users.noreply.github.com>

Fix SkipTestException handling in SingleTestExecutor

2c3e569

Co-authored-by: thomhurst <30480171+thomhurst@users.noreply.github.com>

Copilot AI changed the title ~~[WIP] Throwing SkipTestException doesn't mark test as skipped anymore~~ Fix SkipTestException handling to properly mark tests as skipped Aug 14, 2025

Copilot AI requested a review from thomhurst August 14, 2025 13:30

Copilot finished work on behalf of thomhurst August 14, 2025 13:30

thomhurst marked this pull request as ready for review August 14, 2025 13:35

Copilot AI had a problem deploying to Pull Requests August 14, 2025 13:35 Failure

Merge branch 'main' into copilot/fix-2904

aaca4b2

thomhurst had a problem deploying to Pull Requests August 14, 2025 14:58 — with GitHub Actions Failure

Merge branch 'main' into copilot/fix-2904

5f20ad6

thomhurst had a problem deploying to Pull Requests August 14, 2025 20:46 — with GitHub Actions Failure

Merge branch 'main' into copilot/fix-2904

c760fbe

thomhurst had a problem deploying to Pull Requests August 16, 2025 00:21 — with GitHub Actions Failure

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Fix SkipTestException handling to properly mark tests as skipped #2907

Fix SkipTestException handling to properly mark tests as skipped #2907

Uh oh!

Copilot AI commented Aug 14, 2025 •

edited

Loading

Uh oh!

claude bot commented Aug 14, 2025 •

edited

Loading

Uh oh!

claude bot commented Aug 14, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Aug 15, 2025

Uh oh!

claude bot commented Aug 16, 2025 •

edited

Loading

Uh oh!

thomhurst commented Aug 16, 2025

Uh oh!

Uh oh!

Uh oh!

Fix SkipTestException handling to properly mark tests as skipped #2907

Fix SkipTestException handling to properly mark tests as skipped #2907

Uh oh!

Conversation

Copilot AI commented Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Example of the issue:

Changes Made

Uh oh!

claude bot commented Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Review

Todo List:

Code Review Feedback

✅ Positive Aspects

🔍 Code Quality Assessment

🚫 No Significant Issues Found

🔧 Minor Suggestions

📋 Verification

🎯 Final Assessment

Uh oh!

claude bot commented Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Review

Todo List:

Code Review Feedback

✅ Positive Aspects

🔍 Code Quality Assessment

🚫 No Significant Issues Found

🔧 Minor Suggestions

📋 Verification

🎯 Final Assessment

Uh oh!

github-actions bot commented Aug 15, 2025

Uh oh!

claude bot commented Aug 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Review

Todo List:

Code Review Feedback

✅ Positive Aspects

🔍 Code Quality Assessment

🚫 No Significant Issues Found

🔧 Minor Suggestions

📋 Verification

🎯 Final Assessment

Uh oh!

thomhurst commented Aug 16, 2025

Uh oh!

Uh oh!

Copilot AI commented Aug 14, 2025 •

edited

Loading

claude bot commented Aug 14, 2025 •

edited

Loading

claude bot commented Aug 14, 2025 •

edited

Loading

claude bot commented Aug 16, 2025 •

edited

Loading