Detect injected properties in base classes #3081

thomhurst · 2025-09-09T23:38:05Z

Resolves issue #3055 where tests using [InheritsTests] attribute reported source location at the attribute location instead of the actual test method location. Changes: - Added GetMethodLocation helper to extract actual method source location - Modified GenerateInheritedTestSources to use method location when available - Falls back to class location if method location unavailable - Updated snapshot tests to reflect correct line numbers 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

Improved the source location fix by leveraging the existing CallerFilePath and CallerLineNumber parameters in TestAttribute constructor arguments, which are automatically captured when the [Test] attribute is applied. This approach is: - Simpler and more reliable than parsing syntax trees - Uses the built-in compiler-provided location information - Removes the need for the GetMethodLocation helper method - More consistent with how regular test methods get their source location The TestAttribute already captures the exact file and line number via [CallerFilePath] and [CallerLineNumber] attributes, making this the preferred approach for getting accurate source locations. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

…for clarity and efficiency; add tests for bug 3072

…nd generic types; deduplicate class sources for improved efficiency

claude · 2025-09-09T23:38:18Z

Claude finished @thomhurst's task —— View job

PR Review: Detect injected properties in base classes

Reviewing this pull request that fixes issue #3072 related to detecting injected properties in base classes.

Todo List:

Read repository guidelines (CLAUDE.md)
Understand the bug being fixed by examining test files
Analyze core implementation changes
Review code quality and best practices
Check for potential bugs or issues
Assess performance considerations
Evaluate security concerns
Review test coverage
Provide comprehensive feedback

Understanding the Issue

Bug #3072 involved a case where property injection wasn't working correctly for properties defined in base classes. The test case shows:

BaseClass has a required property TestData with a data source attribute
TestFactory inherits from BaseClass and implements IAsyncInitializer
In TUnit v0.57.24, TestData was null during initialization because the property injection system wasn't detecting properties from base classes

✅ Code Quality & Best Practices

Excellent improvements:

Inheritance-aware property detection: The fix properly traverses the inheritance hierarchy using GetMembersIncludingBase() in PropertyInjectionSourceGenerator.cs:67-69
Deduplication logic: Added processedProperties HashSet to prevent duplicate property processing (PropertyInjectionSourceGenerator.cs:65,73-76)
Correct type targeting: Uses property.ContainingType instead of derived class type for UnsafeAccessor methods (PropertyInjectionSourceGenerator.cs:234,318-321)
Code organization: Well-structured helper class RequiredPropertyHelper that separates concerns

Modern C# compliance:

Uses collection initializers: new List<PropertyWithDataSourceAttribute>()
Proper nullable annotations throughout
Expression-bodied members where appropriate
Clean, descriptive variable names

✅ Bug Fixes & Correctness

Primary fix implemented correctly:

Properties from base classes are now discovered and included in source generation
Backing field access correctly targets the property's defining type, not the derived type
Prevents property name collisions through deduplication

Edge cases handled:

Skip abstract, non-public, and unbound generic types (PropertyInjectionSourceGenerator.cs:36,43-52)
Proper null checks for semantic model and interface resolution
Handles nested types correctly in accessibility checks

✅ Performance Considerations

Performance optimizations:

Deduplication prevents duplicate source generation (PropertyInjectionSourceGenerator.cs:147-150)
Early filtering of unsuitable types before expensive symbol processing
Stable naming for generated classes using deterministic hashing (PropertyInjectionSourceGenerator.cs:361-365)
Uses SymbolEqualityComparer.Default for efficient symbol comparisons

Potential concerns addressed:

The inheritance traversal is bounded (stops at System.Object)
HashSet lookup for processed properties is O(1)

✅ Security Considerations

Safe reflection usage:

UnsafeAccessor is properly constrained to .NET 8+ with preprocessor directives
Falls back to reflection with proper binding flags for older frameworks
No dynamic code generation or unsafe type casting beyond intended use

✅ Test Coverage

Comprehensive test coverage:

New test case specifically for the reported bug (TUnit.TestProject/Bugs/3072/Tests.cs)
Updated snapshot tests across multiple scenarios show the fix works correctly
Tests cover inheritance scenarios, property injection, and async initialization
Snapshots verify both source-generated and reflection modes maintain parity

Test quality:

Test demonstrates actual failure scenario (null property access)
Expected result verification with proper assertion
Covers real-world usage pattern with data sources and async initialization

✅ Architecture Adherence

Follows TUnit principles:

Behavioral parity: Changes maintain consistency between source-generated and reflection modes
Performance-first: Optimizes for compile-time generation over runtime reflection
Modern C#: Leverages latest language features appropriately

