Skip to content

Conversation

@thomhurst
Copy link
Owner

Fixes #3246 #3374

Copilot AI review requested due to automatic review settings December 7, 2025 23:40
@thomhurst thomhurst linked an issue Dec 7, 2025 that may be closed by this pull request
@claude
Copy link
Contributor

claude bot commented Dec 7, 2025

Pull Request Review

Summary

This PR improves exception messages for hook failures by including the original exception message in the wrapper exception. This addresses issues #3246 and #3374 where Visual Studio Test Explorer and other test runners were showing unhelpful generic messages like "BeforeTest hook failed" instead of the actual error.

✅ Positive Aspects

  1. Addresses Real User Pain: The fix directly addresses the reported issue where IDEs (VS, Rider) only display the outer exception message, making debugging difficult.

  2. Consistent Implementation: All 10 hook exception sites are updated uniformly across all hook types (BeforeTestSession, AfterTestSession, BeforeAssembly, AfterAssembly, BeforeClass, AfterClass, BeforeTest, BeforeEveryTest, BeforeTestDiscovery, AfterTestDiscovery).

  3. Preserves Inner Exception: The change maintains the inner exception reference, ensuring the full exception chain and stack trace remain available for runners that support it.

  4. Non-Breaking: This is a pure improvement to error messages with no breaking changes to behavior.

  5. Simple and Focused: The change is minimal and focused on the specific problem.

🔍 Observations & Considerations

1. Message Format

The current format works, but consider these edge cases:

  • Multiline messages: If exception message contains newlines, the output may be harder to read
  • Long messages: Some exceptions have very long messages that might make the prefix less visible
  • Null messages: While rare, some exceptions might have null/empty messages

2. Potential Enhancement (Optional)

Consider whether to include exception type for additional clarity:

throw new BeforeTestException($"BeforeTest hook failed with {ex.GetType().Name}: {ex.Message}", ex);

3. AfterEveryTest Message Differentiation

Lines 305 and 324 use slightly different messages ("After(Test)" vs "AfterEvery(Test)"). This is actually good - it provides more specific context about which hook type failed.

📊 Code Quality Assessment

  • Style: ✅ Follows TUnit modern C# conventions (string interpolation)
  • Performance: ✅ No performance impact (hot path not affected)
  • AOT Compatibility: ✅ No reflection or AOT concerns
  • Security: ✅ No security implications
  • Dual-Mode: ✅ This is Engine-only code (reflection mode); source generator already generates user code directly

🎯 Recommendations

Required: None - the PR is ready to merge as-is.

Optional Enhancements (can be done in future PRs if desired):

  1. Consider adding exception type name to messages for additional context
  2. Consider handling edge cases like null/empty exception messages (though extremely rare)

✅ Final Verdict

APPROVED - This is a well-focused improvement that directly addresses user-reported issues with minimal risk. The change is:

  • Consistent across all hook types
  • Non-breaking
  • Simple and maintainable
  • Addresses real debugging pain points

The PR successfully improves the developer experience when debugging hook failures in Visual Studio and other test runners.


Note: Based on CLAUDE.md guidelines, this change:

  • ✅ Does not require dual-mode implementation (Engine-only)
  • ✅ Does not modify source generator output (no snapshot updates needed)
  • ✅ Does not change public API surface (no PublicAPI test updates needed)
  • ✅ Does not impact performance-critical paths
  • ✅ Follows TUnit code style (modern C# string interpolation)

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR enhances error messages for hook failures by including the original exception message in the wrapped exception. This improvement makes debugging hook failures significantly easier by immediately showing what went wrong without needing to inspect the inner exception.

Key Changes

  • Updated exception messages for all hook types to include the original exception message using string interpolation
  • Maintained the inner exception parameter to preserve full stack trace and exception details
  • Applied changes consistently across all 10 hook types (BeforeTestSession, AfterTestSession, BeforeAssembly, AfterAssembly, BeforeClass, AfterClass, BeforeEveryTest, BeforeTest, BeforeTestDiscovery, AfterTestDiscovery)

@thomhurst thomhurst merged commit cd25f3e into main Dec 8, 2025
18 of 19 checks passed
@thomhurst thomhurst deleted the bug/3246 branch December 8, 2025 00:04
@claude claude bot mentioned this pull request Dec 8, 2025
1 task
This was referenced Dec 17, 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)] hooks don't report useful errors in VS (Code) or Rider [Before], [After] hooks swallow the exception and fail silently

2 participants