fix(engine): reuse the first class-data fetch for the first test of a class row by thomhurst · Pull Request #7195 · thomhurst/TUnit

thomhurst · 2026-10-20T05:42:02Z

Summary

Before: for each class data row, TestBuilder called the class data factory once by itself, before it evaluated the method data sources, and then once more for every test. With SharedType.None, the first call created an extra instance per class row. No test held that instance, so it was never tracked or disposed. In the issue's repro, two tests created four instances and only the two the tests used were disposed.

After: each class row creates one instance per test. Each test holds its instance, and the instance is disposed with the test. The extra instance's constructor and IAsyncDiscoveryInitializer no longer run.

Cause

The objects from the first fetch were used only to evaluate the method data sources (classInstanceArguments and the instance for instance-data sources). After that, every test fetched the row again. Only the instances from those later fetches reached TestData.ClassData, so only they were tracked. The streaming path (BuildTestsStreamingAsync / BuildSingleTestAsync) had the same pattern.

Fix

A small UnclaimedClassRow holder keeps the objects of the first fetch. The first test built from the row takes them instead of calling the factory again. Later tests fetch as before, so each SharedType.None test still gets its own instance.
If a row is skipped because it does not fit a generic method's expected types, its class objects go back to the holder and the next test takes them. Before, they were dropped untracked.
The SkipIfEmpty test of a method data source already holds classData, so building it clears the holder.
The batch path and the streaming path both use the holder. They are shared by source-generated and reflection modes, so both modes behave the same.

This builds on #7150 (failed rows). Disposal for skipped tests and rows is tracked separately in #7156 and is not changed here.

Known remaining edge case: if no test is built from a class row (all method data sources yield nothing and none uses SkipIfEmpty), the first fetch still has no owner, as before. It cannot be disposed safely at build time, because a shared or keyed instance may still be used by tests that are not registered yet.

Tests

New tests/TUnit.TestProject/Bugs/_7157: two SharedType.None classes, one with two tests and one with two [Arguments] rows run twice with [Repeat(1)]. An observer checks that the number of instances created equals the number of tests and that all of them are disposed. Before the fix it failed with Expected to be 2 but found 4 in both source-gen and --reflection modes. After the fix it passes in both modes.
New tests/TUnit.Engine.Tests/Issue7157Tests wraps that run. Issue7123Tests still passes.
Full TUnit.Engine.Tests (net10.0): 468 passed, 264 skipped (AOT/OS-gated), 1 failed. The failure is FSharp.Test, from a local SDK issue: with the 11 RC SDK, TUnit.Assertions.FSharp binds FSharp.Core 11 while the test project ships 10.1. It is unrelated to this change, and I filed it as TUnit.Assertions.FSharp picks up the building SDK's FSharp.Core, so F# tests fail to load FSharp.Core 11 on an 11 RC SDK #7193.
TUnit.UnitTests (net10.0): 608 passed, 4 skipped.

Fixes #7157

Review follow-ups (`73d135d`)

When an instance method data source needs a temporary instance, that instance is built from the first fetch and disposed after the method rows are read. In that case the first fetch now stays with the temporary instance, and every test fetches its own row. Before this follow-up, a class that disposed its injected fixture closed the first test's fixture. New regression test: InstanceMethodDataSourceDisposingFixtureTests. Issue7157Tests now expects 9 tests.
Found while checking a review finding: builder-context Events subscribed during a row fetch never reach the test. This means DependencyInjectionDataSourceAttribute scopes are never disposed. It also happens on main, so it is filed separately as DependencyInjectionDataSourceAttribute scopes are never disposed: builder-context events never reach the test #7205.

Summary by CodeRabbit

Bug Fixes
- Improved lifecycle handling for class-based test data, including data rows that are not received or used by a test.
- Prevented temporary data-source instances from being initialized unnecessarily while ensuring they are still disposed correctly.
- Preserved and reused fetched class-data rows to avoid creating extra untracked instances.
- Applied consistent disposal tracking across skipped, incompatible, generic-resolution-failure, constraint-violation, and streaming test scenarios.
Tests
- Added regression coverage verifying that non-shared class-data instances are correctly associated with tests and disposed.

