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
Discovery and execution use BuildTestsFromMetadataAsync, while an unused streaming implementation duplicated its row-building logic. Remove BuildTestsStreamingAsync from the internal builder interface and implementation, together with its private BuildSingleTestAsync helper.
Keep the existing constructor event, repeat state isolation, and enumeration state regression tests on the production builder path, removing only the streaming variants.
Validation: all 22 ClassRowCleanupTests passed on .NET SDK 10.0.403 / net10.0 in both default and --reflection modes. git diff --check passed. Engine integration tests are left to CI per repository instructions. No public API or generator output changes.
thomhurst/TUnit#7379 — The issue identifies removal of the unused streaming builder as a prerequisite for its cleanup-owner refactor.
thomhurst/TUnit#7454 — The issue covers both builder paths; this PR removes the streaming path and leaves the buffered path.
Possibly related PRs
thomhurst/TUnit#7384 — Changes row-event handling in the same streaming and non-streaming builder paths.
thomhurst/TUnit#7328 — Changes row-event handling in both builder paths that this PR addresses.
thomhurst/TUnit#7476 — Changes row contexts and state isolation in both builder paths, including tests updated here.
Merge Risk: Minimal · previous review 59a38
The change removes unused construction code while retaining the relevant production-path regression coverage.
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: removing the unused streaming test builder path.
Linked Issues check
[#7327] The issue asks to remove the unused streaming path and its helper, or to wire it up and test it. The PR removes the path. At head, src/TUnit.Engine/Building/Interfaces/ITestBuilder.cs no longe…
Out of Scope Changes check
All changed code falls within #7327. The removals in ITestBuilder.cs and TestBuilder.cs delete the unused streaming method and its private helper, as the issue requests. The ClassRowCleanupTests.cs ed…
Full details: Linked Issues check
Explanation
[#7327] The issue asks to remove the unused streaming path and its helper, or to wire it up and test it. The PR removes the path. At head, src/TUnit.Engine/Building/Interfaces/ITestBuilder.cs no longer declares BuildTestsStreamingAsync. The diff for src/TUnit.Engine/Building/TestBuilder.cs removes BuildTestsStreamingAsync (including its generic-instantiation recursion) and BuildSingleTestAsync, with +0/-336 lines. Discovery and execution keep using BuildTestsFromMetadataAsync, which the PR does not change. The issue's grep at base found no callers outside the interface, the implementation, and the recursion, and the PR removes all three. tests/TUnit.UnitTests/ClassRowCleanupTests.cs at head shows the constructor-event regression (InstanceConstructorSubscriptionsReachExecutableRow) now parameterless and calling BuildTestsFromMetadataAsync. The repeat-state and enumeration-state regressions also keep the production path, with their streaming parameter removed. The diff does not add new coding requirements. Limitation: the test file and the TestBuilder.cs diff were read in part because of output truncation. The line counts match the stated totals, and no remaining reference was found in the visible evidence.
Full details: Out of Scope Changes check
Explanation
All changed code falls within #7327. The removals in ITestBuilder.cs and TestBuilder.cs delete the unused streaming method and its private helper, as the issue requests. The ClassRowCleanupTests.cs edits only drop the streaming variant from three tests. Those tests keep their constructor-event, repeat-isolation, and enumeration-state checks on BuildTestsFromMetadataAsync. The PR does not change production behavior, public API, or generator output, as the PR description states and the diff shows. The PR objectives mention auto-merge scripts and review process. These are process notes, not code changes, so they do not create out-of-scope code.
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 3 functions across 1 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 the builder’s trail The streaming branch is gone The buffered tests now follow one path Row-cleanup checks carry on The rabbit files the change away
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: c5da4452-4f36-433e-92da-07b9e1bdbdfe
Commits
Reviewing files that changed from the base of the PR and between 10a0af7 and 59a389f.
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 5 remain after this review.
grep finds no remaining references to BuildTestsStreamingAsync or BuildSingleTestAsync in the PR head. Removing the method from the internal ITestBuilder and from TestBuilder breaks nothing else.
The two helpers are removed together with their [RequiresUnreferencedCode] annotations. The AOT/trimming surface shrinks and nothing is added.
The tests now cover only the production path, BuildTestsFromMetadataAsync. The streaming parameters are gone and the remaining cases are unchanged. DataSources = [] replaces ArgumentsAttribute(streaming) in the constructor-subscription test, and the repeat-isolation test already used DataSources = []. That is consistent, because the test no longer needs the bool argument and an empty source still yields one row (Count == 1).
This is an internal-only change, so there is no public API or generator snapshot impact.
I didn't run the tests. I'm relying on the PR's reported ClassRowCleanupTests runs and on CI. Approving from my side.
Marked ready after positive Claude and Codex verdicts on 59a389f; no inline findings or unresolved threads. Promotion triggered Claude review run 41759062387 and CI. Auto-merge must wait for that new Claude run to pass and its comment to be read, then use scripts/Enable-AutoMerge.ps1.
No references to BuildTestsStreamingAsync or BuildSingleTestAsync remain in pr-head (src, tests, docs). The removed code was only used by the tests that this PR updates.
Deleting the duplicate row-building path removes a real risk: two copies of subtle logic (unclaimed class rows, lent objects, constraint-violation unpacking) that could drift from the production BuildTestsFromMetadataAsync path. The RequiresUnreferencedCode surface also gets smaller.
The tests now exercise only the production path and keep their coverage of constructor events, repeat isolation and enumeration state.
Minor note: in InstanceConstructorSubscriptionsReachExecutableRow, DataSources = [new ArgumentsAttribute(streaming)] became DataSources = []. This relies on the production builder emitting exactly one row for an empty data source. The PR says the tests passed in both modes, so I take that as confirmed. Keeping a trivial [new ArgumentsAttribute()] would pin the single-row intent more explicitly, but it is not required.
The blocker has cleared: #7557 merged as 515f198 and AOT run 41776575570 completed successfully. Rebased this PR onto origin/main including that fix and pushed d8c016a. git range-diff confirms the PR patch is unchanged.
Validation on the rebased head: all 22 ClassRowCleanupTests passed in default mode and all 22 passed with --reflection (SDK 10.0.403, net10.0, macOS arm64). git diff --check passed. Engine integration tests remain for CI per repository instructions. Existing build warnings are tracked by #7410 and #7438.
Fresh CI and current-head Claude/Codex reviews are required before arming auto-merge. Every review surface was checked: no inline comments or review threads; prior comments have no blocking findings. CodeRabbit's existing-test docstring suggestion and Claude's optional explicit empty Arguments suggestion remain nonblocking. The earlier waiting status is superseded by this pushed fix.
The earlier blocker has cleared: #7549 merged and run 41766395044 passed. Rebased onto origin/main including its snapshot fix and pushed caa2377; range-diff confirms the PR patch is unchanged.
Validation on the rebased head: all 22 ClassRowCleanupTests passed in default mode and all 22 passed with --reflection (SDK 10.0.403, net10.0). git diff --check passed. Engine integration tests remain for CI per repository instructions. No remaining references to the removed streaming methods were found under src or tests.
Fresh CI and current-head Claude/Codex reviews are now required before arming auto-merge. Prior review findings were already dispositioned and there are no unresolved threads.
This removes the unused BuildTestsStreamingAsync and its private BuildSingleTestAsync helper. I grepped the PR head and found no remaining references to either name in code or docs.
Every helper the removed code used is still called by the production BuildTestsFromMetadataAsync path. That covers UnpackConstraintViolation, TryInferClassGenericsFromMethodData, CreateFailedTestForInstanceDataSourceError and CreateFailedTestForClassDataSourceCircularDependency. Nothing is left orphaned.
The test changes are sound. The streaming parameter is dropped and the production-path variants are kept. In InstanceConstructorSubscriptionsReachExecutableRow, the ArgumentsAttribute(streaming) data source was only a variant selector, so DataSources = [] is fine. The test still yields one test, as it asserts.
It adds no reflection or AOT surface. It only deletes a RequiresUnreferencedCode member. Removing the dead duplicate also stops the two builders from drifting apart.
I didn't build or run the tests. The description says all 22 ClassRowCleanupTests passed in both modes, and TUnit.Engine.Tests is left to CI.
This is a clean dead-code removal. I found no blocking issues.
A grep of pr-head/ finds no remaining references to BuildTestsStreamingAsync or BuildSingleTestAsync. The only hits are in the base checkout, which this PR deletes.
The change deletes the interface member, its implementation (~336 lines), and the private helper. That removes a second copy of the row-building logic, which could drift from BuildTestsFromMetadataAsync. It is a real maintainability gain, and it matches the repo rule that metadata collection should have a single shared path.
The interface is internal, so there is no public API impact.
The tests keep the production-path variants and drop only the streaming ones. This still covers the constructor-event, repeat-isolation and enumeration-state regressions.
One small thing to check: InstanceConstructorSubscriptionsReachExecutableRow changed DataSources from [new ArgumentsAttribute(streaming)] to []. This is fine if the test's MethodMetadata is built as a no-data row (the PR reports the tests passing). A short comment saying the empty data source is intentional would help a future reader who wonders why the method's bool parameter is no longer supplied. It is optional.
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
Discovery and execution use
BuildTestsFromMetadataAsync, while an unused streaming implementation duplicated its row-building logic. RemoveBuildTestsStreamingAsyncfrom the internal builder interface and implementation, together with its privateBuildSingleTestAsynchelper.Keep the existing constructor event, repeat state isolation, and enumeration state regression tests on the production builder path, removing only the streaming variants.
Validation: all 22
ClassRowCleanupTestspassed on .NET SDK 10.0.403 / net10.0 in both default and--reflectionmodes.git diff --checkpassed. Engine integration tests are left to CI per repository instructions. No public API or generator output changes.Closes #7327
Summary by CodeRabbit