📝 Minor Suggestions

RequiredPropertyHelper.cs:20: Consider caching the data source interface lookup to avoid repeated compilation queries
Error handling: Could add more descriptive error messages for accessibility failures, though current approach is adequate
Documentation: The code is self-documenting, but additional XML documentation on public methods would be beneficial

✅ Overall Assessment

This is a high-quality fix that addresses the reported issue comprehensively:

✅ Fixes the bug: Properties from base classes are now properly detected and injected
✅ Maintains performance: Optimized for both compile-time and runtime efficiency
✅ Preserves architectural integrity: Follows TUnit's dual execution mode principles
✅ Comprehensive testing: Includes specific test case and updated snapshots
✅ Clean implementation: Well-structured, readable code following project conventions

The implementation correctly handles inheritance hierarchies while maintaining the framework's performance characteristics and architectural goals. The extensive snapshot test updates demonstrate thorough verification of the changes.

Recommendation: ✅ Approve and merge

…tions

claude · 2025-09-09T23:40:09Z

Claude finished @thomhurst's task —— View job

PR Review: Detect injected properties in base classes

Reviewing this pull request that fixes issue #3072 related to detecting injected properties in base classes.

Todo List:

Read repository guidelines (CLAUDE.md)
Understand the bug being fixed by examining test files
Analyze core implementation changes
Review code quality and best practices
Check for potential bugs or issues
Assess performance considerations
Evaluate security concerns
Review test coverage
Provide comprehensive feedback

Understanding the Issue

Bug #3072 involved a case where property injection wasn't working correctly for properties defined in base classes. The test case shows:

BaseClass has a required property TestData with a data source attribute
TestFactory inherits from BaseClass and implements IAsyncInitializer
In TUnit v0.57.24, TestData was null during initialization because the property injection system wasn't detecting properties from base classes

✅ Code Quality & Best Practices

Excellent improvements:

Inheritance-aware property detection: The fix properly traverses the inheritance hierarchy using GetMembersIncludingBase() in PropertyInjectionSourceGenerator.cs:67-69
Deduplication logic: Added processedProperties HashSet to prevent duplicate property processing (PropertyInjectionSourceGenerator.cs:65,73-76)
Correct type targeting: Uses property.ContainingType instead of derived class type for UnsafeAccessor methods (PropertyInjectionSourceGenerator.cs:234,318-321)
Code organization: Well-structured helper class RequiredPropertyHelper that separates concerns

Modern C# compliance:

Uses collection initializers: new List<PropertyWithDataSourceAttribute>()
Proper nullable annotations throughout
Expression-bodied members where appropriate
Clean, descriptive variable names

✅ Bug Fixes & Correctness

Primary fix implemented correctly:

Properties from base classes are now discovered and included in source generation
Backing field access correctly targets the property's defining type, not the derived type
Prevents property name collisions through deduplication

Edge cases handled:

Skip abstract, non-public, and unbound generic types (PropertyInjectionSourceGenerator.cs:36,43-52)
Proper null checks for semantic model and interface resolution
Handles nested types correctly in accessibility checks

✅ Performance Considerations

Performance optimizations:

Deduplication prevents duplicate source generation (PropertyInjectionSourceGenerator.cs:147-150)
Early filtering of unsuitable types before expensive symbol processing
Stable naming for generated classes using deterministic hashing (PropertyInjectionSourceGenerator.cs:361-365)
Uses SymbolEqualityComparer.Default for efficient symbol comparisons

Potential concerns addressed:

The inheritance traversal is bounded (stops at System.Object)
HashSet lookup for processed properties is O(1)

✅ Security Considerations

Safe reflection usage:

UnsafeAccessor is properly constrained to .NET 8+ with preprocessor directives
Falls back to reflection with proper binding flags for older frameworks
No dynamic code generation or unsafe type casting beyond intended use

✅ Test Coverage

Comprehensive test coverage:

New test case specifically for the reported bug (TUnit.TestProject/Bugs/3072/Tests.cs)
Updated snapshot tests across multiple scenarios show the fix works correctly
Tests cover inheritance scenarios, property injection, and async initialization
Snapshots verify both source-generated and reflection modes maintain parity

Test quality:

Test demonstrates actual failure scenario (null property access)
Expected result verification with proper assertion
Covers real-world usage pattern with data sources and async initialization

✅ Architecture Adherence

Follows TUnit principles:

Behavioral parity: Changes maintain consistency between source-generated and reflection modes
Performance-first: Optimizes for compile-time generation over runtime reflection
Modern C#: Leverages latest language features appropriately