… class row The test builder fetches each class data row once before it evaluates the method data sources, then fetched it again for every test. With SharedType.None the first fetch created an extra instance per class row that no test held, so the object tracker never disposed it. The first test built from a class row now takes the objects of that first fetch instead of calling the factory again, and a row skipped as incompatible with a generic method hands its class objects to the next test. Both the batch and streaming build paths do this, so source-generated and reflection modes behave the same. Fixes #7157

coderabbitai · 2026-10-20T05:42:11Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 31716238-fa2b-4258-9c3b-9a2644be6ff5

📥 Commits

Reviewing files that changed from the base of the PR and between c4b9977 and 4891a64.

📒 Files selected for processing: 8

src/TUnit.Core/Discovery/ObjectGraphDiscoverer.cs
src/TUnit.Core/TestContext.cs
src/TUnit.Core/TestDetails.cs
src/TUnit.Engine/Building/TestBuilder.cs
src/TUnit.Engine/Services/ObjectLifecycleService.cs
src/TUnit.Engine/Services/TestArgumentRegistrationService.cs
tests/TUnit.Engine.Tests/Issue7157Tests.cs
tests/TUnit.TestProject/Bugs/_7157/FirstClassDataFetchTests.cs

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 4 remain after this review.

📝 Walkthrough

Test construction now reuses the first fetched class-data row or tracks its objects when temporary instances consume it. Unreceived row objects remain tracked for disposal but are excluded from execution-time initialization. Regression tests cover standard and streaming construction paths.

Changes: Class-data lifecycle

Layer / File(s)	Summary
Class-row reuse across test construction `src/TUnit.Engine/Building/TestBuilder.cs`	`TestBuilder` holds and reuses the first class-data row. It preserves objects used by temporary instances across successful, failed, skipped, and streaming test paths.
Unreceived-row disposal tracking `src/TUnit.Core/Discovery/ObjectGraphDiscoverer.cs`, `src/TUnit.Core/TestContext.cs`, `src/TUnit.Core/TestDetails.cs`, `src/TUnit.Engine/Services/ObjectLifecycleService.cs`, `src/TUnit.Engine/Services/TestArgumentRegistrationService.cs`	Unreceived row objects are recorded as disposal-only tracked objects. Object initialization skips them, and argument registration checks for unreceived row objects.
Non-shared class-data regression coverage `tests/TUnit.TestProject/Bugs/_7157/FirstClassDataFetchTests.cs`, `tests/TUnit.Engine.Tests/Issue7157Tests.cs`	Tests verify class-data instance counts, initialization, and disposal. The engine test runs the filtered test set in both modes.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 4891a

No actionable risk introduced by this change remains after normal checks. The no-tests-built ownership gap predates this PR.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 36 functions across 8 files.	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 primary change: reusing the first class-data fetch for the first test of a class row. This matches the pull request objectives and implementation.
Linked Issues check		Issue `#7157` requires removal of the extra untracked class-data instance for SharedType.None. TestBuilder now stores the first class-row fetch in UnclaimedClassRow and gives it to the first test. It ap…
Out of Scope Changes check		The changes remain connected to `#7157`. The generalized UnreceivedDataRowObjects tracking supports class-row objects that the first fetch, a temporary instance, or a failed row does not pass to a test.…

Full details: Linked Issues check

Explanation

Issue #7157 requires removal of the extra untracked class-data instance for SharedType.None. TestBuilder now stores the first class-row fetch in UnclaimedClassRow and gives it to the first test. It applies the same logic to batch and streaming construction. The logic is shared by source-generated and reflection modes. It also tracks objects that a temporary instance or failed row does not pass to a test. New _7157 tests verify instance counts, disposal, initialization, method-data rows, and instance method data sources in both modes. The reported FSharp.Test failure is unrelated to this change.

Full details: Out of Scope Changes check

Explanation

The changes remain connected to #7157. The generalized UnreceivedDataRowObjects tracking supports class-row objects that the first fetch, a temporary instance, or a failed row does not pass to a test. Object tracking and lifecycle changes are required to dispose those objects without execution-time initialization. The new tests directly cover the class-row reuse and temporary-instance cases. No unrelated product behavior is demonstrated.

Fix all pre-merge checks with AI

✨ Finishing Touches: 💡 1

