Repository navigation
fix(generic): fail wrongly sized [GenerateGenericTest] in source-generated mode - #7344
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough
Pre-merge checks:
 |
| 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: source-generated mode now fails incorrectly sized [GenerateGenericTest] attributes. |
| Linked Issues check | |
Directly linked issue #7222 requires that source-generated mode and reflection mode handle a method-level [GenerateGenericTest] with no type arguments or the wrong number of them in the same way. The … |
| Out of Scope Changes check | |
The changes stay connected to #7222 or to the same generic-test discovery path. Connected changes: arity and null/pointer/array type-argument handling in TestMetadataGenerator and ReflectionTestDataCo… |
| Docstring Coverage | |
Docstring coverage is 92.31% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentati… |
Full details: Linked Issues check
-
Explanation
Directly linked issue
#7222requires that source-generated mode and reflection mode handle a method-level [GenerateGenericTest] with no type arguments or the wrong number of them in the same way. The issue allows either a failing source-generated entry with the reflection message or a compile-time analyzer. The PR takes the failing-entry route. Evidence: (1) src/TUnit.Core/Helpers/GenerateGenericTestArityMessages.cs exists at the reviewed head. It defines NoTypeArguments ('... has no type arguments') and WrongCount ('... provides N type argument(s) but method requires M. Provided: [...]'). Its text matches the reflection message quoted in#7222. The source generator compiles this same file. (2) Per the PR summary, TestMetadataGenerator adds one failure per invalid method-level attribute to the method's shared generic-violation entry, in declaration order. ReflectionTestDataCollector.ResolveMethodInstantiations now places the arity failure in that same GenericConstraintViolationTestMetadata entry, so both modes report the method name, message, and order. (3) The#7222repro (mixed arity with an object-row data source) and the empty-attribute case are covered by new tests: tests/TUnit.TestProject/Bugs/_7222, Issue7222Tests (engine), and Tests7222 (generator, with snapshot). The PR summary reports matching names, outcomes, and messages across both modes. Limits: the repository diff for ReflectionTestDataCollector.cs and the generator source could not be read in this review (changed blobs were unavailable). The generator and reflection behavior therefore rests on the PR summary and the author's reported test runs. TUnit.Engine.Tests was compiled but not run locally. The shared helper file was read directly and matches the issue text. No coding requirement of#7222appears unmet in the available evidence.
Full details: Out of Scope Changes check
-
Explanation
The changes stay connected to
#7222or to the same generic-test discovery path. Connected changes: arity and null/pointer/array type-argument handling in TestMetadataGenerator and ReflectionTestDataCollector; the shared TUnit.Core arity-message helper, linked into the source-generator projects and eng/Roslyn.props so that both modes use one message source; the snapshot and assertion updates in Tests7116, Tests7107, GenericConstraintValidationTests, and Issue7116Tests, which follow from the changed arity behavior; and the new regression tests under tests/TUnit.TestProject/Bugs/_7222 and tests/TUnit.Engine.Tests/Issue7222Tests.cs. The constructor-instance fix (InstanceFactoryGenerator for a non-generic class with constructor arguments and a generic [GenerateGenericTest] method) changes the same generated-test path, and the PR states that a valid attribute failed with CS7036 before the fix. It is disclosed as a pre-existing bug fixed alongside and is closely related. The deferred refactor items (#7352,#7369) are not implemented here, so they add no out-of-scope code. No public API change is reported. No unrelated feature or unrelated public surface appears in the summary. Limit: the diff hunks could not be read in this review, so the assessment relies on the PR summary for file-level scope.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 92.31% which is sufficient. 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 13 functions across 11 files.
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
Warning
Review coverage is incomplete: 1 file could not be fully reviewed. Findings from completed review steps are included; see review info for details.
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 type with care
Missing arguments get errors there
Rows meet classes, matched just right
Valid tests hop into sight
The wrong-sized ones are logged outright
CodeRabbit Verify
- Run CodeRabbit Verify to test the changes in this pull request.
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
 [Medium impact] The PR appears safe to merge; no new actionable issue was found. SummaryInvalid method-level
