You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Fix dependencies on deferred enumeration tests - #7471
Deferred test expansion removed placeholders from the scheduled test list but left dependency resolution pointing at them. A dependent test could therefore execute the placeholder directly and report an extra failure.
Share the discovery resolver with the session coordinator, replace expanded placeholders as a batch and invalidate affected dependency edges, then resolve against all concrete cases and rebuild context dependency lists before scheduling. Already-completed empty or failed expansions remain dependency outcomes without re-resolving or entering placeholder execution. Regression counters are scoped to the test session. Add a deferred dependency chain regression and restore DependsOn(..., ProceedOnFailure = true) in the original argument-registration failure regression.
Isolated Release benchmark using the old/new replacement methods with lightweight test objects (not an end-to-end suite benchmark): 1,000 placeholders x 3 children took 11.536 ms / 162,344 allocated bytes before and 0.119 ms / 139,656 bytes after. The batch avoids repeatedly scanning the growing test list.
git diff --check passed.
An additional empty-source mode discrepancy reproduced against unchanged main is recorded on #7442: #7442 (comment).
Summary by CodeRabbit
Bug Fixes
Dependencies on deferred tests now wait for all generated test cases to complete, including dependencies across multiple levels of deferred tests.
Deferred tests that produce no cases or fail during expansion retain their completed outcome and are not run as ordinary tests.
Test results now accurately reflect completion and dependency status across deferred test cases.
Newly expanded test cases appear consistently in test discovery and dependency resolution.
It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.
No actionable comments were generated in the recent review.
Walkthrough
Deferred test expansion now updates discovery and dependency tracking before dependency resolution. Empty or failed expansions are marked as completed without children, and the runner skips completed empty placeholders. Tests cover dependency ordering and counts across deferred rows.
The coordinator updates discovery and dependency tracking after expansion, resolves dependencies, and refreshes dependency data. Empty or failed expansions are marked as completed without children. The runner skips completed empty placeholders, and TestFinder rebuilds indexes when the cached-tests version changes.
Tests check dependency ordering and counts across deferred rows, preservation of completed empty placeholder outcomes, and the engine run result. The argument-injection observer declares its dependency with proceed-on-failure enabled.
sequenceDiagram
participant TestSessionCoordinator
participant TestDiscoveryService
participant TestDependencyResolver
participant TestBuilderPipeline
TestSessionCoordinator->>TestDiscoveryService: Replace deferred tests in cached contexts
TestSessionCoordinator->>TestDependencyResolver: Replace deferred tests and resolve dependencies
TestSessionCoordinator->>TestBuilderPipeline: Refresh registered dependency data
Loading
Possibly related issues
thomhurst/TUnit#7474: The issue describes batching deferred-placeholder replacement and refreshing dependency state, which overlap with the replacement and dependency updates in this PR.
Possibly related PRs
thomhurst/TUnit#7391: This PR updates the argument-injection observer test introduced in that PR to declare a dependency and proceed after failure.
Merge Risk: Minimal · up to a5c95
No actionable merge-blocking issue is established. The deferred-dependency changes are mergeable after normal CI checks.
Pre-merge checks: 4 · 1
Failed checks: 1 warning
Check name
Status
Explanation
Resolution
Docstring Coverage
Docstring coverage is 31.25% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…
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 "Fix dependencies on deferred enumeration tests" matches the main change. The PR makes dependency resolution replace deferred-enumeration placeholders with their expanded child tests, and it…
Linked Issues check
[#7409] The PR meets the coding requirements. TestSessionCoordinator shares TestDependencyResolver, replaces expanded placeholders, resolves dependencies against concrete tests, and refreshes cont…
Out of Scope Changes check
The production changes implement [#7409] through shared resolver wiring, placeholder replacement, dependency invalidation, context refresh, and completed-outcome handling. The added tests verify these…
Full details: Title check
Explanation
The title "Fix dependencies on deferred enumeration tests" matches the main change. The PR makes dependency resolution replace deferred-enumeration placeholders with their expanded child tests, and it stops placeholders from running directly. The title is short, specific, and in the imperative mood, so a teammate scanning history would understand the primary change.
Full details: Linked Issues check
Explanation
[#7409] The PR meets the coding requirements. TestSessionCoordinator shares TestDependencyResolver, replaces expanded placeholders, resolves dependencies against concrete tests, and refreshes context dependency lists before scheduling. Completed empty or failed expansions preserve their outcomes and do not execute as placeholders. The deferred dependency regression and restored ProceedOnFailure = true regression cover the reported path. The reported targeted tests support the implementation.
Full details: Out of Scope Changes check
Explanation
The production changes implement [#7409] through shared resolver wiring, placeholder replacement, dependency invalidation, context refresh, and completed-outcome handling. The added tests verify these behaviors. The argument-injection dependency metadata reproduces the linked failure path. No unrelated change is demonstrated.
Full details: Docstring Coverage
Explanation
Docstring coverage is 31.25% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 16 functions across 13 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches
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.
A rabbit checks each row in flight The expanded cases line up right Empty placeholders keep their state While dependency paths update The test run counts each case with care And hops along without a spare
CodeRabbit Verify
Run CodeRabbit Verify to test the changes in this pull request.
Recent review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 4e98e012-0230-4935-a157-066616196ada
Commits
Reviewing files that changed from the base of the PR and between ff56220 and a5c9536.
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 0 remain after this review.
The approach is sound. Sharing one TestDependencyResolver between discovery and the session coordinator is the right fix for the root cause. Re-resolving after all expansions, rather than patching edges one by one, also avoids ordering problems when a dependent placeholder expands before the test it depends on. The regression test covers a three-level chain, and restoring ProceedOnFailure = true in the 7391 test gives a more faithful regression. I only read the diff and the resolver. I did not run anything.
Suggestions, none of them blockers:
ReplaceDeferredTest is O(N) per placeholder, or O(N·P) overall. It clears every index, resets _indexedCount, and scans every test's dependency list with LINQ Any on each call. That is fine for typical suites. With many deferred placeholders in a large suite it becomes quadratic, and it allocates a closure per test. A batch API such as ReplaceDeferredTests(IReadOnlyList<(placeholder, children)>) would do one index rebuild and one pass over the edges. It could also stop at the first match with a plain loop instead of Any. As written, TestSessionCoordinator could collect the expansions and make one call before ResolveAllDependencies().
Zero children leaves the placeholder in the resolver.ReplaceDeferredTest returns early when children.Count == 0, so the placeholder stays in _allTests and in the edges. This is deliberate. The ExpansionCompletedWithoutChildren flag plus the early return in TestRunner.ExecuteTestAsync makes the placeholder complete as passed, so dependents don't fail. It does leave two code paths for what is conceptually the same replacement. The failure path (expansion threw) intentionally keeps the placeholder as a failed dependency outcome, which matches the PR description. A short comment on the early return saying that the placeholder is kept on purpose would stop a later reader from treating it as a bug.
Clearing test.Dependencies = [] drops already-resolved edges to other tests.ResolveAllDependencies then rebuilds them, so the result is correct. But between the clear and the rebuild, a concurrent TryResolveDependencies call sees Length == 0 and re-resolves. That is harmless under _resolutionLock. Removing only the placeholder edges would be tighter, and the current approach relies on resolution being idempotent. The coordinator is the only caller today, so this is acceptable.
ExpansionCompletedWithoutChildren is a mutable internal flag on the test. It is also set for the failure case, where the name is a bit misleading. Something like ExpansionHandled or SkipExecution would describe it better. Alternatively, the coordinator could mark the placeholder's execution as already completed, so that TestRunner needs no type check on a hot path.
[Medium impact] The PR appears safe to merge; the previous parallel lookup issue is fixed and no new actionable issues were found.
Summary
The PR replaces deferred placeholders with concrete cases before scheduling, rebuilds dependency lists, and keeps completed empty or failed expansions as dependency outcomes.
Dependent tests wait for the generated deferred cases.
Test lookups return generated cases after deferred expansion.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Discover placeholders and resolve dependencies] --> B[Expand selected placeholders]
B --> C[Publish concrete cases to discovery cache]
C --> D[Replace resolver entries as a batch]
D --> E[Resolve dependencies and refresh context lists]
E --> F[Schedule concrete cases]
F --> G[Read completed cases through locked lookup snapshot]
Reviewed every review surface for dd9ebe8. Claude and Codex both returned positive verdicts; no blocking findings remain.
Claude's nonblocking suggestions: batching and allocation measurements are tracked in #7474, together with Greptile's scan concern. Retaining zero-child placeholders is intentional so completed outcomes remain dependency targets; expansion and re-resolution finish before scheduling, which makes clearing and rebuilding edges safe here. The internal flag naming/comment suggestions can accompany that follow-up without changing this correctness patch. The restored argument-injection failure regression exercises the failure dependency outcome; the empty-source discrepancy remains tracked on #7442. Engine integration coverage will run in CI after promotion, as documented in the PR.
Promoting this reviewed head to ready to start CI. Auto-merge must wait for the new ready-for-review Claude run to complete and its comment to be checked.
The fix is sound. Dependencies on expanded placeholders are invalidated and then re-resolved against all concrete children before scheduling. The shared resolver instance is wired correctly in TUnitServiceProvider. The regression test and the restored DependsOn(..., ProceedOnFailure = true) are good. I could not run the tests here; this comes from reading the diff.
Suggestions
Quadratic cost in ReplaceDeferredTest. For every placeholder it does the following:
Clears all lookup indices and sets _indexedCount = 0, so the next resolve re-indexes every test.
Scans every test's Dependencies with LINQ Any and a closure.
With P deferred placeholders and N tests, that is roughly O(P·N) per session, plus allocations on a path the repo guidelines say to keep lean. A batched API would avoid this: ReplaceDeferredTests(IReadOnlyList<(placeholder, children)>) does one removal pass, one scan, and one re-index. Alternatively, collect the placeholders into a HashSet and call the resolver once after the loop, which you already do for ResolveAllDependencies. The loop in ExpandDeferredPlaceholdersAsync is the natural place for this. Dependencies being a plain array would also let you replace the Any with a simple loop.
Empty expansion keeps the placeholder in the resolver. When children.Count == 0, ReplaceDeferredTest returns early, so the placeholder stays in _allTests. It is marked Passed and skipped in TestRunner through the ExpansionCompletedWithoutChildren flag. This works. However, the flag is then the only thing stopping a direct run of the placeholder, and the State = Passed plus flag pair is set in two places that must stay in sync. Consider moving that into a small method on DeferredEnumerationExecutableTest (e.g. MarkExpanded(bool failed)) so the invariant lives in one place.
Failed-expansion path.ExpansionCompletedWithoutChildren = true is set on failure, and dependents are not re-resolved. That seems intended: they see a failed dependency and honour ProceedOnFailure. A small engine test for that case (dependent of a placeholder whose expansion throws) would pin the behaviour. The PR only covers the happy path and the fix(core): hold shared instances fetched while runtime tests register #7391 case.
Test coverage.Dependency_Waits_For_All_Deferred_Cases covers the main scenario. The _completed and _dependentCompleted statics are never reset, so the tests would behave oddly if run twice in a process (e.g. retries). That is minor.
Overall this is a good fix with a clear root cause. I would address item 1 before merging.
The earlier wait for run 41610303471 has ended: that run succeeded. Addressed the subsequent review findings in a956b53 while other checks were running.
Claude item 1: batch all placeholder replacements, clear indices once, and scan affected edges once without the LINQ closure. Isolated Release replacement benchmark (lightweight test objects, not end-to-end): 1,000 placeholders x 3 children improved from 11.536 ms / 162,344 allocated bytes to 0.119 ms / 139,656 bytes.
CodeRabbit: refresh all registered TestContext dependency lists after re-resolution; preserve completed empty/failed placeholder outcomes through the final pass; scope regression counters to their session. Each inline thread has its specific fix and evidence.
Claude items 2/3: retained placeholders are explicitly documented; new focused tests cover both passed and failed completed placeholders remaining dependency targets without overwriting outcomes. The flag remains internal; further naming cleanup is optional.
CodeRabbit docstring-coverage warning is nonblocking: the internal batch method and refresh parameter describe their invariants; no repository rule requires the suggested 80% threshold.
Validation: dependency chain 8/8 in each mode; resolver tests 2/2; existing runner tests 9/9; #7391 exactly 2 passes and 3 expected failures in each mode; both new regressions fail with their respective fixes removed; diff check clean. Engine integration tests remain for CI per repository instructions. New-head CI/reviews are pending, so auto-merge is not armed.
Fresh Claude review on this head is positive with four explicitly nonblocking polish suggestions. Disposition: the failed placeholder is intentionally retained as an outcome (covered by the new test); flag centralization and a context-reset helper can remain optional cleanup; reverse-edge indexing is unnecessary for this single-pass fix and would need separate graph benchmarks. No further correctness changes are required by that review.
The approach is sound. The placeholder is replaced by its concrete cases in the shared resolver as one batch. Affected dependency edges are invalidated once and re-resolved against all cases. Context dependency lists are then rebuilt before scheduling. Sharing a single TestDependencyResolver between discovery and the session coordinator through DI is cleaner than a second instance. The regression coverage is good: the engine test, the unit test for the empty/failed outcome, and the restored ProceedOnFailure in the _7391 test.
I did not build or run anything; this is from reading the diff.
Observations, none blocking:
Failed expansions are not in expanded. The catch path does not add the placeholder to expanded, so ReplaceDeferredTests never sees it. It stays in _allTests as a dependency outcome, kept there by the ExpansionCompletedWithoutChildren skips in ResolveAllDependencies and TestRunner.ExecuteTestAsync. That is correct, but the invariant is implicit. A short comment at the catch site saying why the placeholder is intentionally not passed to the resolver would help.
The flag is set in three places. It is set in TestSessionCoordinator for the empty and failed paths and checked in the resolver and the runner. A small method on DeferredEnumerationExecutableTest, for example MarkExpansionCompleted(state, ex), would keep the state, result and flag consistent in one place. It would also stop a future caller from setting one and forgetting the other.
PopulateAllDependencies(refresh: true) reaches into TestContext internals. It touches _dependencies and _dependenciesPopulated directly. A ResetDependencies() method on the context would keep that encapsulated.
Invalidation cost. The invalidation loop walks every test's dependencies, which is fine at one pass. If suites with huge dependency graphs show up, you could track reverse edges instead. The benchmark in the description shows this is already far better than before.
The change is correct in the cases I traced. Approving in spirit; the items above are polish.
The earlier wait for run 41610303471 is complete (success), and current-head Claude/Codex reviews are positive with all review threads dispositioned. The remaining failure is Ubuntu net8.0 Session_Shared_Object_Disposal_Emits_Span_Under_Assembly_Activity: duplicate matching spans in .NET run 41612969657. This matches #7435 and remains unfixed on main.
Worked that blocker under its own lock rather than rerunning the unchanged failure or deferring it: #7492 fixes capture isolation and closes #7435. Its new regression fails with the old helper and all five targeted disposal-span tests pass with the fix on net10.0. Once #7492 merges, refresh this PR onto main for fresh Ubuntu net8.0 CI. No on-hold label is needed.
The earlier blocker has cleared: #7492 is merged as 8112ade, and run 41626176380 completed successfully. Rebased this PR onto current origin/main to include the disposal-span isolation fix and replace the stale Ubuntu failure with fresh CI.
Pushed 54d38f7. The rebase was conflict-free; range-diff confirms both patches are unchanged. Validation: DeferredDependencyTests net10.0 passed 8/8 in source-generated mode and 8/8 with --reflection; git diff --check passed. Engine integration tests remain for CI per repository instructions.
All prior review surfaces were checked and findings have dispositions. Fresh-head CI and reviews are now required; auto-merge remains disabled until those gates pass.
Overall this fixes the root cause correctly. Dependency edges that pointed at expanded placeholders are now invalidated in one batch, then re-resolved against the concrete children. Context dependency lists are rebuilt before scheduling. I only read the diff and did not build or run the tests.
Good
Replacing the placeholders in one pass is better than rescanning _allTests for each placeholder, and the benchmark in the description backs that up.
Sharing one TestDependencyResolver between discovery and the session coordinator through DI avoids a second source of truth.
Empty or failed expansions keep their outcome as a dependency result. They are skipped in TestRunner.ExecuteTestAsync and in ResolveAllDependencies. There is a focused unit test for both cases.
The regression covers a chain of deferred tests, and the counters are scoped to the test session.
Suggestions (non-blocking)
Use a state enum instead of a bool.ExpansionCompletedWithoutChildren is set in three places, and the State = Passed line is only set in the empty case. A bool also hides why a placeholder is terminal. A small ExpansionOutcome (Pending/Expanded/Empty/Failed) on the placeholder, or a method such as MarkExpansionCompleted(...) that sets state and flag together, would stop the two drifting apart.
Make the invalidation rule explicit.ReplaceDeferredTests clears test.Dependencies = [] wholesale for any test that depends on a replaced placeholder. That is correct because ResolveAllDependencies re-resolves tests with empty dependencies and non-empty metadata. It does rely on that implicit rule, though. A one-line comment would help, or an explicit InvalidateDependencies(test).
Check re-entrancy.RegisteredTests returns the live _allTests list without taking the lock, and PopulateAllDependencies enumerates it. This is safe while expansion happens before scheduling, as it does today. If dynamic tests or nested expansion ever run during execution, it would need a snapshot.
Narrow the refresh in PopulateAllDependencies.refresh: true touches Context._dependencies and _dependenciesPopulated for every test. Restricting it to tests whose edges were invalidated would cut the cost on large suites.
None of these block merging. The [NotInParallel] to DependsOn(..., ProceedOnFailure = true) change in the _7391 test also looks like the right way to express that ordering. LGTM, pending CI on TUnit.Engine.Tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Deferred test expansion removed placeholders from the scheduled test list but left dependency resolution pointing at them. A dependent test could therefore execute the placeholder directly and report an extra failure.
Share the discovery resolver with the session coordinator, replace expanded placeholders as a batch and invalidate affected dependency edges, then resolve against all concrete cases and rebuild context dependency lists before scheduling. Already-completed empty or failed expansions remain dependency outcomes without re-resolving or entering placeholder execution. Regression counters are scoped to the test session. Add a deferred dependency chain regression and restore
DependsOn(..., ProceedOnFailure = true)in the original argument-registration failure regression.Closes #7409
Validation:
DeferredDependencyTestson net10.0: 8 passed in source-generated mode and 8 passed with--reflection.Bugs._7391on net10.0 in both modes: exactly 5 results, 2 passed and 3 expected failures; no extra placeholder failure.TUnit.Engine.Testslocally, per repository instructions.TestDependencyResolverTests: 2 passed. ExistingTestRunnerTests: 9 passed.git diff --checkpassed.An additional empty-source mode discrepancy reproduced against unchanged main is recorded on #7442: #7442 (comment).
Summary by CodeRabbit