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
Share evaluated constructor-row state across discovery modes - #7497
Generic constructor-row discovery now shares one internal EvaluatedDataRow implementation across generated partitions, inferred method sources, and reflection partitions. It hands out the already evaluated values once, then calls the original factory for fresh constructor data. Raw and grouped reflection sources share the same state instead of parallel first-fetch and factory arrays.
Wrapped row settings, null-row fallback, and disposal ownership retain their existing paths. Added coverage for shared state across sources and enumerations, plus null evaluated rows. No public API or generator-output changes.
Improved handling of evaluated rows in generic class data sources, keeping row results consistent across raw and grouped test data.
Null results are fetched again when requested, and shared evaluated rows are returned only once before the original data source is used for later requests.
No actionable comments were generated in the recent review.
Walkthrough
The change adds shared per-row state for evaluated rows and their original factories. Generic class partitions, method data sources, and reflection discovery use this state to coordinate row fetching.
Changes: Evaluated row lifecycle
Layer / File(s)
Summary
Shared row state and generic source consumers src/TUnit.Core/Attributes/TestData/EvaluatedDataRow.cs, src/TUnit.Core/Attributes/TestData/GenericClassRowPartition.cs, src/TUnit.Core/Attributes/TestData/MethodDataSourceAttribute.cs
EvaluatedDataRow stores an evaluated row and its factory. Generic partitions and filtered method data sources use FetchAsync to return a non-null evaluated row once, then call the factory on later fetches.
Reflection discovery and static data sources src/TUnit.Engine/Discovery/ReflectionTestDataCollector.cs, src/TUnit.Core/Attributes/TestData/StaticDataSourceAttribute.cs, tests/TUnit.UnitTests/GenericClassSourceRuntimeTests.cs
Reflection discovery stores evaluated rows and factories in EvaluatedRows, which static data sources use when creating row delegates. Tests cover repeated fetches when the evaluated result is null and sharing row state across raw and grouped sources.
Issue 7443: The change uses a shared per-row state for evaluated values and factories across generic row sources.
Issue 7446: The change provides shared evaluated-row runtime support, but does not implement generator inference.
Issue 7445: The change updates the runtime row lifecycle and shares evaluated rows across generic constructor sources.
Merge Risk: Minimal · up to c6d6f
No merge-blocking issue was identified; normal CI validation remains appropriate.
Pre-merge checks: 4 · 1
Failed checks: 1 warning
Check name
Status
Explanation
Resolution
Docstring Coverage
Docstring coverage is 50.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. Documenta…
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: sharing evaluated constructor-row state across discovery modes.
Linked Issues check
Directly linked issue #7443 has coding requirements only. Requirement 1 (one first-fetch implementation for generated and reflection discovery): met. The head adds EvaluatedDataRow in src/TUnit.Core…
Out of Scope Changes check
All changed files map to #7443: EvaluatedDataRow.cs (new shared state type), GenericClassRowPartition.cs (removes FirstFetchReturnsEvaluatedRow, uses EvaluatedDataRow), `MethodDataSourceAttrib…
Full details: Linked Issues check
Explanation
Directly linked issue #7443 has coding requirements only. Requirement 1 (one first-fetch implementation for generated and reflection discovery): met. The head adds EvaluatedDataRow in src/TUnit.Core/Attributes/TestData/EvaluatedDataRow.cs. GenericClassRowPartition.Coordinator.EvaluateAsync stores each evaluated row in EvaluatedDataRow and yields its FetchAsync. StaticDataSourceAttribute replaces FirstFetchRows and RowFactories with EvaluatedRows, which the raw source and its partitions share. MethodDataSourceAttribute.GetDataRowsAsync and ReflectionTestDataCollector.GroupClassRowsByTypeArgumentsAsync use the same type. Requirement 2 (consume the evaluated value once, then call the original factory): met. FetchAsync uses Interlocked.Exchange to take the row once and calls factory() on later calls. Requirement 3 (preserve wrapped TestDataRow settings, null-row behavior, and disposal ownership): met. EvaluateWrappedFactory is unchanged. A null evaluated row calls the factory on each fetch. The evaluation context and TestSessionContext.DeferOnDisposeToSessionEnd path is unchanged from the merge base. Requirement 4 (validation): the PR reports the runtime fixture runs in both discovery modes and the Native AOT publish. It also adds GenericClassSourceRuntimeTests coverage for shared state and null evaluated rows (+38 lines). Limits: the reflection and MethodDataSourceAttribute hunks were verified from the change summary and PR description, not from full source reads. No #7422 first-fetch closure is present at the reviewed head, so no additional path needed refactoring.
Full details: Out of Scope Changes check
Explanation
All changed files map to #7443: EvaluatedDataRow.cs (new shared state type), GenericClassRowPartition.cs (removes FirstFetchReturnsEvaluatedRow, uses EvaluatedDataRow), MethodDataSourceAttribute.cs (replaces the inline Interlocked.Exchange closure), StaticDataSourceAttribute.cs (replaces FirstFetchRows and RowFactories with EvaluatedRows), ReflectionTestDataCollector.cs (replaces StrongBox first-fetch state and parallel factory arrays), and GenericClassSourceRuntimeTests.cs (tests the shared state). The removed and added properties on StaticDataSourceAttribute are on an internal class, so the public API is unchanged. The evaluation-context isolation and session-end disposal code comes from the merge base (#7459), not from this PR. No unrelated change was found.
Full details: Docstring Coverage
Explanation
Docstring coverage is 50.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 6 functions across 6 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 stores one row with care, Its factory waits for fetches there. The first green leaf is passed along, Then fresh ones follow, crisp and strong. Null leaves prompt another try, While grouped rows hop by nearby.
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: 1bb18399-a99a-4d46-add6-9bede50504e1
Commits
Reviewing files that changed from the base of the PR and between 374bb7c and c6d6fc0.
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 1 remain after this review.
[Medium impact] The PR appears safe to merge; no actionable issues were found.
Summary
The PR adds EvaluatedDataRow to share constructor-row values and their original factory across discovery modes.
Generic constructor rows share one state across discovery and test runs.
No actionable issues were found. Tests were not run during this review.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Evaluated constructor row] --> S[EvaluatedDataRow]
B[Original factory] --> S
C[Generated or reflection source] --> F[FetchAsync]
F --> S
S --> D{Evaluated values remain?}
D -->|Yes| E[Return values once]
D -->|No| G[Call original factory]
I found no blocking issues. The change replaces three copies of the same logic with one type, and I checked that the behavior is preserved.
Checked
Single hand-out:Interlocked.Exchange hands the evaluated values out once. After that, calls go to the factory. This matches the old StrongBox and FirstFetchReturnsEvaluatedRow behavior.
Static rows with no wrapped factory:EvaluatedRows[i] is null, so they still return a clone. The fallback for a wrapped row, item?.ToArray(), is equivalent to the old CloneArguments shallow copy. It also gives each fetch its own array.
Grouping loop:.Row is read before any fetch happens, so the consumed-to-null state can't affect partitioning.
Tests: the null-row fallback and the case where raw sources, grouped sources and repeat enumerations share one state are both covered.
Minor suggestions (non-blocking)
Row is a live view that becomes null after the first fetch, which is easy to misuse. A short doc comment saying it is only valid before the first FetchAsync would help. Alternatively, rename it to something like PeekEvaluated.
FetchAsync is passed around as a method group, so each yield return state.FetchAsync allocates a delegate. This only happens at discovery and is fine. The new test builds two StaticDataSourceAttribute instances over one state, which is what discovery now does too.
Approving in spirit. Net result is less code and one place for the semantics.
Reviewed all review surfaces for 8bbcb7a. Claude and Codex both report no blocking findings; Greptile also found no actionable issues, and there are no inline threads.
Disposition of Claude’s non-blocking suggestions: the only Row read is in reflection grouping before any fetch, so its current use is safe; a property documentation refinement is optional and does not require a new head. Delegate creation is confined to discovery and needs no performance change without measurement. Retaining the reviewed implementation.
Marking ready to start CI. Promotion triggers a fresh Claude run; auto-merge must wait for that run and its review comment.
The earlier wait on run 41631205058 was still active, but main had also made this PR conflicting. Rebased onto 374bb7c (#7459) and pushed c6d6fc0. The conflict resolution retains the isolated evaluation context and session-end disposal from #7459, and constructs the shared EvaluatedDataRow inside that context. The reflection merge was also checked to preserve its cleanup paths.
Validation: net10.0 GenericClass*Tests passed 13/13 in generated mode and 13/13 with --reflection, including GenericClassRowPartitionCleanupTests. git diff --check passed. Engine.Tests was not run locally per repository instructions.
Returned to draft for fresh reviews of the rebased head; requested Codex and the synchronize event starts Claude. The old run is no longer the gate for this new head. No prerequisite or superseding PR was identified; #7443 is delivered by this PR and its Closes reference remains in the body.
This is a good consolidation. Three hand-rolled copies of the "hand out the evaluated values once, then call the factory" logic (Interlocked.Exchange on a captured local or a StrongBox) become one small internal type. The parallel FirstFetchRows and RowFactories arrays are replaced by a single EvaluatedRows array, which removes the risk of the two getting out of step. The new unit tests cover the shared-state, single-consumption and null-row cases. There is no public API change.
One thing to check. In ReflectionTestDataCollector, a row that is not a MethodDataSource row but had a wrapped factory evaluated now gets this fallback factory:
()=>Task.FromResult<object?[]?>(item?.ToArray())
The old code returned CloneArguments(_data[i]) as its fallback. That is a different clone from the shallow ToArray(), and it was created once, whereas now each fetch makes a new array. If CloneArguments does more than a shallow copy, the later fetches could see different aliasing than before. Please confirm the difference doesn't matter, or reuse the same cloning helper. Also, EvaluatedDataRow.FetchAsync doesn't handle a null factory, so it's worth keeping the factory non-null by construction, as it is here.
Minor.EvaluatedDataRow.Row is read during grouping and FetchAsync clears it. That's safe today because grouping finishes before any fetch. A short comment on Row saying so would help the next reader.
Nothing blocking. Approving in spirit, pending CI on the other target frameworks.
This is a clean consolidation. It replaces three copies of the "hand out the evaluated row once, then call the factory" logic (FirstFetchReturnsEvaluatedRow, the inline lambda in MethodDataSourceAttribute, and the StrongBox plus RowFactories arrays in StaticDataSourceAttribute) with one small type. I didn't run the tests; this is from reading the diff against the pr-head code.
What I checked:
Consume-once behaviour:Interlocked.Exchange is still the only hand-out point. The new test shows a row shared between the raw and grouped sources is handed out exactly once.
Null rows: a null evaluated row still falls through to the factory on every fetch, and the new test covers it.
Static fallback: the old fallback was CloneArguments, a shallow array copy. item?.ToArray() in the collector gives the same result. It now makes a fresh copy per fetch where before it shared one copy, which is harmless.
Group sharing: replacing the parallel FirstFetchRows and RowFactories arrays with one EvaluatedDataRow?[] removes the risk of the two arrays drifting out of sync.
Minor observations, none blocking:
Row becomes null once the row is consumed. The collector reads .Row at discovery, before any fetch, so it is fine today. A short doc comment on Row saying it is only valid before the first fetch would stop a later caller misreading it.
In GroupClassRowsByTypeArgumentsAsync the lambda () => Task.FromResult(item?.ToArray()) captures the loop-local item. That is correct, but it allocates a closure for every wrapped row. If this path is hot, a small static helper taking the row would avoid that. It is discovery-time only, so it's optional.
Public API and generator output are unchanged, as the description says. Reflection and generated modes both have test coverage, and the Native AOT publish was run. LGTM.
Run 41631205058 completed successfully and belongs to the older head; it is no longer a blocker. Current head c6d6fc0 has positive Claude and Codex verdicts, no inline comments or review threads, and no failing checks.
Verified the remaining review observations against the diff and CloneArguments: CloneArguments allocates an array and calls Array.Copy, so item?.ToArray() preserves its shallow copying semantics. A fresh fallback array per fetch does not change element ownership. Row is read only during grouping before fetching; its optional documentation suggestion and the optional discovery closure optimization are dispositioned without changing this reviewed head. The factory remains non-null by construction.
Promoting to ready. This starts CI and a fresh Claude review; auto-merge must wait for that review to pass and its comment to be checked. #7443 is fully covered and remains linked with Closes; there is no prerequisite, superseding PR, stale issue scope, or finished parent requiring action.
Nice consolidation: one EvaluatedDataRow replaces the StrongBox array plus parallel RowFactories array and the two duplicated inline Interlocked.Exchange lambdas. The hand-out-once semantics are preserved. The tests cover the shared-state and null-row cases.
Two minor points, neither blocking:
Fallback no longer clones. In ReflectionTestDataCollector, the non-factory fallback is now () => Task.FromResult(item?.ToArray()). The old path in StaticDataSourceAttribute used CloneArguments(_data[i]). If CloneArguments does more than a shallow copy, repeat fetches of wrapped rows with no rowFactories now get a shallower copy than before. Please confirm the two are equivalent, or reuse CloneArguments.
Row goes null after the first fetch.EvaluatedDataRow.Row returns null once the row has been consumed, but it is only read during grouping, before any fetch. A short doc comment saying so would stop a later caller from reading it after a fetch and silently falling back to dataItems[rowIndex].
Otherwise this looks good, and it cuts duplicate code with no public API change.
Run 41633685555 completed successfully on c6d6fc0, so the earlier blocker is cleared. Read every review surface again: Codex completed with no findings for this head; Claude's post-promotion review has no blocking findings; Greptile and CodeRabbit report no actionable findings. There are no inline comments or review threads.
Confirmed Claude's repeated clone question against this head: StaticDataSourceAttribute.CloneArguments creates an object array and calls Array.Copy; item?.ToArray() likewise makes a shallow array copy. Both retain the same element references. The new fallback creates a fresh array per fetch, without changing element ownership. Row is read in the grouping loop before FetchAsync can consume it. The optional Row documentation, discovery allocation suggestion, and CodeRabbit docstring coverage warning are non-blocking; retaining the reviewed implementation. No reviewer has challenged the earlier evidence or requested a required change.
The merge gate passed: CLEAN, 34 checks green, no unresolved threads. Live protection requires eight CI checks and no formal approval; all eight passed. The documentation discrepancy is already tracked in #7444. #7443 remains fully delivered by this PR with Closes in the body; no prerequisite or replacement requires separate action. Proceeding through Merge-Pr.ps1.
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
Error
Summary
Generic constructor-row discovery now shares one internal
EvaluatedDataRowimplementation across generated partitions, inferred method sources, and reflection partitions. It hands out the already evaluated values once, then calls the original factory for fresh constructor data. Raw and grouped reflection sources share the same state instead of parallel first-fetch and factory arrays.Wrapped row settings, null-row fallback, and disposal ownership retain their existing paths. Added coverage for shared state across sources and enumerations, plus null evaluated rows. No public API or generator-output changes.
Validation
GenericClassSourceRuntimeTests: 11 passed._7210in generated and reflection modes: each 15 passed, 3 skipped, 1 intentional failure (19 total), including wrapped factory reuse/freshness._7223null/array fixtures in both modes: each 13 passed._7208: generated 39 passed / 4 discovery failures; reflection 42 passed / 1 intentional discovery failure. The additional generated class-inference failures remain tracked in Source-gen: generic test class does not infer its type arguments from a class-level [MethodDataSource]; reflection does #7333; evidence added there.-m:1. Published native_7210: 15 passed, 3 skipped, 1 intentional failure;_7223: 13 passed. Initial parallel publish lost MSBuild child nodes; the serial publish succeeded. Existing fixture/hook preservation warnings remain tracked in Regression test fixtures emit Native AOT preservation warnings #7493 and Generated instance hooks emit IL2075 during Native AOT publishing #7430.Closes #7443
Summary by CodeRabbit