You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(analyzers): validate class data against TestConstructor - #7449
Class-level data-source validation now selects the constructor marked [TestConstructor], falling back to the first constructor when none is marked. This fixes false data-type errors and makes matrix and combined-data-source diagnostics inspect the selected constructor's parameters.
Use a shared constructor lookup for all three analyzers and the shared matrix-attribute lookup. Add seven regression cases covering generic tuple rows, mismatches, matrix and combined sources, missing class attributes, and the unmarked fallback. Remove the obsolete TUnit0001 suppression from the existing generic tuple fixture.
Validation: .NET SDK 10.0.403, net10.0:
Full TUnit.Analyzers.Tests: 930 passed, 1 existing skip.
TUnit.TestProject filtered to OwnClassTupleMethodDataSourceWithTestConstructor: 1 passed.
git diff --check passed.
Existing dependency and compiler warnings are tracked in #7412 and #7410; this run's additional observations were added there. No generator output or runtime metadata paths changed.
Data source and matrix validation now use the designated test constructor when checking constructor arguments. Classes without a designated constructor retain the existing first-constructor behavior.
Tests
Added coverage for marked and unmarked constructors, argument matching, and class-level attribute requirements.
Review was skipped as selected files did not have any reviewable changes.
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 0a7e0b29-7865-4f75-8fef-c3b9c0d14dd4
Commits
Reviewing files that changed from the base of the PR and between 9c6967c and 576be38.
You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.
Use the checkbox below for a quick retry:
Trigger review
No actionable comments were generated in the recent review.
Walkthrough
Class-level data-source analyzers now validate against the constructor marked with [TestConstructor], falling back to the first instance constructor when none is marked. New tests cover method, matrix, and combined data sources, and a test fixture no longer suppresses TUnit0001.
Changes: Class-level data-source validation
Layer / File(s)
Summary
Select the test constructor src/TUnit.Analyzers/Extensions/SymbolExtensions.cs
GetTestConstructor selects the first constructor marked with [TestConstructor], or the first instance constructor if none is marked. HasMatrixValues uses this selection for named types.
Use the selected constructor for analyzer validation src/TUnit.Analyzers/TestDataAnalyzer.cs, src/TUnit.Analyzers/MatrixAnalyzer.cs, src/TUnit.Analyzers/CombinedDataSourceAnalyzer.cs, tests/TUnit.Analyzers.Tests/TestConstructorDataSourceAnalyzerTests.cs, tests/TUnit.TestProject/Bugs/_7208/ClassMethodDataSourceOnGenericClassTests.cs
The three analyzers use the selected constructor's parameters. Tests cover constructor matching and required class-level attributes. The fixture's TUnit0001 warning suppression is removed.
Issue 7377: The PR implements the constructor-selection criterion for analyzer validation. It does not address the issue's other requirements, such as sharing matching helpers or adding parity tests.
Possibly related PRs
thomhurst/TUnit#7346: Selects [TestConstructor] for analyzer conversion and source-generator inference; this PR applies the constructor rule to class-level data-source validation.
thomhurst/TUnit#7336: Selects [TestConstructor] for constructor-derived metadata; this PR uses that selection when validating class-level rows.
thomhurst/TUnit#7422: Uses the selected constructor for generic class rows and instance creation; this PR aligns analyzer validation with that selection.
Merge Risk: Minimal · previous review 9c696
The analyzers select the marked constructor for class-level data. No merge-blocking issue was identified.
Pre-merge checks: 4 · 1
Failed checks: 1 warning
Check name
Status
Explanation
Resolution
Docstring Coverage
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed checks: 4 passed
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title clearly and concisely describes the main change: analyzer validation now uses the constructor marked with TestConstructor.
Linked Issues check
The changes satisfy the coding requirements in #7370. GetTestConstructor selects the first constructor marked [TestConstructor] and falls back to the first instance constructor. TestDataAnalyzer…
Out of Scope Changes check
The changes remain within #7370. The shared helper, analyzer updates, regression tests, and removal of the obsolete suppression directly support constructor-based class-level data validation. No unrel…
Full details: Linked Issues check
Explanation
The changes satisfy the coding requirements in #7370. GetTestConstructor selects the first constructor marked [TestConstructor] and falls back to the first instance constructor. TestDataAnalyzer, MatrixAnalyzer, and CombinedDataSourceAnalyzer use this lookup. HasMatrixValues uses the same constructor selection. The seven regression tests cover marked and unmarked constructors, matching and mismatching rows, matrix data, combined data, and missing class attributes. The obsolete TUnit0001 suppression was removed from OwnClassTupleMethodDataSourceWithTestConstructor.
Full details: Out of Scope Changes check
Explanation
The changes remain within #7370. The shared helper, analyzer updates, regression tests, and removal of the obsolete suppression directly support constructor-based class-level data validation. No unrelated production behavior, generator output, or runtime metadata paths changed.
Full details: Docstring Coverage
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 5 functions across 5 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches
Generate docstrings
Commit to this branch
Create a new PR
Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit checks each row with care, The marked constructor gets its share. If none is marked, the first is found, New tests trace the matching ground. The warning hush is gone today.
CodeRabbit Verify
Run CodeRabbit Verify to test the changes in this pull request.
Recent review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 9fe3d87b-ff33-4aa4-83c5-1e66ea00b9ff
Commits
Reviewing files that changed from the base of the PR and between 309ed46 and 9c6967c.
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 4 remain after this review.
The change is correct and small. GetTestConstructor() picks the [TestConstructor] constructor and falls back to the first one. The three analyzers and HasMatrixValues now share it, and the new tests cover the marked, unmarked-fallback, matrix and combined cases. Removing the #pragma warning disable TUnit0001 from the fixture also works as a regression check.
Suggestions, none blocking:
Use one attribute-matching rule. The helper compares ToDisplayString() == "TUnit.Core.TestConstructorAttribute". That allocates a string for every attribute on every constructor, on an analyzer hot path. MultipleConstructorsAnalyzer.cs:58 already matches the attribute with Name: "TestConstructorAttribute" plus a namespace check. Reuse that logic, or compare against compilation.GetTypeByMetadataName(...) with SymbolEqualityComparer, so the analyzers cannot drift apart on what counts as a marked constructor.
Other first-constructor lookups remain.TestDataAnalyzer.cs:512 still uses testClassType.InstanceConstructors.First().Parameters.IsDefaultOrEmpty. That check decides whether a class has a parameterless constructor, so it can give a wrong answer when a [TestConstructor] is marked on a different constructor. Please check whether it should use GetTestConstructor(), or note why it should not.
Multiple marked constructors. If several constructors carry the attribute, the first one wins silently. That is probably fine, since MultipleConstructorsAnalyzer reports it separately. A short comment on the helper would say so.
Overall this is good to merge once the first two points are looked at.
Reviewed Claude's three nonblocking suggestions against the code:
Both helpers currently compare the same fully qualified attribute name, so their matching semantics agree. The name prefilter is a possible optimization; no performance change is needed for this correctness fix.
The lookup at TestDataAnalyzer.cs:512 controls instance-method source availability, rather than selection of the constructor for class-level data rows. MethodDataSourceAttribute.GetOrCreateInstanceAsync can use a public parameterless constructor independently of [TestConstructor], so replacing this lookup with GetTestConstructor would impose the wrong rule. The existing declaration-order dependency is tracked separately in Instance method data-source diagnostic depends on constructor declaration order #7456, including the source evidence and suggested regression.
First-marked selection is deterministic. MultipleConstructorsAnalyzer only diagnoses the absence of any marked constructor; it does not currently diagnose multiple marked constructors. This PR preserves the requested selection behavior; no additional diagnostic is introduced.
No blocking findings or review threads remain. Codex's completed no-findings verdict and Claude's positive review cover current head 9c6967c. Promoting for CI.
Review: looks good. The shared GetTestConstructor() helper is the right shape. It replaces three copies of InstanceConstructors.FirstOrDefault() plus the matrix lookup, and it keeps the first-constructor fallback. The tests cover marked, unmarked, mismatch, matrix and combined cases. Removing the TUnit0001 pragma from the TestProject fixture is a good check that the fix works.
Non-blocking suggestions:
TestDataAnalyzer.cs:512 still uses testClassType.InstanceConstructors.First().Parameters.IsDefaultOrEmpty, which has the same first-constructor assumption. It's worth a look, and probably a switch to the helper, if that check relates to the class under test.
The helper compares ToDisplayString() to a string for every attribute on every constructor. The analyzers already resolve well-known types elsewhere, so a cached GetTypeByMetadataName("TUnit.Core.TestConstructorAttribute") comparison would avoid string allocation on this per-class path. It would also be consistent with the rest of the analyzers.
Consider one test where a type has several [TestConstructor] attributes, or a derived attribute, to pin the behavior. Today the first marked constructor wins, which should match the runtime. Please confirm that it does.
The previous waiting run 41585221480 completed successfully. Its nonblocking suggestions retain the dispositions above: instance-source construction is tracked in #7456; attribute lookup optimization and extra constructor-selection coverage are follow-ups, with no change required for this fix.
The subsequent .NET run 41585220907 failed on four mock generator snapshots on all three operating systems. Rebased onto current main d9e9e66 instead of rerunning the unchanged failed head. Fresh local Release/net10.0 validation at rebased head 576be38: all 232 mock generator tests passed without snapshot edits; analyzer tests passed (930 passed, 1 existing skip); git diff --check passed. Existing warnings remain tracked in #7410 and #7412.
The diff still contains only the six original analyzer/regression-test files. Returned to draft for fresh reviews of the rebased head; no auto-merge is armed. The next unit should read all review surfaces, promote after both current-head verdicts, and read the promotion-triggered Claude review before arming auto-merge.
Review: fix(analyzers): validate class data against TestConstructor
The change is small and correct. I found no blocking issues.
All three analyzers and HasMatrixValues now go through one GetTestConstructor() helper. That removes the duplicated InstanceConstructors.FirstOrDefault() and keeps the analyzers in step with the runtime.
Unmarked classes keep the first-constructor fallback. InstanceConstructors excludes static constructors, so the helper can't pick one.
The seven new tests cover the main cases: generic tuple rows, a mismatch, matrix and combined sources, a missing class attribute, and the unmarked fallback. Removing the #pragma warning disable TUnit0001 from the existing test fixture is a good sign that the false positive is gone.
Optional follow-ups:
The helper matches the attribute with ToDisplayString() == "TUnit.Core.TestConstructorAttribute". That allocates a string for every attribute on every constructor. Other analyzers may already use WellKnownFullyQualifiedClassNames or symbol comparison, so check whether one fits and use it for consistency and speed. If it doesn't, ContainingNamespace plus Name checks avoid the allocation.
If a class has more than one [TestConstructor], the helper silently takes the first. This is an edge case, and it matches whatever the engine does. A one-line test pinning that behaviour would stop the analyzer and engine drifting apart.
The earlier blocker, run 41585221480, completed successfully. Reviewed all conversation comments, reviews, inline comments, and review threads: both Claude and Codex give positive verdicts for current head 576be38, with no blocking findings or unresolved threads. The latest optional suggestions retain the documented dispositions above; no further code change is required for this fix.
Marked ready at the unchanged head. Waiting for promotion-triggered Claude run https://github.com/thomhurst/TUnit/actions/runs/41592542414 and fresh CI. Auto-merge is not armed: the next unit must read the promotion review and every review surface, then use Enable-AutoMerge.ps1 if the gate passes. No repository prerequisite, superseding PR, or human decision blocks progress.
The change swaps InstanceConstructors.FirstOrDefault() for a shared GetTestConstructor() in all three analyzers and in HasMatrixValues. That fixes the false TUnit0001 on generic tuple rows, and the unmarked case still falls back to the first constructor. The seven new tests cover both the marked and unmarked paths. Dropping the #pragma warning disable TUnit0001 in the _7208 fixture also shows the fix works.
Minor, non-blocking suggestions:
GetTestConstructor calls a.AttributeClass?.ToDisplayString() on every attribute of every constructor. The analyzers run on every class, so this allocates a string per attribute. Comparing AttributeClass.Name == "TestConstructorAttribute" and then the containing namespace, or resolving the type once with compilation.GetTypeByMetadataName and using SymbolEqualityComparer, would avoid that. This matters most on the early-exit paths in MatrixAnalyzer and CombinedDataSourceAnalyzer, which were written to be cheap.
Please confirm the engine and source generator select the constructor with the same rule, including the case where several constructors are marked (first marked wins) and the case where the marked constructor is non-public. If they differ, the analyzer will still disagree with runtime in those edge cases. A test with two marked constructors would pin this down.
The earlier blocker, Claude run 41592542414, completed successfully. Its promotion review says there are no blocking issues, and Codex's completed no-findings verdict identifies current head 576be38. All current CI checks have passed; reviews, inline comments and review threads are empty, and every conversation comment has been read.
Disposition of the promotion review's optional suggestions:
Attribute-symbol matching is a possible allocation optimization, already dispositioned above; no performance change is required for this correctness fix.
Checked the source at this head: Engine/Discovery/ConstructorHelper.cs enumerates public and non-public instance constructors and returns the first marked constructor. Core.SourceGenerator/CodeGenerators/Helpers/InstanceFactoryGenerator.cs likewise scans instance constructors without an accessibility filter and returns the first marked constructor. The analyzer helper follows the same marked-constructor rule, including multiple marked constructors and a non-public marked constructor. Extra edge-case tests remain optional coverage, rather than a merge blocker.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GitHub Actions
Failure
Class-level data-source validation now selects the constructor marked
[TestConstructor], falling back to the first constructor when none is marked. This fixes false data-type errors and makes matrix and combined-data-source diagnostics inspect the selected constructor's parameters.Use a shared constructor lookup for all three analyzers and the shared matrix-attribute lookup. Add seven regression cases covering generic tuple rows, mismatches, matrix and combined sources, missing class attributes, and the unmarked fallback. Remove the obsolete TUnit0001 suppression from the existing generic tuple fixture.
Validation: .NET SDK 10.0.403, net10.0:
Existing dependency and compiler warnings are tracked in #7412 and #7410; this run's additional observations were added there. No generator output or runtime metadata paths changed.
Closes #7370
Summary by CodeRabbit