Better exception messages when injected data sources throw

[thomhurst]

No description provided.

[claude]

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

Overview

This PR addresses issue #4049 by improving error messages when data sources fail during initialization. The change propagates exceptions from nested initializer property access instead of silently swallowing them.

---

Strengths

1. Excellent Error Context: The new error messages clearly identify which property failed, which type contains the failing property, and when the failure occurred. This is a significant improvement for debugging.

2. Proper Exception Handling: OperationCanceledException is still propagated correctly, maintains backward compatibility by still recording errors in DiscoveryErrors, and inner exceptions are preserved for full stack traces.

3. Comprehensive Test Coverage: Tests cover both nested initializer failures AND direct initializer failures, uses InvokableTestBase which tests BOTH source-generated AND reflection modes, integration tests verify actual behavior, and tests verify specific error message content.

4. Good Documentation: Clear XML doc comments explain the behavior change, issue reference included in test files, and examples show expected failure scenarios.

---

Concerns and Questions

1. Breaking Change in Error Handling Behavior (Severity: Medium)

The previous code caught exceptions and continued discovery, but the new code propagates exceptions immediately.

Impact: This is a behavioral change. Previously, if one data source property failed, discovery would continue and potentially discover other valid tests. Now, a single failing property stops all discovery for that object graph.

Questions:

• Is this the desired behavior? Should one failing property block discovery of other properties?
• Could this cause issues in scenarios where some properties are expected to throw?
• Have you tested this with complex test suites that have multiple data sources?

2. DataSourceException API Design (Severity: Low)

The FromNestedFailure factory method sets DataSourceName to empty string, which feels like a compromise.

Suggestions:

• Consider making DataSourceName nullable instead of empty string
• Or populate it with the type and property name
• The isCustomMessage parameter is unused and can be removed

3. Dual-Mode Implementation Question (Severity: Medium)

Per CLAUDE.md Rule 1, all changes must work identically in both execution modes. The tests use InvokableTestBase which is excellent.

However:

• ObjectGraphDiscoverer is in TUnit.Core and used by TUnit.Engine (reflection path)
• I did not find evidence of source generator changes in this PR

Questions:

• Does the source generator also use ObjectGraphDiscoverer for nested initializer discovery?
• Or does the source generator handle this at compile-time in a different way?
• If the source generator does not use this code path, does it exhibit the same exception propagation behavior?

Verification needed: Can you confirm that the source-generated mode also propagates these exceptions correctly?

---

Suggestions

1. Consider Adding a Config Option: Since this changes error handling behavior, consider adding a configuration option to allow users to opt into old behavior if needed.

2. Improve Error Message Granularity: Consider including the property type in the error message.

3. Performance Consideration: No concerns - fail-fast behavior reduces wasted work, and exception throwing only happens on failures.

---

Pre-Commit Checklist Review

Based on CLAUDE.md requirements:

• Tests pass (assumed - CI would verify)
• Test coverage for both modes (uses InvokableTestBase)
• Dual-mode parity needs verification (see concern 3)
• No source generator output changes - no snapshot updates needed
• No public API changes - no PublicAPI test updates needed
• Modern C# syntax used
• Proper exception handling patterns
• AOT compatibility maintained

---

Recommendations

1. Address Concern 3: Explicitly verify that source-generated mode also propagates these exceptions. If it does not, this needs dual-mode implementation.

2. Consider Backward Compatibility: Document this as a breaking change if it affects existing test suites.

3. Minor Code Cleanup: Remove the unused isCustomMessage parameter or consider making DataSourceName nullable.

4. Documentation: Update release notes to mention this behavioral change.

---

Overall Assessment

Code Quality: 4/5 | Test Coverage: 5/5 | Documentation: 4/5

This is a solid improvement that makes debugging much easier! The main concern is ensuring dual-mode parity and considering the behavioral change impact. Once concern 3 is verified, this looks ready to merge.

Great work on the comprehensive test coverage!

[Copilot]

Pull request overview

This PR improves error handling and reporting when data sources or their nested dependencies fail during initialization. Previously, property access failures during object graph discovery were silently collected for diagnostics but didn't cause tests to fail. Now, these failures are propagated as DataSourceException with detailed context about which property and type failed, ensuring developers receive clear feedback when their data sources are misconfigured.

Key Changes