📝 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.

❤️ Share

_{A rabbit fetched a row at dawn
Then kept its class-data from being gone
The test used one, the rest stayed tracked
For disposal, none fell through the crack
The fixtures hopped, all counted right}

_{Comment @coderabbitai help to get the list of available commands.}

chatgpt-codex-connector · 2026-10-20T05:42:15Z

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-20T05:44:37.438710Z	`a9eb88b`	PR opened

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a9eb88bb6c

ℹ️ 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".

greptile-apps · 2026-10-20T05:47:19Z

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge; the previous initialization bug is fixed, with no new blocking issues found.

Summary

The PR reuses the first class-data fetch where possible and tracks temporary-instance fixtures without passing them to runnable tests.

The first class-data row now supplies the first test.
Temporary class-data objects are tracked without being handed to tests.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[First class-data fetch] --> B{Temporary instance needed?}
  B -->|No| C[First test receives the objects]
  B -->|Yes| D[Temporary instance uses the objects]
  D --> E[First test tracks them for disposal only]
  D --> F[Runnable tests fetch their own objects]
  C --> G[Initialize received objects]
  F --> G
  E --> H[Skip execution initialization]
  G --> I[Reference-counted cleanup]
  H --> I

_{Reviews (4) · Last reviewed commit: "fix(engine): do not initialize class-row..." · Reviewed by Greptile}

…nce that uses it An instance method data source needs a temporary instance of the test class. The builder creates it from the class row's first fetch and disposes it once the method rows are read. The first test took those same objects, so a class that disposed its injected fixture closed the first test's fixture before the test ran. When a temporary instance is created, it now owns the first fetch, and every test fetches its own row, as before #7157. Adds a regression test in which the test class disposes its fixture and reads it through an InstanceMethodDataSource.

An instance method data source needs a temporary instance of the test class, built from the class row's first fetch. Since the previous commit no test takes that fetch, so its objects were disposed only if the test class disposed its own constructor arguments. The fetch is now lent to the temporary instance. The next test built from the class row, whether it passes, fails or is skipped, tracks those objects without receiving them, so the object tracker disposes them when that test completes. TestDetails.FailedDataRowObjects is renamed to UnreceivedDataRowObjects, since it now also holds these objects. Adds a regression test with a test class that does not dispose its fixture.

…y instance used Objects a test tracks without receiving them (UnreceivedDataRowObjects) were added to TrackedObjects, and execution initializes every tracked object. A fixture lent to the temporary instance for an instance method data source was therefore initialized after that instance had disposed it. Discovery now records the objects reachable only from unreceived rows, and execution skips them while disposal still counts them.

thomhurst · 2026-10-20T08:22:36Z

Review dispositions for this round:

Greptile, "Disposed fixtures start again": confirmed with a failing regression test and fixed in 4891a64. Row objects that a test tracks without receiving them are no longer initialized during execution; they are still disposed through reference counting. Bugs._7157 passes 13 of 13 in both modes, and Issue7157Tests and Issue7123Tests pass.

github-actions · 2026-10-20T09:18:54Z

Review of #7195

I reviewed the diff statically. I did not build the PR or run its tests.

The fix targets the right cause. The first class-row fetch is now held in UnclaimedClassRow, and the first test built from the row takes it instead of calling the factory again. That removes the untracked extra instance. The batch and streaming paths use the same holder, so source-generated and reflection modes behave the same. The new Issue7157 repro covers both modes.

I checked that LendToTemporaryInstance runs once per class row. Calling it twice would drop the lent objects, but it doesn't happen in either path. The DisposalOnlyTrackedObjects skip in ObjectLifecycleService also stops execution from initializing objects the temporary instance may already have disposed. That is the main risk, and it is handled.

Suggestions (non-blocking):

UnclaimedClassRow is an implicit state machine. _data and _lent are cleared by Clear, Lend, Take and Return, and callers must call them in the right order. If a future call path forgets Return or TakeLentObjects, an object leaks silently. Consider asserting in debug builds, or in tests, that the holder is empty when the class row loop finishes. That would also make the known edge case (no test built from a row) detectable.
WithLentObjects has a side effect (it takes the lent objects) but reads like a pure helper. A name such as TakeLentAndCombine would make the mutation clearer at call sites.
TrackUnreceivedDataRowObjects copies every visited bucket into a HashSet per test, but only when unreceived objects exist, so the cost is acceptable. A short comment saying so would help.

