perf(engine): skip tracked object graph rediscovery

[thomhurst]

Summary

• skip nested object graph rediscovery when initializing objects already present in TestContext.TrackedObjects
• keep nested discovery for the test class instance and standalone InitializeObjectForExecutionAsync paths
• preserve existing OpenTelemetry span behavior through a shared object-only initialization helper

Closes #5718.

Verification

• dotnet test TUnit.TestProject\TUnit.TestProject.csproj -c Release -f net10.0 -- --treenode-filter "/*/*/InheritedDataSourceTests/*"
• dotnet test TUnit.TestProject\TUnit.TestProject.csproj -c Release -f net10.0 -- --treenode-filter "/*/*/InheritedDataSourceTests/*" --reflection
• dotnet test TUnit.TestProject\TUnit.TestProject.csproj -c Release -f net10.0 -- --treenode-filter "/*/*/CombinedDataSourceTests/CombinedDataSource_WithNestedPropertyInjectionAndMultipleIAsyncInitializers*"
• dotnet test TUnit.TestProject\TUnit.TestProject.csproj -c Release -f net10.0 -- --treenode-filter "/*/*/CombinedDataSourceTests/CombinedDataSource_WithNestedPropertyInjectionAndMultipleIAsyncInitializers*" --reflection
• dotnet test TUnit.TestProject\TUnit.TestProject.csproj -c Release -f net10.0 -- --treenode-filter "/*/*/ParallelPropertyInjectionTests/*"
• dotnet test TUnit.TestProject\TUnit.TestProject.csproj -c Release -f net10.0 -- --treenode-filter "/*/*/ParallelPropertyInjectionTests/*" --reflection
• dotnet test TUnit.TestProject\TUnit.TestProject.csproj -c Release -f net8.0 -p:TargetFrameworks=net8.0 -- --treenode-filter "/*/*/InheritedDataSourceTests/*"
• git diff --check

Existing unrelated analyzer warnings were emitted during the retargeted net8.0 build.

[claude]

Code Review

Summary

This PR addresses a genuine performance issue (#5718) where every test was walking its object graph twice during execution:

1. Once at registration time via TrackableObjectGraphProvider.GetTrackableObjects → stored depth-first in TestContext.TrackedObjects.
2. Again at execution time inside InitializeTrackedObjectsAsync → each tracked object would re-invoke InitializeNestedObjectsForExecutionAsync, re-traversing the same graph.

The fix is correct in its core insight: objects in TestContext.TrackedObjects are already ordered deepest-first at registration time, so calling InitializeNestedObjectsForExecutionAsync on each of them during the execution loop is redundant and wasteful. The new InitializeTrackedObjectWithSpanAsync → InitializeObjectOnlyWithSpanAsync path correctly skips the nested traversal while keeping the OTel span behaviour intact for each object.

---

Findings

1. InitializeTrackedObjectWithSpanAsync is a pure pass-through — consider inlining it (minor style)

// ObjectLifecycleService.cs ~274
private Task InitializeTrackedObjectWithSpanAsync(object obj, TestContext testContext, CancellationToken cancellationToken)
{
    return InitializeObjectOnlyWithSpanAsync(obj, testContext, cancellationToken);
}

InitializeTrackedObjectWithSpanAsync does nothing except delegate. Since the call site (InitializeTrackedObjectsAsync, line 253) already reads clearly, this intermediary adds method count without adding clarity. The XML <summary> is what carries the intent — that can be moved to InitializeObjectOnlyWithSpanAsync or the call site as a comment. Not a bug, just noise.

2. Correctness: what about objects that enter TrackedObjects after registration (e.g. late-registered shared objects)?

TrackedObjects is populated by ObjectTracker.TrackObjects, which is called from RegisterTestAsync. If any object is added to TrackedObjects after that point but before InitializeTrackedObjectsAsync runs, the optimisation holds as long as those objects were also registered/traversed by the same mechanism. Based on the code, no late writes appear to happen, but this invariant should be documented on TrackedObjects or TrackObjects itself. A future contributor might break it silently.

3. InitializeObjectOnlyWithSpanAsync has a subtle naming gap

The XML doc says "Callers that use this directly must initialize nested dependencies first." However, there is no enforcement of this precondition and no assertion or guard. The only callers today honour it correctly, but a future caller of the public-ish (it is private) method might not. A brief comment at the single external call site in InitializeObjectWithSpanAsync would suffice:

// Nested dependencies already initialized by InitializeNestedObjectsForExecutionAsync above
await InitializeObjectOnlyWithSpanAsync(obj, testContext, cancellationToken);

4. Performance: List<Task> allocation in the tracked-objects loop (pre-existing, unchanged)

var tasks = new List<Task>(objectsAtLevel.Count);

This is unchanged from before the PR, so out of scope, but worth noting: if objectsAtLevel.Count == 1 (single-object levels, which are common) the allocation and Task.WhenAll wrapper could be skipped entirely with a fast path. No action needed for this PR.

5. #if NET guard in InitializeObjectOnlyWithSpanAsync (unchanged, pre-existing)

The _spannedObjects ConditionalWeakTable and OTel path are gated on #if NET. The extracted method correctly carries this guard through, so the refactor does not regress non-NET builds.

---

Assessment

Approve. The change is correct, well-tested against the target scenarios (source-gen and reflection modes, net8/net10), and the PR description clearly explains the invariant being relied upon. The findings above are minor style/documentation observations — none are blocking.

The one thing I would encourage before merge is a brief comment on TestContext.TrackedObjects (or on TrackObjects) documenting that it must be fully populated before InitializeTrackedObjectsAsync is called, since the optimisation in this PR relies on that ordering invariant being stable.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

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]