• Exception propagation behavior changed from swallowing to throwing for property access failures on IAsyncInitializer properties
• Added factory method DataSourceException.FromNestedFailure() to create exceptions with custom messages
• Added comprehensive test coverage for both direct and nested initializer failures across multiple test modes

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
TUnit.Core/TestBuilder/TestBuilderException.cs	Added FromNestedFailure() factory method and private constructor to support custom error messages for nested dependency failures
TUnit.Core/Discovery/ObjectGraphDiscoverer.cs	Changed exception handling to propagate failures with contextual information instead of silently collecting them
TUnit.TestProject/Bugs/4049/InitializerExceptionPropagationTests.cs	Added integration test verifying that direct InitializeAsync() failures properly fail tests
TUnit.TestProject/Bugs/4049/NestedInitializerExceptionPropagationTests.cs	Added integration test verifying that nested property access failures properly fail tests
TUnit.Engine.Tests/DataSourceExceptionPropagationTests.cs	Added engine tests validating error message content and test failure behavior across multiple test modes

[Copilot]

The unused parameter 'isCustomMessage' is accepted but never used in the constructor body. If this parameter is intended for future use or to disambiguate constructor overloads, consider documenting its purpose. Otherwise, remove it from the signature.

[Copilot]

The error message construction spans multiple lines but uses string concatenation which creates multiple temporary strings. Consider using string interpolation for better readability and performance, or using a single multi-line string. For example: $"Failed to access property '{property.Name}' on type '{type.Name}' during object graph discovery. This may indicate that a data source or its nested dependencies failed to initialize. See inner exception for details."

Suggested change

	$"Failed to access property '{property.Name}' on type '{type.Name}' during object graph discovery. " +
	$"This may indicate that a data source or its nested dependencies failed to initialize. " +
	$"See inner exception for details.",
	ex);
	$"Failed to access property '{property.Name}' on type '{type.Name}' during object graph discovery. This may indicate that a data source or its nested dependencies failed to initialize. See inner exception for details.",
	ex);

[claude]

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

Overview

This PR improves error handling when data sources fail during initialization, specifically addressing issue #4049 where exceptions in nested IAsyncInitializer properties were silently swallowed.

Strengths

• Good problem identification and comprehensive testing
• API snapshots correctly updated
• Clear error messages with helpful context

CRITICAL ISSUE: Dual-Mode Implementation Missing

BLOCKER: Per CLAUDE.md Rule 1, ALL changes must work in both execution modes:

• Reflection mode (TUnit.Engine): Implemented
• Source-generated mode (TUnit.Core.SourceGenerator): NOT IMPLEMENTED

The source generator needs similar exception wrapping in generated code.

Code Quality Issues

1. Breaking Behavior Change
The code now throws instead of logging and continuing. This changes ObjectGraphDiscoverer from fault-tolerant to fail-fast. Was this intentional? It may break tests relying on graceful degradation.

2. OperationCanceledException Handling
Cancellation exceptions will now be added to DiscoveryErrors before being re-thrown. The catch block should be ordered before the general exception handler.

3. Design: DataSourceName = string.Empty
Setting DataSourceName to empty string feels semantically wrong. Consider accepting it as a parameter or using a different exception type.

4. Redundant Error Logging
Code both adds to DiscoveryErrors AND throws. Pick one or document why both are needed.

Potential Bug: Original Issue Not Fully Addressed

Issue #4049 was about property getters being called eagerly (causing WebApplicationFactory to build prematurely). This PR makes exceptions propagate but doesn't stop ALL property getters from being called during discovery.

The root problem remains: TUnit still calls every property getter, not just those with ClassDataSource attributes.

Recommendations

Must Fix:

1. Implement in source generator or demonstrate it's not needed
2. Fix OperationCanceledException handling
3. Clarify if fail-fast behavior change is intentional

Should Fix:
4. Simplify DataSourceException API
5. Fix DataSourceName semantics
6. Remove redundant DiscoveryErrors.Add if throwing

Consider:
7. Does this PR address the root cause? (Eager property evaluation)
8. Should the TUnit.TestProject integration tests be kept?

Happy to help implement the source generator side if needed!

[claude]

Code Review: Better Exception Messages for Injected Data Sources

Summary: This PR improves error handling and reporting when data sources (especially nested IAsyncInitializer types) fail during initialization. The implementation correctly follows TUnit's dual-mode architecture and AOT requirements.

---

✅ Strengths

1. Excellent Dual-Mode Implementation

• ✅ Source-generated path via PropertyInjectionSourceGenerator (lines 63-84)
• ✅ Reflection fallback in ObjectGraphDiscoverer.TraverseInitializerPropertiesViaReflection (lines 512-568)
• ✅ Both paths have identical exception handling behavior
• ✅ Module initializers used for AOT-compatible registration

2. Strong AOT/Trimming Compliance

• ✅ Proper use of [UnconditionalSuppressMessage] with clear justifications
• ✅ Source-generated metadata avoids reflection in AOT scenarios
• ✅ Registry pattern (InitializerPropertyRegistry) is AOT-safe
• ✅ Added [DynamicallyAccessedMembers] to PropertyCacheManager.GetCachedProperties

3. Clean Architecture

