Skip to content

Conversation

@thomhurst
Copy link
Owner

Fixes #3517

@claude
Copy link
Contributor

claude bot commented Oct 26, 2025

Claude finished @thomhurst's task —— View job


PR Review: Include Referenced Assemblies in Analyzer

I've completed a thorough review of this pull request. Here's my analysis:

Review Progress:

Summary

This is a focused and correct fix that addresses a cross-assembly analysis limitation in the AbstractTestClassWithDataSourcesAnalyzer. The change modifies line 103 from:

- var allTypes = GetAllNamedTypes(context.Compilation.Assembly.GlobalNamespace);
+ var allTypes = GetAllNamedTypes(context.Compilation.GlobalNamespace);

Analysis

Code Quality & Best Practices

  • Change is minimal and targeted: Only the specific line causing the issue was modified
  • Proper comment update: The comment was appropriately updated to reflect the new behavior
  • Consistent with analyzer patterns: The change aligns with how Roslyn analyzers typically analyze across compilation boundaries

Bug Fix Assessment

  • Addresses cross-assembly scenario: The original code only searched within the current assembly (context.Compilation.Assembly.GlobalNamespace), missing types from referenced assemblies
  • Preserves existing functionality: All existing same-assembly scenarios continue to work unchanged
  • Test coverage exists: The existing test suite in AbstractTestClassWithDataSourcesAnalyzerTests.cs has good coverage, including cross-project inheritance scenarios

Performance Considerations

  • Broader search scope: The new approach searches all types in the compilation (including referenced assemblies), which increases the search space
  • Acceptable trade-off: This is the correct behavior for an analyzer - it needs to see the full compilation to work properly
  • Efficient implementation: The GetAllNamedTypes method is already optimized for recursive traversal

Security Concerns

  • No security implications: This is a pure analyzer change that only affects diagnostic reporting
  • Read-only operation: The change only affects type discovery, not code generation or execution

Thread Safety

  • Analyzer inheritance: The class inherits from ConcurrentDiagnosticAnalyzer, indicating proper thread safety considerations
  • Immutable operations: The change only affects read-only type enumeration

Test Coverage

The existing test suite already covers the key scenarios:

  • No_Warning_When_Concrete_Class_With_InheritsTests_Exists() (lines 175-200)
  • No_Warning_When_Single_Concrete_Class_With_InheritsTests_Exists() (lines 203-226)
  • Cross-project inheritance scenarios are covered by InheritedTestsFromDifferentProjectTests

The fix ensures the analyzer will now properly detect [InheritsTests] classes in referenced assemblies, eliminating false positive warnings.

Recommendation

✅ APPROVE - This is a well-focused bug fix that:

  1. Correctly addresses the cross-assembly analysis limitation
  2. Maintains backward compatibility
  3. Has appropriate test coverage
  4. Follows established patterns in the codebase
  5. Includes proper documentation updates

The change is minimal, safe, and directly addresses the reported issue without introducing any side effects.


@thomhurst thomhurst merged commit 5f7990e into main Oct 26, 2025
13 checks passed
@thomhurst thomhurst deleted the bug/3517-2 branch October 26, 2025 23:03
@claude claude bot mentioned this pull request Oct 27, 2025
1 task
This was referenced Oct 27, 2025
This was referenced Oct 27, 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.

Regression: Analysis warning when using abstract class with data sources

2 participants