Reviews (17) · Last reviewed commit: "test: refresh skipped generic constraint..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Review: PR 7344 (GenerateGenericTest arity failures in both modes)I found no blocking issues. The change does what the description says. What I checked
Non-blocking suggestions
Overall this is a clean change with good test coverage. |
33ca9bc to
dc3bc08
Compare
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
Review: fail wrongly sized
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc3bc08bfd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
dc3bc08 to
6a953f7
Compare
Review: fail wrongly sized
|
|
Dispositions for the Claude review comments, as of ed70a76:
|
|
@codex review Addresses the five review threads (generic-class concrete classes, order, class rows, constructor arguments, pointer type arguments). |
Review of #7344I read the diff but did not build it or run any tests. What it does well
Concerns
Overall the direction is right and the parity goal is worthwhile. I would address the duplication in 1 and the second enumeration in 2 before merge, or open a follow-up issue for them. |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
… arity messages (#7222) A constraint-violation entry that wraps an [InstanceMethodDataSource] now keeps the class data sources, so source-gen mode can build the instance its rows come from instead of failing once with a constructor error (Greptile P1 on #7344). The [GenerateGenericTest] arity messages now live in one file in TUnit.Core that the source generator also compiles, so the two modes cannot drift.
|
Dispositions for the Claude review of ed70a76 (comment 6563902020), fixed in e12efab:
|
|
@codex review Keeps class data sources on violation entries that wrap an instance data source (Greptile P1), and shares the [GenerateGenericTest] arity messages between both modes through one linked TUnit.Core file. |
ReviewPrior review feedback (shared arity-message file, instance-data-source What I checked
Remaining observations (non-blocking)
No blocking issues. Tests cover both modes, and the snapshot changes look intentional. LGTM. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e12efabdc5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Summary
A method-level
[GenerateGenericTest]with no type arguments, or the wrong number of them, failed in reflection mode but was silently dropped in source-generated mode. Both modes now fail it the same way.Source generator (
TestMetadataGenerator): each such attribute adds a failure, in declaration order, to the method's shared generic-violation entry. Its message is the one reflection mode already used:[GenerateGenericTest] on method 'M' provides 2 type argument(s) but method requires 1. Provided: [MemoryStream, Int32]/... has no type arguments. On a generic class, it fails once on each concrete class, because reflection mode resolves the method per concrete class.Reflection (
ReflectionTestDataCollector.ResolveMethodInstantiations): the arity failure now goes into the sameGenericConstraintViolationTestMetadataentry as constraint violations, not into a separate[GENERIC METHOD RESOLUTION FAILED] ...test. Both modes therefore report the same test name (the method name), message and order.One rule for broad data sources: with any method-level
[GenerateGenericTest], a data source whose declared type breaks the constraints defers to it, as reflection mode does. Before this change the generator reported the declared-type violation when every[GenerateGenericTest]was wrongly sized. That was its only way to keep the rows from disappearing, and the arity failure now covers it. As a resultWrongArity…/EmptyGenerateGenericTestWithObjectRowsreport the arity message in both modes, not a different failure in each.Fixed alongside:
ExtractTypeArgsFromGenerateGenericTestAttributedropped array type arguments (typeof(int[])is not anINamedTypeSymbol). Without that fix, this change would have reported[GenerateGenericTest(typeof(int[]))]as having no type arguments. It now runsM<int[]>in both modes.Review follow-ups: on a generic class, a wrongly sized attribute now fails on every concrete class, even when no method-level attribute is valid. Its failure keeps declaration order beside constraint failures. Violation entries no longer carry class data sources, as in reflection mode, so a failure does not repeat for each class row. This also drops the class row from two existing violation entries in the
Tests7107andGenericConstraintValidationTestssnapshots. A pointer type argument (typeof(int*)) fails in both modes instead of emittingM<int*>(), which does not compile.Pre-existing bug fixed alongside: a non-generic class with a constructor argument and a generic
[GenerateGenericTest]method emittednew C(), which failed with CS7036 even for a valid attribute. It now builds the instance throughInstanceFactoryGenerator.Second review round: a violation entry that wraps an
[InstanceMethodDataSource]keeps its class data sources, so source-gen mode builds the instance its rows come from and fails each row, not once with a constructor error (Greptile P1). The arity messages now come from one internalTUnit.Core/Helpers/GenerateGenericTestArityMessages.cs, which the source generator compiles through a linked file. Follow-ups filed: Source-gen does not infer generic method types from [InstanceMethodDataSource]; drops it on generic classes #7351 (source-gen does not infer method types from[InstanceMethodDataSource], and drops it on generic classes; this also happens on main) and Source generator: record generic-class arity violations during concrete-class discovery instead of re-enumerating #7352 (generator refactor from the Claude review).Third review round: source-gen cleared class data sources on every violation entry. Reflection mode drops them only for methods with a method-level
[GenerateGenericTest], so a constraint-violating row on a method without one failed once in source-gen mode and once per class row in reflection mode (Codex P1). Source-gen now clears them only in that case (_7222/ClassRowsConstraintViolationTests, 2 failures in both modes). That restores main's class rows in theTests7107andGenericConstraintValidationTestssnapshots, which an earlier round of this PR had dropped.eng/Roslyn.propsalso links the shared arity-message file into the Roslyn44/47/414 core generator builds, which failed with CS0234 (Greptile P1).Third-round validation: the Roslyn44/47/414 core generators build.
tests/TUnit.Core.SourceGenerator.Tests: 863 passed, 4 skipped, 0 failed.tests/TUnit.TestProjectin both modes:_7222/*/*gives 23 tests and 15 failed;_710746 and 26;GenericConstraintValidationTests9 and 5;GenericConstraintRowParityTests16 and 8;_7116gives 7 and 4 failures (the intended differences).TUnit.Engine.Testscompiled, not run locally.Fourth review round (CodeRabbit): a null
[GenerateGenericTest]type argument threwNullReferenceExceptionout of reflection-mode discovery, and source-gen dropped it, so[GenerateGenericTest(typeof(int), null)]ranM<int>. Both modes now count the null and fail with the same message. On a generic class, a wrongly sized method-level attribute, or one naming a null or pointer type, is no longer paired with the class type arguments, soM<int*>()is no longer emitted. A typed data source or[MethodDataSource]whose declared type breaks the constraints now defers to the method-level[GenerateGenericTest]there too, as reflection mode does. New cases:NullTypeArgumentGenerateGenericTestTests,BroadSourceOnGenericClassRowTests,BroadSourceOnGenericClassTypeTests, and the inlinePointerOnGenericClassTestsinTests7222.Fourth-round validation:
tests/TUnit.Core.SourceGenerator.Testsall TFMs: 867 passed, 4 skipped, 0 failed.tests/TUnit.TestProjectin both modes:_7222/*/*gives 27 tests and 19 failures in each, with identical names, outcomes and messages apart from the existingStructOnlymessage (Source-gen does not infer generic method types from [InstanceMethodDataSource]; drops it on generic classes #7351)._711614 (7/4, the intended differences)._710746/26,GenericConstraintValidationTests9/5,GenericConstraintRowParityTests16/8,_711715/4. The Roslyn44/47/414 generators build.TUnit.Engine.Testscompiles; not run locally.Decision note: the issue offered either a failing entry in source-gen mode or a compile-time analyzer. I chose the failing entry. It matches how the other generic violations (#7107, #7153, #7163) are reported, it also covers reflection-only users, and it is reversible. An analyzer could be added later on top.
Tests
tests/TUnit.TestProject/Bugs/_7222cases: too many, none, too few, mixed valid and invalid, an array type argument, and a generic class with a generic method.Issue7222Tests(engine) andTests7222(generator, with snapshot)._7222:OnlyWronglySizedOnGenericClassTests,ArityAndConstraintOrderTests,ClassRowsGenerateGenericTestArityTests,ConstructorArgumentGenerateGenericTestArityTests, and an unsafe pointer case added as an inline syntax inTests7222. TUnit.TestProject does not allow unsafe code.Issue7116Tests.GenerateGenericTest_With_Full_Arity_Runs_Broad_Rows_Beside_A_Wrongly_Sized_One: it now expects 2 tests in both modes, where it used to expect a mode-dependent count. Updated theTests7116assertions and snapshot to match.Validation
tests/TUnit.Core.SourceGenerator.Tests863 passed, 4 skipped, 0 failed (no snapshot changes).tests/TUnit.TestProject_7222/*/*gives 21 tests and 13 failed in each mode;_7107gives 46 and 26 in both;GenericConstraintValidationTests9 and 5 in both;_7116gives 14 tests with 7 failures in source-gen mode and 4 in reflection mode, the 3 intended differences.TUnit.Engine.Testscompiled, not run locally.tests/TUnit.Core.SourceGenerator.Tests, all TFMs (net472, net8, net9, net10): 863 passed, 4 skipped, 0 failed (rerun after the review fixes).tests/TUnit.TestProjectrun in both modes with these filters:_7222/*/*gives 19 tests and 11 failed in each mode, with identical names and outcomes;_7107/*/*gives 46 and 26;GenericConstraintRowParityTests16 and 8;GenericConstraintValidationTests9 and 5;GenericClassWith*6 and 0._7116differs only in its three intended declared-type cases.TUnit.Engine.Testswas compiled but not run locally, per AGENTS.md; CI runs it.tests/TUnit.Engine.Tests(net10.0, reflection mode locally; the AOT/source-gen leg runs only in CI):Issue7222Tests,Issue7116Tests,GenericConstraintValidationTests,Issue7107TestsandIssue7163Testsall pass.tests/TUnit.TestProjectrun directly in both modes with--treenode-filter "/*/TUnit.TestProject.Bugs._7222/*/*": source-gen and--reflectionboth give 11 tests, 6 failed (intended) and 5 passed, with the same names._7116/DeclaredTypeInferenceTests/*now differs only in the cases Source-gen infers generic type arguments from a data source's declared type, so broad declared types fail constraints that the runtime value meets #7116 intends to differ.Closes #7222
Summary by CodeRabbit
Bug Fixes
Tests
Explicit-test review fix
Commit d0fa04b preserves non-row method attributes on generic-resolution failures in reflection metadata and both shared failed-test construction paths. Explicit methods with empty or wrongly sized
[GenerateGenericTest]are excluded by wildcard filters and still report the intended error when selected directly, in both modes. Added engine regressions for CI. Also fixed a nullable warning inIssue7116Tests.Validation: before the fix, the new class wildcard ran 3 tests with 2 unwanted failures in each mode; afterward it runs 1 passing normal test. Direct selection of each explicit method gives 1 intended arity failure in each mode.
TUnit.Engine.Testscompiled (not run locally per AGENTS.md). No public API or generator-output changes. The prior macOS ASP.NET Core disposal failure is tracked separately in #7408.