Fix shared fixture lifetime for reused discovery instances

[thomhurst]

Summary

• cache injected properties even when a test reuses the discovery instance
• add a unit regression for reused discovery instance property tracking
• add a functional repro for [Bug]: Fixture is disposed before the last test case is run #5982 under TUnit.TestProject/Bugs/5982
• mark the functional repro classes with [EngineTest(ExpectedResult.Pass)] so they run in the engine regression suite

Tests

• dotnet test --framework net10.0 --treenode-filter "/*/*/PropertyInjectorTests/*"
• dotnet test --framework net10.0 /p:DisableGitVersionTask=true /p:GenerateGitVersionInformation=false --treenode-filter "/*/*/TUnit.TestProject.Bugs._5982.SomeTest/*|/*/*/TUnit.TestProject.Bugs._5982.AnotherTest/*"
• dotnet test --framework net10.0 /p:DisableGitVersionTask=true /p:GenerateGitVersionInformation=false --treenode-filter "/*/*/TUnit.TestProject.Bugs._5982.SomeTest/*[EngineTest=Pass]|/*/*/TUnit.TestProject.Bugs._5982.AnotherTest/*[EngineTest=Pass]"

Closes #5982

[claude]

Code Review: Fix shared fixture lifetime for reused discovery instances

Summary

The fix is correct, minimal, and well-targeted. The root cause — skipping property caching entirely when IsDiscoveryInstanceReused is true — made a logical category error: it conflated "already initialized" with "already cached for ref-counting." Removing the early exit is the right call.

---

Core fix (PropertyInjector.cs)

The removal is clean. The idempotency guard in ResolveAndCacheSourceGeneratedPropertyAsync (and its reflection counterpart) already prevents double-resolution:

if (testContext.Metadata.TestDetails.TestClassInjectedPropertyArguments.ContainsKey(cacheKey))
    return;

So removing the outer guard is safe — shared fixture instances won't be created twice. The replacement comment explains the why clearly.

One thing to verify (not a blocker): IsDiscoveryInstanceReused now has no callers that affect ResolveAndCachePropertiesAsync. If the flag is no longer needed anywhere else in the engine for this purpose, it may be worth noting in a follow-up whether it can be retired or repurposed to avoid misleading future readers.

---

Unit test (PropertyInjectorTests.cs)

The test directly pins the regression: setting IsDiscoveryInstanceReused = true and asserting properties are still cached. That's exactly right.

Observation — CreateContext<T> verbosity: The helper is around 40 lines of boilerplate. If more PropertyInjector unit tests are added in future, this will be duplicated. Consider extracting it into a TestContextBuilder or TestContextFactory utility (either in a Helpers/ sub-folder or a base test class). Not required for this PR, but worth a note.

Minor: PassthroughInitializationCallback only returns the object unchanged, so the unit test doesn't exercise the shared-instance lookup path (i.e., it won't pick up an already-registered session fixture). This is fine for a unit test — the functional test in FixtureLifetimeTests.cs covers the end-to-end path — but a comment explaining the intentional simplification would make the test easier to reason about for the next reader.

---

Functional repro (FixtureLifetimeTests.cs)

The structure is correct and the repro scenario matches #5982. One thing is non-obvious without reading the issue: the Wait1–Wait4 tests and AnotherTest exist purely to create the scheduling conditions that cause the fixture to be disposed prematurely. A short comment at the top of the file (or on the class) would help future readers understand why these seemingly-unrelated tests are present in a bug repro file.

// Wait tests + AnotherTest exist to force scheduling conditions that trigger premature fixture disposal.
// See https://github.com/thomhurst/TUnit/issues/5982

---

Verdict

Good fix. The change is minimal, safe (thanks to existing idempotency guards), and backed by both a unit regression test and a functional repro. The suggestions above are all minor quality-of-life improvements for future maintainers, not blockers.

[codacy-production]

Not up to standards ⛔

🔴 Issues 5 medium

Alerts:
⚠ 5 issues (≤ 0 issues of at least minor severity)

Results:
5 new issues

Category	Results
BestPractice	5 medium

View in Codacy

🟢 Metrics 6 complexity

Metric	Results
Complexity	6

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[claude]

Code Review: Fix shared fixture lifetime for reused discovery instances

Summary

This is a clean, targeted bug fix that correctly addresses the root cause of #5982 — shared fixtures being prematurely disposed when data-driven tests reuse the discovery instance.

The Fix (PropertyInjector.cs)

The removal of the early return is correct. The original guard was overly broad:

// Before (incorrect): skips ALL property resolution when discovery instance is reused
if (testContext.IsDiscoveryInstanceReused)
    return Task.CompletedTask;

The intent was to avoid redundant work, but it also prevented ref-count tracking for shared fixtures. The fix is safe because ResolveAndCacheSourceGeneratedPropertyAsync already contains its own idempotency guard:

if (testContext.Metadata.TestDetails.TestClassInjectedPropertyArguments.ContainsKey(cacheKey))
    return; // no double-resolution

So removing the outer early-return only allows the ref-counting path to execute — the inner guard still prevents actual duplicate resolution work. The new comment explains the invariant clearly.

Tests

Unit test (PropertyInjectorTests.cs): Good regression coverage — directly verifies that TestClassInjectedPropertyArguments is populated even when IsDiscoveryInstanceReused = true.

One observation: CreateContext<T>() manually wires up a large object graph (ClassMetadata, AssemblyMetadata, TestBuilderContext, etc.). This is somewhat unavoidable given the domain complexity, but consider whether a shared test helper/builder could reduce the boilerplate if more unit tests in this file are added in the future. Not a blocker.

Functional repro (FixtureLifetimeTests.cs): Well-structured. The combination of [NotInParallel] on SomeTest with PerTestSession-shared fixture, mixed test methods (parameterized + plain), directly reproduces the race in #5982. The [EngineTest(ExpectedResult.Pass)] annotation ensures it runs in the engine regression suite.

One minor note: AnotherTest runs in parallel with SomeTest (no [NotInParallel] constraint), which strengthens the repro — it exercises the shared fixture across concurrent class instances.

No Issues Found

The approach is correct, minimal, and well-covered by tests. The fix targets the right layer (registration/caching, not injection), and the inner idempotency guard makes the change safe without any regressions to the non-reused case.