fix: create fresh non-shared instances per CombinedDataSources combination

[thomhurst]

Summary

Fixes #6177 — CombinedDataSource_WithNestedPropertyInjectionAndMultipleIAsyncInitializers failing intermittently in AOT.

Root cause

CombinedDataSourcesAttribute materialized each parameter''s data source values once and reused the same object reference across every cartesian combination. With [ClassDataSource<T>] (default SharedType.None), one Address instance was shared by both test cases — violating SharedType.None semantics.

The race (mode-agnostic, surfaced by parallel registration introduced in #5524):

1. Both test cases register in parallel (TestFilterService.RegisterTestsAsync uses Parallel.ForEachAsync for ≥8 tests) and concurrently run PropertyInjector.InjectPropertiesAsync on the same shared instance — this path has no per-instance dedup.
2. The IsPropertyAlreadyPopulated → resolve → SetProperty sequence is not atomic, so both create their own nested Location; the last SetProperty wins.
3. The losing test''s TrackObjects graph holds the orphaned Location, so at execution it initializes the orphan while the test body reads the winner — whose InitializeAsync only runs via the other test''s initialization. If the loser''s body runs first → IsGeolocated == false → "Expected to be true", with exactly one (scheduling-dependent) case failing. Matches both CI failures.

Fix

Defer value materialization to per-combination factories: each test case invokes the data source row factory itself, so non-shared sources produce a fresh instance per test case while shared sources (PerClass/Keyed/etc.) still resolve to their cached shared instance.

Testing

• New regression test CombinedDataSource_NonSharedClassDataSource_CreatesDistinctInstancePerTestCase — verified RED before the fix (second case received the same instance), GREEN after.
• All 200 CombinedDataSourceTests, 30 ClassLevelCombinedDataSources_*, and 6 CombinedDataSourceEdgeCaseTests pass in both source-gen and reflection modes (net10.0).
• TUnit.Core builds clean across all TFMs; no public API changes.

Note

A residual (narrower) race remains for genuinely shared instances (PerClass/Keyed) with injected properties, since RegisterObjectAsync bypasses EnsureInitializedAsync''s per-object dedup — can be addressed separately if desired.

[claude]

Code Review

Overall: This is a well-reasoned fix that correctly addresses the root cause of the shared-instance race. The PR description is thorough and the approach is sound. A few observations below.

---

The Core Fix — Correct

The deferred-materialization pattern is the right solution. By wrapping each value in a Func<Task<object?>> factory instead of materializing eagerly, the cartesian product correctly invokes each data source once per combination. For SharedType.None this produces a fresh instance per test; for SharedType.PerClass/Keyed the factory resolves to the cached shared instance. This honours each SharedType's semantics correctly.

---

Subtle Behaviour Change — Null/Empty Row Handling

Old code (ProcessDataSourceAsync):

var dataRow = await dataRowFunc();
if (dataRow != null && dataRow.Length > 0)
{
    values.Add(dataRow[0]);  // null/empty rows are SKIPPED
}

New code:

valueFactories.Add(async () =>
{
    var dataRow = await dataRowFunc();
    return dataRow is { Length: > 0 } ? dataRow[0] : null;  // factory still added; null returned
});

Previously a null/empty data row would silently filter the entry from the factory set entirely — it would not expand the cartesian product. Now it contributes a factory that returns null, which IS included in the product and grows the combination count. For any data source that legitimately yields an empty row (even as a sentinel), this changes test count and test data unexpectedly.

In practice this is unlikely to matter — ClassDataSource never yields empty rows — but since the filtering was intentional in the original design it's worth being explicit. Consider either preserving the skip behaviour:

// Only add factory if the row is non-empty
Func<Task<object?>>? factory = null;
var dataRow = await dataRowFunc();
if (dataRow is { Length: > 0 })
{
    var captured = dataRow[0];
    factory = () => Task.FromResult(captured);
}
if (factory != null) valueFactories.Add(factory);

…or documenting that null-producing rows are intentionally now first-class values.

---

GetCartesianProduct<T> Generics — Good Clean-Up

Making this generic is a nice improvement. It avoids boxing/unboxing during the product construction phase and makes the method reusable. No concerns here.

---

Regression Test — Well Designed

Using ConcurrentDictionary.TryAdd to assert distinct-by-reference identity is concise and accurate. Running this in parallel (as the engine does) turns the bug into a deterministic test failure rather than a race — good choice. The comment linking to #6177 is also helpful.

---

The Residual Race (Acknowledged)

The PR note about shared instances (PerClass/Keyed) with injected properties still racing via RegisterObjectAsync is accurate and reasonable to track separately. Out of scope here.

---

Minor

No public API changes, all TFMs clean, both source-gen and reflection modes covered — nothing more to add there.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics -9 complexity

Metric	Results
Complexity	-9

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 (Follow-up)

Checking previous review items and assessing final state.

---

Previous Concern: Null/Empty Row Handling — Still Unaddressed

My earlier review flagged a subtle behaviour change in ProcessDataSourceAsync. The current code is still:

valueFactories.Add(async () =>
{
    var dataRow = await dataRowFunc();
    return dataRow is { Length: > 0 } ? dataRow[0] : null;
});

The factory is always added to valueFactories, even when the underlying row is null/empty. In the original code, a null/empty row would silently skip that entry — it would never appear in the cartesian product at all. Now it contributes a factory that produces null, which does enter the product and expands the test case count.

This is still worth a deliberate call: either preserve the skip with an eager check, or explicitly document that null rows are now first-class values in CombinedDataSources. Given no built-in data sources produce empty rows in normal usage, this is low-risk in practice — but it's still a semantic change from the original design.

---

Overall Assessment

Everything else is solid:

• Core fix is correct — deferred materialization properly addresses the SharedType.None instance-sharing race during parallel cartesian expansion.
• CartesianProductHelper<T> is a clean, well-motivated extraction — eliminates duplication between MatrixDataSourceAttribute and CombinedDataSourcesAttribute, and the generic avoids boxing.
• Regression test is well-designed — ConcurrentDictionary.TryAdd gives reference-identity assertion without manual locking, and running under the normal parallel engine makes the pre-fix bug deterministic.
• PR description documents root cause, fix, and residual race clearly.

The only open item is the null/empty row question from the prior review. Depending on whether that's intentional, a one-line comment or a guard in ProcessDataSourceAsync would close it out.