📝 Minor Suggestions

RequiredPropertyHelper.cs:20: Consider caching the data source interface lookup to avoid repeated compilation queries
Error handling: Could add more descriptive error messages for accessibility failures, though current approach is adequate
Documentation: The code is self-documenting, but additional XML documentation on public methods would be beneficial

✅ Overall Assessment

This is a high-quality fix that addresses the reported issue comprehensively:

✅ Fixes the bug: Properties from base classes are now properly detected and injected
✅ Maintains performance: Optimized for both compile-time and runtime efficiency
✅ Preserves architectural integrity: Follows TUnit's dual execution mode principles
✅ Comprehensive testing: Includes specific test case and updated snapshots
✅ Clean implementation: Well-structured, readable code following project conventions

The implementation correctly handles inheritance hierarchies while maintaining the framework's performance characteristics and architectural goals. The extensive snapshot test updates demonstrate thorough verification of the changes.

Recommendation: ✅ Approve and merge

aradalvand · 2025-09-10T02:24:22Z

@thomhurst I think this caused a bug: #3084
Probably this part, though I didn't inspect the PR too meticulously.

thomhurst and others added 6 commits September 8, 2025 20:29

Remove redundant comments

021f308

Fix InheritsTests line numbers to reflect correct test method locations

1dab8ae

Refactor PropertyInjectionSourceGenerator and RequiredPropertyHelper …

bf73ac7

…for clarity and efficiency; add tests for bug 3072

Enhance PropertyInjectionSourceGenerator to skip non-public and unbou…

fcce4d6

…nd generic types; deduplicate class sources for improved efficiency

thomhurst had a problem deploying to Pull Requests September 9, 2025 23:38 — with GitHub Actions Failure

thomhurst temporarily deployed to Pull Requests September 9, 2025 23:38 — with GitHub Actions Inactive

Fix line numbers in verified test files for accurate test method loca…

cc2d2e4

…tions

thomhurst had a problem deploying to Pull Requests September 9, 2025 23:40 — with GitHub Actions Failure

thomhurst temporarily deployed to Pull Requests September 9, 2025 23:40 — with GitHub Actions Inactive

thomhurst enabled auto-merge (squash) September 9, 2025 23:40

thomhurst merged commit 591e23d into main Sep 10, 2025
9 of 10 checks passed

thomhurst deleted the bug/3072 branch September 10, 2025 00:08

This was referenced Sep 10, 2025

Bump TUnit from 0.57.24 to 0.57.63 Sauceforge/Saucery#318

Closed

Bump the open-source-license group with 11 updates GuyWhoKnowsTheGuy/megaschool1#35

Closed

Bump TUnit from 0.57.1 to 0.57.63 rschili/RSHome#33

Closed

aradalvand mentioned this pull request Sep 10, 2025

0.57.63 broken source generator #3084

Closed

dependabot bot mentioned this pull request Sep 10, 2025

Bump TUnit from 0.57.1 to 0.57.63 MrKWatkins/Assertions#82

Closed

Copilot AI mentioned this pull request Sep 10, 2025

Fix PropertyInjectionSourceGenerator processing static extension method classes #3085

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Detect injected properties in base classes #3081

Detect injected properties in base classes #3081

thomhurst commented Sep 9, 2025

Uh oh!

claude bot commented Sep 9, 2025 •

edited

Loading

Uh oh!

claude bot commented Sep 9, 2025 •

edited

Loading

Uh oh!

Uh oh!

aradalvand commented Sep 10, 2025 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Detect injected properties in base classes #3081

Detect injected properties in base classes #3081

Conversation

thomhurst commented Sep 9, 2025

Uh oh!

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

Uh oh!

PR Review: Detect injected properties in base classes

Understanding the Issue

✅ Code Quality & Best Practices

✅ Bug Fixes & Correctness

✅ Performance Considerations

✅ Security Considerations

✅ Test Coverage

✅ Architecture Adherence

📝 Minor Suggestions

✅ Overall Assessment

Uh oh!

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

Uh oh!

PR Review: Detect injected properties in base classes

Understanding the Issue

✅ Code Quality & Best Practices

✅ Bug Fixes & Correctness

✅ Performance Considerations

✅ Security Considerations

✅ Test Coverage

✅ Architecture Adherence

📝 Minor Suggestions

✅ Overall Assessment

Uh oh!

Uh oh!

aradalvand commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

claude bot commented Sep 9, 2025 •

edited

Loading

claude bot commented Sep 9, 2025 •

edited

Loading

aradalvand commented Sep 10, 2025 •

edited

Loading