Skip to content

Conversation

@thomhurst
Copy link
Owner

Description

Related Issue

Fixes #

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Performance improvement
  • Refactoring (no functional changes)

Checklist

Required

  • I have read the Contributing Guidelines
  • If this is a new feature, I started a discussion first and received agreement
  • My code follows the project's code style (modern C# syntax, proper naming conventions)
  • I have written tests that prove my fix is effective or my feature works

TUnit-Specific Requirements

  • Dual-Mode Implementation: If this change affects test discovery/execution, I have implemented it in BOTH:
    • Source Generator path (TUnit.Core.SourceGenerator)
    • Reflection path (TUnit.Engine)
  • Snapshot Tests: If I changed source generator output or public APIs:
    • I ran TUnit.Core.SourceGenerator.Tests and/or TUnit.PublicAPI tests
    • I reviewed the .received.txt files and accepted them as .verified.txt
    • I committed the updated .verified.txt files
  • Performance: If this change affects hot paths (test discovery, execution, assertions):
    • I minimized allocations and avoided LINQ in hot paths
    • I cached reflection results where appropriate
  • AOT Compatibility: If this change uses reflection:
    • I added appropriate [DynamicallyAccessedMembers] annotations
    • I verified the change works with dotnet publish -p:PublishAot=true

Testing

  • All existing tests pass (dotnet test)
  • I have added tests that cover my changes
  • I have tested both source-generated and reflection modes (if applicable)

Additional Notes

@github-actions
Copy link
Contributor

Summary

Adds early-exit logic to detect tests that failed during registration and reports them with clearer error messages showing the full exception chain.

Critical Issues

None found

Code Review

TestCoordinator.cs Changes: The early-exit check (lines 68-76) is correctly positioned AFTER context restoration but BEFORE result clearing. It properly checks test.Context.Execution.Result which is set by TestBuilderPipeline.cs when creating FailedExecutableTest instances.

PropertyInjector.cs Context: The exception wrapping at PropertyInjector.cs:197-201 provides nested context that creates a chain like: Failed to inject properties for type DataSource to Failed to inject properties for type DataSource2 to Oops something went wrong

Test Coverage: DataSourceExceptionPropagationTests.cs validates nested property injection failures are properly reported with error messages containing the original exception and the nested injection path. Test is properly marked as failed with correct counters.

Test Implementation: NestedDataSourcesThrow.cs creates a realistic scenario with DataSource to DataSource2 to DataSource3 where the constructor throws.

TUnit Rules Compliance

No source generator changes, no snapshot updates needed. No public API changes. No VSTest usage. No reflection without AOT annotations. No performance concerns - early-exit improves performance.

Verdict

APPROVE - Clean implementation that improves error reporting

@thomhurst thomhurst merged commit 8eec15d into main Dec 29, 2025
13 checks passed
@thomhurst thomhurst deleted the feature/clearer-injection-failure-messages branch December 29, 2025 13:46
This was referenced Dec 29, 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.

2 participants