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
The nullable method-row regression fixture still disables TUnit0001 pending #7610, although its analyzer fix has merged. Remove the suppression and obsolete comment so the fixture also guards against analyzer regressions.
Rechecked #7573 against origin/main at 55f4100. #7613 (3dfd584) supplies compatible int/int? analyzer binding and constraint coverage; #7614 supplies generated/reflection discovery parity and constructor precedence. Both native children are closed and both fixes are ancestors of this branch. Their merged PRs record full analyzer/generator and Native AOT validation with passing CI.
CI follow-up: updated the nullable row generator snapshot to account for the two removed source lines (30 source-location entries; no other generated differences). The affected generator snapshot test passed on net10.0 after reproducing the failure. Initial multi-framework execution could not run net8.0/net9.0 because those runtimes are absent locally; net10.0 was run explicitly. git diff --check passed.
Summary by CodeRabbit
Tests
Updated generated test snapshots to report source line numbers two lines earlier for nullable class method-row cases.
Removed a warning suppression and its accompanying comment from a test fixture. The test declarations and implementation remain unchanged, and no changes to generated test invocation, data sources, or registration behavior are indicated.
No actionable comments were generated in the recent review.
Walkthrough
The nullable generic class method-row fixture no longer suppresses TUnit0001. Its verified generated test-entry metadata now lists source lines two lines earlier. The generated invocation, data-source, and registration logic are unchanged.
Changes: Nullable method-row regression
Layer / File(s)
Summary
Remove suppression and refresh generated metadata tests/TUnit.TestProject/Bugs/_7573/NullableClassMethodRows.cs, tests/TUnit.Core.SourceGenerator.Tests/NullableClassMethodRowsTests.Generates_Nullable_Class_From_Method_Rows.verified.txt
The fixture removes the TUnit0001 suppression and its related comment. The verified entries record source lines two lines earlier.
thomhurst/TUnit#7610 — The issue concerns the analyzer behavior tracked by the fixture’s removed TUnit0001 suppression.
thomhurst/TUnit#7611 — The issue concerns nullable generic method-row discovery in the same regression fixture.
Possibly related PRs
thomhurst/TUnit#7591 — Added the nullable generic class method-data-source validation and the regression fixture used here.
thomhurst/TUnit#7614 — Changed discovery for nullable generic class method rows in the same regression area.
thomhurst/TUnit#7613 — Fixed TUnit0001 validation for rows passed to a nullable generic class parameter.
Merge Risk: Minimal · up to ae746
This change only re-enables analyzer coverage and refreshes matching generated metadata.
Pre-merge checks: 4 · 1
Failed checks: 1 inconclusive
Check name
Status
Explanation
Resolution
Linked Issues check
Issue #7573 requires TUnit0001 to accept an IEnumerable method data source for a T? parameter on a generic class with where T : struct, and requires analyzer coverage. This PR does not change ana…
Confirm from repository evidence at head that the TUnit0001 analyzer accepts an int method source for a T? parameter on a generic class constrained with where T : struct. Identify the analyzer test file(s) that cover this case, or confirm t…
Passed checks: 4 passed
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title "Remove obsolete nullable method-row analyzer suppression" matches the main change. The PR removes the TUnit0001 suppression and its comment from NullableClassMethodRows.cs, and the generate…
Out of Scope Changes check
The whole-PR diff against merge base 55f4100 contains two files. The fixture change removes only the TUnit0001 suppression and its obsolete comment. This change directly supports the issue objective o…
Docstring Coverage
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Full details: Title check
Explanation
The title "Remove obsolete nullable method-row analyzer suppression" matches the main change. The PR removes the TUnit0001 suppression and its comment from NullableClassMethodRows.cs, and the generated verified snapshot shifts by two lines as a result. The title is specific and concise, and a teammate scanning history would understand the primary change.
Full details: Out of Scope Changes check
Explanation
The whole-PR diff against merge base 55f4100 contains two files. The fixture change removes only the TUnit0001 suppression and its obsolete comment. This change directly supports the issue objective of guarding against TUnit0001 regressions for this case. The snapshot change updates only the 30 source-location lineNumber values that shift by two lines after the removal. No generated invocation, data-source, or registration logic changes. No engine, generator, public API, or reflection code changes appear in the diff. No unrelated change was found.
Full details: Docstring Coverage
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. 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 0 functions across 0 files. (1 skipped: 1 unsupported.)
Full details: Linked Issues check
Explanation
Issue #7573 requires TUnit0001 to accept an IEnumerable<int> method data source for a T? parameter on a generic class with where T : struct, and requires analyzer coverage. This PR does not change analyzer code. Its diff changes only the fixture file tests/TUnit.TestProject/Bugs/_7573/NullableClassMethodRows.cs (removes the #pragma warning disable TUnit0001 line and its comment) and the generator snapshot NullableClassMethodRowsTests.Generates_Nullable_Class_From_Method_Rows.verified.txt (30 lineNumber entries, each shifted by -2). The fixture contains the reproduction case (NullableValue, MethodDataSource(nameof(NullableValues)) with IEnumerable<int?>, and Values/Factory/Tuple variants). Removing the suppression makes TUnit0001 active for these cases. The analyzer fix is reported as merged in #7613 and present in the merge base 55f4100. I could not read the TUnit0001 analyzer source or its tests at head within the inspection limit. Therefore I cannot independently confirm that the analyzer accepts int for T? at the reviewed head, or that analyzer coverage exists at head. The build and test results in the PR description are author-reported and were not verified from repository evidence.
Resolution
Confirm from repository evidence at head that the TUnit0001 analyzer accepts an int method source for a T? parameter on a generic class constrained with where T : struct. Identify the analyzer test file(s) that cover this case, or confirm that the fixture build with TUnit0001 enabled passes in CI. Then the assessment can be re-run to reach a verdict.
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 the rows at dawn, The warning’s quiet, the old lines gone. Two lines shift in the verified page, The tests still hop from stage to stage. Soft paws mark the source lines right, Then tuck the fixture in for night.
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: bc010613-f6ca-4d9d-ba7b-75676b3aa07f
Commits
Reviewing files that changed from the base of the PR and between 55f4100 and ae7465d.
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 5 remain after this review.
Review: LGTM. The change only removes the obsolete #pragma warning disable TUnit0001 and its comment from the nullable method-row fixture, now that the analyzer fix (#7610/#7613) has merged. This lets the fixture catch analyzer regressions. It changes no engine or generator code, so there is no dual-mode or AOT concern. The PR description records the targeted cases passing in generated and reflection modes. I did not build or run them myself. CI's analyzer build of the TestProject will confirm that no TUnit0001 diagnostic remains. No issues found.
The earlier wait on run 41968545548 completed successfully. The subsequent snapshot fix at ae7465d has positive current-head Claude and Codex verdicts and no review threads or blocking findings. Marked ready again with the head unchanged. Promotion started Claude review run https://github.com/thomhurst/TUnit/actions/runs/41975811189 and CI. Read the promotion review after it passes, then arm auto-merge with scripts/Enable-AutoMerge.ps1; the repository gate requires that promotion review before arming.
Review: LGTM. This removes the stale #pragma warning disable TUnit0001 and its comment now that the analyzer fix for #7610 has merged. The fixture now guards against analyzer regressions. It's a test-only change with no engine, generator or API impact. The PR description reports 39 targeted cases passing in both generated and reflection modes. CI should confirm the build has no TUnit0001 warnings, because warnings are errors in this repo. I have no concerns.
The previous Claude wait completed successfully. Fixed the CI snapshot failure in ae7465d: removing the obsolete two-line suppression shifts all 30 generated source locations by two. Verified that these are the only snapshot differences; the affected generator snapshot test now passes on net10.0. Returned to draft for fresh review.
The change removes the stale #pragma warning disable TUnit0001 and its comment, so the fixture now also guards against analyzer regressions now that #7610's fix has merged.
The snapshot update matches the source change. Two lines were removed from the top of the file, so every lineNumber in the verified file drops by exactly 2. The diff has no other generated differences.
No engine, generator or public API code changed, so no extra Native AOT or reflection-path concerns arise.
I didn't build the project or run the tests. The PR description says the 39 targeted _7573 cases pass in both generated and reflection modes, and CI will confirm the analyzer is clean without the suppression.
One minor thing: the snapshot's lineNumber values are brittle to any edit above the test methods. That's pre-existing and not worth changing here.
The change removes the obsolete #pragma warning disable TUnit0001 and its stale comment, now that #7610's analyzer fix has merged. The fixture therefore also guards against analyzer regressions.
The snapshot diff only shifts lineNumber by -2 across 30 entries. That matches the two removed source lines, and I saw no other generated differences.
No engine, generator or public API code changed, so the metadata-collection parity and AOT constraints aren't affected.
The PR description records generated and --reflection runs of the 39 targeted cases.
One optional point: the snapshot's lineNumber values will break again whenever the fixture's header changes. That's an existing characteristic of these snapshots, so I wouldn't change it in this PR.
The earlier blocker is cleared: promotion review run 41975811189 passed for ae7465d, with Claude LGTM and a completed, finding-free Codex verdict for the same head. All current CI checks passed; Assert-PrGreen reports 33 green checks and no unresolved threads.
Disposition of CodeRabbit's inconclusive linked-issue assessment: analyzer coverage is present at this head in MethodDataSourceAnalyzerTests.cs, including Nullable_Generic_Class_Parameter_Accepts_Value_Data with both int and int? rows for T? where T : struct, plus explicit-type and constructor-inferred cases. The Ubuntu pipeline passed its build and test pipeline with the suppression removed. No additional change is needed. Claude's optional note about snapshot line-number sensitivity describes existing behavior and is deferred from this PR as suggested by the reviewer.
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
The nullable method-row regression fixture still disables TUnit0001 pending #7610, although its analyzer fix has merged. Remove the suppression and obsolete comment so the fixture also guards against analyzer regressions.
Rechecked #7573 against origin/main at 55f4100. #7613 (3dfd584) supplies compatible int/int? analyzer binding and constraint coverage; #7614 supplies generated/reflection discovery parity and constructor precedence. Both native children are closed and both fixes are ancestors of this branch. Their merged PRs record full analyzer/generator and Native AOT validation with passing CI.
Closes #7573
Validation (SDK 10.0.403, net10.0, macOS arm64):
_7573cases passed in generated mode.--reflection.git diff --checkpassed.CI follow-up: updated the nullable row generator snapshot to account for the two removed source lines (30 source-location entries; no other generated differences). The affected generator snapshot test passed on net10.0 after reproducing the failure. Initial multi-framework execution could not run net8.0/net9.0 because those runtimes are absent locally; net10.0 was run explicitly.
git diff --checkpassed.Summary by CodeRabbit