Better exception messages when injected data sources throw

[thomhurst]

No description provided.

[claude]

Code Review - PR #4108: Better exception messages when injected data sources throw

Overview

This PR improves error handling and messaging when data sources fail during test building, converting silent failures and skipped tests into explicit failed tests with proper exception information.

Positive Aspects ✅

1. Improved User Experience

• Before: Exceptions during generic type inference or instance creation were silently caught, resulting in skipped tests with no clear indication of what went wrong.
• After: Failed tests are explicitly created with the original exception preserved, making debugging much easier.

2. Structured Error Handling

The new InstanceCreationResult struct provides a clean, type-safe way to represent success/failure:

private readonly struct InstanceCreationResult
{
    public bool Success => Exception == null;
    public static InstanceCreationResult CreateSuccess(object? instance)
    public static InstanceCreationResult CreateFailure(Exception exception)
}

This follows good patterns and makes the intent explicit.

3. Better Exception Preservation

Changed from catching all exceptions silently (catch { }) to capturing and preserving them (catch (Exception ex)), which is critical for debugging.

Suggestions & Concerns 💡

1. Struct Nullability Design (Minor)

The InstanceCreationResult struct allows Exception? to be null on success and Instance? to be null on failure. Consider adding validation:

private InstanceCreationResult(object? instance, Exception? exception)
{
    if (exception == null && instance == null)
    {
        throw new ArgumentException("Either instance or exception must be provided");
    }
    if (exception != null && instance != null)
    {
        throw new ArgumentException("Cannot have both instance and exception");
    }
    Instance = instance;
    Exception = exception;
}

However, this might be overkill for internal code. If you keep it as-is, consider adding a comment about the invariant.

2. Null-Forgiving Operator Usage (Minor - Line 1454, 1457)

yield return await CreateFailedTestForInstanceDataSourceError(metadata, instanceResult.Exception!);
// ...
instanceForMethodDataSources = instanceResult.Instance!;

The null-forgiving operators are safe here because they're guarded by the Success check, but it would be more defensive to use:

if (instanceResult.Exception is not null)
{
    yield return await CreateFailedTestForInstanceDataSourceError(metadata, instanceResult.Exception);
    continue;
}

3. Code Duplication (Moderate)

The same pattern appears twice (lines 459-510 and 518-569):

Exception? genericResolutionException = null;
try { ... }
catch (Exception ex) { genericResolutionException = ex; }

if (genericResolutionException != null) {
    var failedTest = await CreateFailedTestForDataGenerationError(...);
    tests.Add(failedTest);
}
else {
    // ... create skipped test
}

Consider extracting this into a helper method to reduce duplication:

private async Task<AbstractExecutableTest> CreateTestForEmptyDataSource(
    TestMetadata metadata, 
    Type[] resolvedClassGenericArgs, 
    Exception? genericResolutionException,
    // ... other parameters)
{
    if (genericResolutionException != null)
    {
        return await CreateFailedTestForDataGenerationError(metadata, genericResolutionException);
    }
    
    var testData = new TestData { /* ... */ };
    var testSpecificContext = new TestBuilderContext { /* ... */ };
    var test = await BuildTestAsync(metadata, testData, testSpecificContext);
    test.Context.SkipReason = "Data source returned no data";
    return test;
}

4. Performance Consideration (Minor)

The InstanceCreationResult struct contains an object? and Exception?, making it not a small struct. Since it's only used for return values and not stored, this should be fine. However, if you're concerned about copying, you could make it a readonly record struct for clarity:

private readonly record struct InstanceCreationResult(object? Instance, Exception? Exception)
{
    public bool Success => Exception == null;
    // ...
}

5. Missing Test Coverage (Important)

I don't see any new tests added for this change. According to CLAUDE.md, you should:

• Add tests that verify exceptions during data source expansion result in failed tests (not skipped)
• Add tests that verify the exception message is preserved
• Test both code paths (generic resolution failure and instance creation failure)

Consider adding integration tests like:

[Test]
public async Task DataSourceThatThrows_CreatesFailedTest()
{
    // Test that a failing data source creates a failed test with the exception
}

[Test]  
public async Task GenericInferenceFailure_CreatesFailedTestWithException()
{
    // Test that generic type inference failures produce helpful error messages
}

6. Snapshot Tests (Important)

According to CLAUDE.md Rule 2, since this changes test discovery behavior in the reflection mode, you should:

dotnet test TUnit.Engine.Tests
# If behavior changed in ways that affect output
dotnet test TUnit.Core.SourceGenerator.Tests

Have you verified snapshot tests still pass (or updated them if needed)?

Technical Correctness ✅

Exception Handling

The change from catch to catch (Exception ex) is correct and follows best practices in CLAUDE.md.

Async/Await

All async operations are properly awaited, and Task.FromResult is used appropriately for synchronous returns.

Code Style

The code follows TUnit conventions:

• Modern C# syntax (collection expressions: [])
• Proper async patterns
• File-scoped namespaces

Security & Safety ✅

• No security concerns identified
• Exception information properly preserved without exposing sensitive data
• AOT compatibility should be maintained (no new reflection usage)

Pre-Commit Checklist

Per CLAUDE.md, please verify:

• All tests pass: dotnet test
• Dual-mode implementation: This is reflection mode only (source-gen doesn't use this code path), which is correct ✅
• Added integration tests for the new behavior
• Verified snapshot tests pass
• No performance regression (this is in test discovery, which is not the hottest path, but should still be verified)

Recommendation

Approve with minor suggestions. This is a valuable improvement that makes debugging much easier. The suggestions above are mostly minor improvements, with the most important being:

1. Add test coverage for the new error handling behavior
2. Verify/update snapshot tests
3. Consider extracting duplicated code into a helper method

The core logic is sound and this significantly improves the developer experience when data sources fail.

---

Rating: 8/10 - Good improvement, needs test coverage to be complete.

[Copilot]

Pull request overview

This PR improves error handling for data source exceptions during test discovery by replacing silent test skipping with explicit failed tests that include detailed error messages. When data sources throw exceptions (either during instance creation or generic type resolution), the framework now creates FailedExecutableTest instances that surface the underlying exception, making debugging much easier for test authors.

Key Changes

• Introduced InstanceCreationResult struct to capture success/failure states when creating test class instances for method data sources
• Enhanced exception handling to capture and report specific exceptions instead of using generic catch blocks
• Modified test building logic to create failed tests (with exception details) instead of silently skipping when data source errors occur