Overall this is a solid, well-documented fix.

) * fix(engine): use UnreceivedDataRowObjects in skipped-test tracking #7195 renamed TestDetails.FailedDataRowObjects to UnreceivedDataRowObjects, and #7191 merged afterwards with a use of the old name, so main no longer compiles (CS1061 in TestArgumentRegistrationService). The CloudShop Example Tests workflow fails at its Build step on every main push since then. * ci: give the macOS AOT test-app publish a 60-minute module timeout Since #7131 the publish builds into its own artifacts tree and overlaps the test modules. On the 3-core macOS runner it now takes 24-27 minutes when it passes, and the default 30-minute ModularPipelines timeout killed it on six .NET runs on 2026-10-20, failing those PRs' macOS jobs.

thomhurst had a problem deploying to Pull Requests October 20, 2026 05:42 — with GitHub Actions Error

chatgpt-codex-connector Bot reviewed Oct 20, 2026

View reviewed changes

Comment thread src/TUnit.Engine/Building/TestBuilder.cs Resolved

greptile-apps Bot reviewed Oct 20, 2026

View reviewed changes

Comment thread src/TUnit.Engine/Building/TestBuilder.cs Resolved

thomhurst had a problem deploying to Pull Requests October 20, 2026 06:03 — with GitHub Actions Error

thomhurst mentioned this pull request Oct 20, 2026

DependencyInjectionDataSourceAttribute scopes are never disposed: builder-context events never reach the test #7205

Closed

greptile-apps Bot reviewed Oct 20, 2026

View reviewed changes

Comment thread src/TUnit.Engine/Building/TestBuilder.cs Resolved

thomhurst had a problem deploying to Pull Requests October 20, 2026 07:01 — with GitHub Actions Error

greptile-apps Bot reviewed Oct 20, 2026

View reviewed changes

Comment thread src/TUnit.Engine/Building/TestBuilder.cs Resolved

thomhurst had a problem deploying to Pull Requests October 20, 2026 08:22 — with GitHub Actions Failure

thomhurst temporarily deployed to Pull Requests October 20, 2026 08:22 — with GitHub Actions Inactive

thomhurst merged commit 4773281 into main Oct 20, 2026
24 of 25 checks passed

thomhurst deleted the fix/issue-7157-class-data-first-fetch branch October 20, 2026 09:52

thomhurst mentioned this pull request Oct 20, 2026

fix: restore main build and stop the macOS AOT publish timing out #7216

Merged

Navigation Menu

Repository navigation

fix(engine): reuse the first class-data fetch for the first test of a class row - #7195

fix(engine): reuse the first class-data fetch for the first test of a class row#7195
thomhurst merged 4 commits into
mainfrom
fix/issue-7157-class-data-first-fetch

thomhurst commented Oct 20, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Oct 20, 2026 •

edited

Loading

Uh oh!

chatgpt-codex-connector Bot commented Oct 20, 2026 •

edited

Loading

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

Uh oh!

greptile-apps Bot commented Oct 20, 2026 •

edited

Loading

Diagram

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thomhurst commented Oct 20, 2026

Uh oh!

github-actions Bot commented Oct 20, 2026

Uh oh!

Uh oh!

This branch was previously deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

thomhurst commented Oct 20, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Cause

Fix

Tests

Review follow-ups (73d135d)

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chatgpt-codex-connector Bot commented Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codex Review Summary

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

greptile-apps Bot commented Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Confidence Score: 5/5

Summary

Diagram

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thomhurst commented Oct 20, 2026

Uh oh!

github-actions Bot commented Oct 20, 2026

Uh oh!

Uh oh!

This branch was previously deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

thomhurst commented Oct 20, 2026 •

edited by coderabbitai Bot

Loading

Review follow-ups (`73d135d`)

coderabbitai Bot commented Oct 20, 2026 •

edited

Loading

chatgpt-codex-connector Bot commented Oct 20, 2026 •

edited

Loading

greptile-apps Bot commented Oct 20, 2026 •

edited

Loading