• ✅ New InitializerPropertyRegistry provides clear separation of concerns
• ✅ DataSourceException.FromNestedFailure factory method for contextual errors
• ✅ Good use of XML documentation throughout
• ✅ Test coverage for both failure scenarios

4. Performance Considerations

• ✅ ConcurrentDictionary used in registry (thread-safe, lock-free reads)
• ✅ Source-gen avoids runtime reflection overhead
• ✅ No allocations added to hot paths

[claude]

🔍 Issues & Suggestions

Critical Issue: Exception Handling Changes Behavior

Location: ObjectGraphDiscoverer.cs:492-503, 554-566

Problem: The PR changes exception handling from "swallow and continue" to "propagate and fail". While this is the intended fix, it's a breaking behavioral change that could affect existing tests.

Before: Exceptions were caught and logged but discovery continued
After: Exceptions are now propagated as DataSourceException

Impact: Any existing tests with data sources that throw during property access will now fail instead of silently continuing with null values.

Recommendation:

• ✅ This is the correct behavior (issue Undesirable side effect of all property getters being called during initialization #4049 requests this)
• ⚠️ Consider documenting this as a behavioral change in release notes
• ⚠️ May want to verify no unintended test failures in CI

---

Minor Issue: DataSourceName Set to Empty String

Location: TestBuilderException.cs:85

When using FromNestedFailure, the DataSourceName property is empty, which could confuse code that expects it to always have a value.

Suggestions:

1. Extract the type name from the message and use it as DataSourceName
2. Make DataSourceName nullable (string?) to indicate "not applicable"
3. Use a special value like "<nested>" to indicate this is a nested failure

---

Minor Issue: Potential Double Registration

Location: PropertyInjectionSourceGenerator.cs:73-84 and InitializerPropertyRegistry.Register

Question: What happens if Register is called twice for the same type?

Current Behavior: Overwrites previous registration

Recommendation: Add duplicate registration detection with TryAdd instead of direct assignment

[claude]

Enhancement: Consider Caching in Source Generator

Location: PropertyInjectionSourceGenerator.cs:668-736

The source generator looks up the IAsyncInitializer interface for every type. Consider caching this lookup across invocations for better compile-time performance in large codebases.

Note: Not critical since this only runs at compile-time.

---

Minor Style Issue: LINQ in Source Generator

Location: PropertyInjectionSourceGenerator.cs:705-708

The code uses LINQ to filter properties. From CLAUDE.md Rule 4: "Minimize allocations in hot paths... avoid LINQ"

Counter-argument: Source generators run at compile-time, not runtime, so LINQ is acceptable here. This is NOT a hot path.

Verdict: ✅ This is fine - the rule applies to runtime hot paths, not build-time code.

---

📝 Test Coverage

Excellent test coverage for the new functionality:

1. ✅ InitializerExceptionPropagationTests - Tests direct InitializeAsync failures
2. ✅ NestedInitializerExceptionPropagationTests - Tests property access failures
3. ✅ DataSourceExceptionPropagationTests - Engine-level integration tests
4. ✅ Verifies error messages contain contextual information

Tests follow best practices:

• Use [EngineTest(ExpectedResult.Failure)] to mark expected failures
• Validate error message content, not just that an error occurred
• Cover both source-gen and reflection paths (via test mode enum)

[claude]

🎯 Compliance with TUnit Development Guide

✅ Rule 1 (Dual-Mode): Fully compliant - both paths implemented
✅ Rule 2 (Snapshots): Updated .verified.txt files for public API changes
✅ Rule 3 (No VSTest): Not applicable
✅ Rule 4 (Performance): No hot path allocations added
✅ Rule 5 (AOT): Excellent AOT compliance with source-gen approach

Code Style: ✅ Modern C# syntax, proper documentation, correct patterns

---

🏁 Final Verdict

Overall Assessment: ⭐⭐⭐⭐½ (4.5/5)

Pros:

• Solves a real user pain point (issue Undesirable side effect of all property getters being called during initialization #4049)
• Architecturally sound with excellent AOT support
• Clean implementation with good test coverage
• Proper dual-mode implementation

Cons:

• Breaking behavioral change (but intentional and documented)
• Minor issue with DataSourceName being empty string
• Could benefit from duplicate registration handling

Recommendation: ✅ Approve with minor suggestions

This is high-quality work that correctly implements exception propagation while maintaining TUnit's architectural principles. The behavioral change is intentional and improves the user experience by surfacing initialization failures instead of silently swallowing them.

---

💡 Optional Future Enhancements

1. Consider adding a configuration option to restore old behavior (swallow exceptions) for backward compatibility
2. Add telemetry/metrics for tracking how often data source initialization fails
3. Consider a DiscoveryOptions class to control exception handling behavior

---

Great work on this PR! The implementation is solid and follows TUnit conventions. The minor issues mentioned are non-blocking suggestions for improvement.