Follow-up Review (second commit: chore(engine): inline tracked object initializer)

Previous Findings Addressed

Finding #1 (pass-through method) — resolved. The InitializeTrackedObjectWithSpanAsync wrapper was removed and the call site now directly calls InitializeObjectOnlyWithSpanAsync. The explanatory comment was correctly moved to the call site at line 253–254 where it carries the most meaning.

---

Remaining Observations

Finding #2 (ordering invariant documentation) — still open. The invariant that TrackedObjects must be fully populated before InitializeTrackedObjectsAsync runs is still undocumented at the TrackedObjects declaration or TrackObjects call site. This is the only thing I'd encourage before merge — a single-line comment on TrackedObjects (or the TrackObjects call in RegisterTestAsync) noting that the collection must be complete before execution begins, so a future contributor understands why adding objects later would silently break this optimisation.

Finding #3 (precondition comment) — adequately mitigated. The XML doc on InitializeObjectOnlyWithSpanAsync says callers must initialize nested dependencies first. With the pass-through gone there are now only two callers: the tracked-objects loop (which relies on pre-registration ordering, documented by the new comment) and InitializeObjectWithSpanAsync (which calls InitializeNestedObjectsForExecutionAsync immediately before it). The risk is low and the current docs are sufficient.

---

Assessment

The second commit is a clean improvement that addressed the valid feedback from the first review without introducing new issues. The code is correct, readable, and the inline comment at the call site is more useful than the removed wrapper's XML doc.

Approve. The single open item (Finding #2 — ordering invariant) is non-blocking; it is a documentation suggestion, not a correctness concern.

[claude]

Second commit addresses Finding #1 from prior review. Approve.

[claude]

Review (third pass — current HEAD 8e581a97)

Previous Findings Status

#	Finding	Status
1	Pass-through wrapper should be inlined	✅ Resolved in second commit
2	Ordering invariant undocumented	✅ Resolved — see below
3	Unenforced precondition on span helper	✅ Adequately mitigated

Finding #2 Re-assessment

My previous review marked the ordering invariant as still open, but reviewing the current HEAD more carefully, it is fully addressed:

1. Call-site comment (ObjectLifecycleService.cs, line ~252–253) explicitly states the invariant:

   // Tracked objects were discovered before execution and are already
   // ordered deepest-first, so nested graph traversal would be redundant.

2. Implementation is verifiably correct — ObjectGraphDiscoverer organises objects into a SortedList<int, HashSet<object>> keyed by depth during discovery. InitializeTrackedObjectsAsync iterates values in reverse (for (var i = values.Count - 1; i >= 0; i--)), giving deepest-first order at zero extra cost.

3. Symmetric documentation already exists — ObjectTracker.cs has a matching comment on the disposal path noting the ascending/descending ordering contract, so the invariant is established in at least two places.

Overall Assessment

The optimisation is correct and well-scoped. Skipping InitializeNestedObjectsForExecutionAsync for objects already in TrackedObjects is safe precisely because the discovery phase pre-orders them; the test class instance (which is outside TrackedObjects) still gets full graph traversal. The bool includeNestedObjects = true default preserves all other callers without change.

No further changes needed. ✅ Approve.

[claude]

Code Review — Fourth Pass (HEAD bd1767f6)

Previous Findings Status

#	Finding	Status
1	Pass-through wrapper should be inlined	✅ Resolved (commit 2)
2	Ordering invariant undocumented	✅ Resolved (commit 3, call-site comment)
3	Unenforced precondition on span helper	✅ Adequately mitigated

---

New Commit: fix(engine): satisfy non-net lifecycle analysis

The sole change is adding _ = testContext; in the #else (non-NET) branch:

#else
        _ = testContext;
        await ObjectInitializer.InitializeAsync(obj, cancellationToken);
#endif

This is needed because testContext is used only inside the #if NET block (for span tracking / TraceScopeRegistry). Without this discard, a lifecycle/flow analyzer correctly flags the parameter as unread on the non-NET path.

The fix is correct. _ = param; is the idiomatic C# pattern for silencing unused-parameter analyzer warnings in conditional-compilation scenarios. The alternatives (#pragma warning disable CA1801, splitting into a separate non-NET overload) are all more verbose without being meaningfully clearer.

No new concerns introduced by this commit.

---

Overall Assessment

All four commits are coherent and correct. The core optimisation (skipping redundant nested graph traversal for pre-ordered TrackedObjects) is sound, the OTel span behaviour is preserved, the non-NET build compiles cleanly, and the PR has been well-maintained across review iterations.

Approve. Ready to merge.