Fix instance MethodDataSource isolation

[thomhurst]

Summary

• Stop reusing the instance MethodDataSource enumeration object as the first generated test instance.
• Dispose the enumeration-only instance after data source expansion.
• Add regression coverage for [Bug]: Instance MethodDataSource reuses enumeration instance for first yielded test case, breaking test isolation consistency #6361 across source-generated engine execution.

Validation

• dotnet build TUnit.Engine\TUnit.Engine.csproj -c Release
• dotnet build TUnit.TestProject\TUnit.TestProject.csproj -c Release (existing warning flood only)
• dotnet build TUnit.Engine.Tests\TUnit.Engine.Tests.csproj -c Release
• .\TUnit.TestProject\bin\Release\net10.0\TUnit.TestProject.exe --treenode-filter "/*/TUnit.TestProject.Bugs._6361/Issue6361InstanceMethodDataSourceIsolationTests/*" --disable-logo --no-progress
• dotnet test TUnit.Engine.Tests\TUnit.Engine.Tests.csproj -c Release --no-build --treenode-filter "/*/*/Issue6361Tests/*" (AOT case skipped locally)
• dotnet test TUnit.UnitTests\TUnit.UnitTests.csproj -c Release --no-build --treenode-filter "/*/*/PropertyInjectorTests/ResolveAndCacheProperties_CachesInjectedProperties"

Fixes #6361

[codacy-production]

Not up to standards ⛔

🔴 Issues 1 high · 1 minor

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

Results:
2 new issues

Category	Results
ErrorProne	1 high
CodeStyle	1 minor

View in Codacy

🟢 Metrics 5 complexity

Metric	Results
Complexity	5

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 instance MethodDataSource isolation (#6379)

Overview

Fixes #6361 — a test class instance created solely to enumerate an instance-bound [MethodDataSource] was previously reused as the first generated test's instance (an optimization to skip duplicate init). That reuse leaked enumeration-time state into the "first" test and made it behave differently from the other generated tests. This PR:

• Stops reusing the enumeration instance as a real test instance — every generated test now always gets a fresh instance via CreateInstance(...).
• Wraps method-data-source instance creation in a new CreateInitializedInstanceForMethodDataSources helper and disposes the enumeration-only instance in a finally block (DisposeInstanceForMethodDataSourcesAsync) in both BuildTestsFromMetadataAsync and the streaming BuildTestsStreamingAsync path.
• Removes the now-dead TestContext.IsDiscoveryInstanceReused flag and the isReusingDiscoveryInstance parameter threaded through ITestBuilder/TestBuilder.BuildTestAsync.
• Adds regression coverage at three levels: TUnit.TestProject/Bugs/6361 (real isolation repro, asserting the enumeration instance ID never shows up as a test instance ID and all 3 generated tests get distinct instances), TUnit.Engine.Tests/Issue6361Tests.cs (source-gen engine execution), and an updated PropertyInjectorTests.

Correctness

• Verified the try/finally scoping in both BuildTestsFromMetadataAsync (line ~233) and BuildTestsStreamingAsync (line ~618): the continue statements inside the try (e.g. on !instanceResult.Success) still run the finally/dispose correctly since they only continue the enclosing classDataFactory loop, not skip the block.
• CreateInitializedInstanceForMethodDataSources disposes the instance itself if property-registration/discovery-init throws after a successful creation, avoiding a leak on that failure path — a real improvement over the pre-PR code, which had no cleanup there at all.
• DisposeInstanceForMethodDataSourcesAsync correctly skips PlaceholderInstance/SkippedTestInstance sentinels and swallows disposal exceptions, matching normal test-instance disposal semantics (comment explicitly calls this out).
• Confirmed this doesn't interact badly with shared/ref-counted properties: the enumeration instance's properties go through RegisterObjectAsync → PropertyInjector.InjectPropertiesAsync, which is a separate path from _objectTracker.TrackObjects (only invoked via RegisterTestAsync for real test contexts). So disposing the enumeration instance doesn't touch the ref-counting used for [ClassDataSource<T>(Shared = ...)]-style shared objects — this behavior is unchanged from before the PR, just now the outer instance itself is properly disposed instead of silently dropped.
• Grepped the repo for lingering references to IsDiscoveryInstanceReused/isReusingDiscoveryInstance — none remain, so the flag removal is complete and doesn't strand any dual-mode (source-gen vs reflection) code path, since this is purely TUnit.Engine-side logic abstracted behind TestMetadata.InstanceFactory.

Verification performed

• dotnet build TUnit.Engine/TUnit.Engine.csproj -c Release — succeeds, 0 warnings/errors.
• dotnet build TUnit.TestProject/TUnit.TestProject.csproj -c Release — succeeds (578 pre-existing, unrelated TUnitAssertions00xx warnings only, matches what the PR description called out).
• dotnet build TUnit.UnitTests/TUnit.UnitTests.csproj -c Release — succeeds.
• Ran the new regression test directly: TUnit.TestProject.exe --treenode-filter "/*/TUnit.TestProject.Bugs._6361/Issue6361InstanceMethodDataSourceIsolationTests/*" → 4/4 passed, confirming the enumeration instance ID is never reused as a test instance ID and all 3 data-driven tests get distinct instances.

Design/architecture note (worth being explicit about, not a blocker)

This trade-off intentionally gives up the "reuse enumeration instance as first test" perf optimization in exchange for correctness — every test generated from an instance-bound method data source now pays for one extra full instance construction + property injection (the enumeration-only instance), on top of its own. That's the right call: the previous optimization was observably broken (that's #6361), and the feature (instance methods as data sources) is a comparatively uncommon, already-not-cheap path (reflection/property injection was already happening for the enumeration instance regardless). No changes needed here — just flagging it so the perf trade-off is understood as deliberate, matching what the PR description already states.

Minor nit (non-blocking)

TUnit.UnitTests/PropertyInjectorTests.cs still names its fixture/test class ReusedDiscoveryInstanceTestClass / ReusedDiscoveryInstanceFixture, even though the "reused discovery instance" concept this PR removes no longer exists. Since the test method itself was already renamed to ResolveAndCacheProperties_CachesInjectedProperties, renaming these leftover type names (e.g. to something like PropertyInjectionTestClass/PropertyInjectionFixture) would avoid future confusion about what's being tested. Not required for this PR.

Test coverage

Good — the new tests cover the bug at the right levels (unit test for property caching, TestProject-level black-box isolation repro asserting instance identity across 3 generated tests, and an engine-level execution test asserting pass/fail counts). No gaps spotted for this fix's scope.

Verdict

✅ Looks good — correct fix for a real isolation bug, disposal path is sound, tests genuinely exercise the regression, and the code builds/passes as described in the PR's own validation steps.