Skip to content

feat: add DeferEnumeration to defer data-source expansion to runtime (#5833)#6197

Merged
thomhurst merged 6 commits into
mainfrom
feat/5833-defer-enumeration
Jun 8, 2026
Merged

feat: add DeferEnumeration to defer data-source expansion to runtime (#5833)#6197
thomhurst merged 6 commits into
mainfrom
feat/5833-defer-enumeration

Conversation

@thomhurst

Copy link
Copy Markdown
Owner

What

Adds DeferEnumeration — TUnit's equivalent of xUnit's DisableDiscoveryEnumeration (closes #5833).

Setting DeferEnumeration = true on a data source tells TUnit not to enumerate it during discovery. The test appears as a single placeholder node, and the data rows are enumerated into individual cases only when the test is run (each reported nested under the placeholder). This avoids the IDE/test-explorer overhead of expanding a data source that produces thousands of cases.

public static IEnumerable<int> ManyCases() => Enumerable.Range(0, 10_000);

[Test]
[MethodDataSource(nameof(ManyCases), DeferEnumeration = true)]
public async Task MyTest(int input)
{
    await Assert.That(input).IsGreaterThanOrEqualTo(0);
}

With the flag set, discovery shows one node for MyTest instead of 10,000; running it expands and reports each case.

How

Deferral only suppresses discovery enumeration — at run time, enumerating is fine. So:

  • Flag: bool DeferEnumeration added to IDataSourceAttribute (honored by all data sources). [Arguments] and the internal empty source hardcode it to false (a single row — nothing to defer).
  • Discovery: TestBuilder emits one DeferredEnumerationExecutableTest placeholder when any data source on the test defers (gated by a new TestBuildingContext.IgnoreDeferral). Built in both the discovery and execution builds so it matches the IDE's UID filter.
  • Execution: TestSessionCoordinator.ExecuteTests calls a new DeferredTestExpander before counters/scheduling. It re-runs the builder with IgnoreDeferral to produce the real cases, registers them, reports the placeholder, and adds the children to the scheduled list — so the children flow through the normal pipeline with correct hooks and lifecycle counting. The placeholder itself never runs as a test (its Create/Invoke throw as a safety guard).
  • Nesting: TUnitMessageBus now surfaces TestContext.ParentTestId as the MTP parentTestNodeUid, so the rows nest under the placeholder. (This also improves nesting for existing runtime test variants.)
  • Source generator: threads the DeferEnumeration named argument into the emitted MethodDataSource factory.

Works in both source-gen and reflection modes; no new reflection (AOT-safe).

Tradeoffs / limitations (documented)

  • Individual rows can't be selected/filtered from the IDE or targeted by another test's [DependsOn] — they don't exist until runtime.
  • The placeholder is reported once as a passed container (counts as +1 per deferred test). If the data source throws while enumerating, the error surfaces as a failed result at run time instead of failing discovery for the whole assembly.

⚠️ Breaking-ish: IDataSourceAttribute gains a non-default DeferEnumeration member, so anyone implementing the interface directly must add it. This mirrors the existing SkipIfEmpty member (a default interface member isn't safe given net472 support).

Verification

Verified locally in both source-gen and reflection modes:

  • Discovery collapses deferred tests to 1 node each (including a [Repeat(2)] variant).
  • Run all → full expansion; run a single deferred test via filter → expands all its children (filter-bypass works).
  • Throwing data source → surfaces as a failed result, no discovery crash.

Tests/snapshots added/updated:

  • Samples in TUnit.TestProject/DeferEnumerationTests/
  • Engine tests TUnit.Engine.Tests/DeferEnumerationTests.cs (dual-mode; AOT runs in CI)
  • Source-gen snapshot DeferEnumerationTests.Test.verified.txt
  • PublicAPI snapshots regenerated (all 4 TFMs)
  • Docs: docs/docs/writing-tests/defer-enumeration.md + overview table + sidebar

Existing TUnit.UnitTests (219), source-gen snapshot tests (118), and variant engine tests pass unchanged.

…on to runtime (#5833)

TUnit's equivalent of xUnit's DisableDiscoveryEnumeration. A data source marked
`DeferEnumeration = true` is not enumerated during discovery: the test shows as a
single placeholder node, and its rows are expanded into real cases at runtime
(reported nested under the placeholder). Cuts IDE/test-explorer overhead for data
sources that produce thousands of cases.

- New `bool DeferEnumeration` on `IDataSourceAttribute` (honored by all sources;
  hardcoded false for single-row `[Arguments]`/empty sources).
- Source generator threads the named arg into the emitted MethodDataSource factory.
- `TestBuilder` emits one `DeferredEnumerationExecutableTest` placeholder when any
  data source defers; `DeferredTestExpander` re-runs the builder (IgnoreDeferral) at
  the start of execution, registers the children and schedules them through the
  normal pipeline so hooks/lifecycle counting stay correct.
- `TUnitMessageBus` surfaces `ParentTestId` as the MTP `parentTestNodeUid` so the
  rows nest under the placeholder (also improves nesting for runtime test variants).

Works in both source-gen and reflection modes. Adds samples, engine tests, a
source-gen snapshot, docs, and updated PublicAPI snapshots.
@codacy-production

codacy-production Bot commented Jun 8, 2026

Copy link
Copy Markdown

Not up to standards ⛔

🔴 Issues 6 high · 5 medium · 8 minor

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

Results:
19 new issues

Category Results
BestPractice 5 medium
ErrorProne 6 high
CodeStyle 8 minor

View in Codacy

🟢 Metrics 22 complexity

Metric Results
Complexity 22

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 claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: feat/defer-enumeration (#6197)

This is a well-designed feature that cleanly solves a real problem. The architecture — placeholder node in discovery, expansion at runtime before counting/scheduling — correctly threads children through the existing hook/lifecycle pipeline. A few design issues are worth addressing before merge.


Issue 1: Silent no-op setter on [Arguments] / EmptyDataSourceAttribute (design concern)

Files: TUnit.Core/Attributes/TestData/ArgumentsAttribute.cs, TUnit.Engine/EmptyDataSourceAttribute.cs

The current implementation:

// [Arguments] yields a single row, so deferring its enumeration would be pure overhead - always false.
public bool DeferEnumeration { get => false; set { } }

A user who writes [Arguments(1, 2, 3, DeferEnumeration = true)] gets silent no-op behavior — the setter is called, nothing happens, and discovery still enumerates. No compile error, no runtime warning.

Better approaches (pick one):

  1. Make the interface property read-only: bool DeferEnumeration { get; } — it is only ever set at compile time via attribute initializer syntax and never mutated. This removes the need for no-op setters entirely. SkipIfEmpty is legitimately mutable; DeferEnumeration on a single-row attribute can never meaningfully be true.

  2. If { get; set; } is required for API consistency, throw in the setter:

    public bool DeferEnumeration {
        get => false;
        set { if (value) throw new NotSupportedException("[Arguments] is always a single row; DeferEnumeration has no effect."); }
    }
  3. Add a TUnit analyzer rule warning when DeferEnumeration = true is set on a single-row attribute.

Option 1 is the cleanest — avoids the problem for all current and future single-row implementors.


Issue 2: Placeholder inflates test counts (+1 per deferred test)

File: TUnit.Engine/TestSessionCoordinator.cs

From the engine tests:

// 10-row source: 10 cases + 1 placeholder = 11
result => result.ResultSummary.Counters.Total.ShouldBe(11),
result => result.ResultSummary.Counters.Passed.ShouldBe(11)

A user with a 10,000-row deferred source sees 10,001 passed tests in CI. This confuses dashboards, count-based quality gates, and test-count comparisons over time. Worse, the placeholder reports Passed even when every child fails — its state reflects successful expansion, not successful execution.

Preferred approach: Report the placeholder as a container/suite node that does not count toward test totals. If MTP does not support this, the docs should prominently call out the count inflation and explain what the +1 represents. The current docs page (defer-enumeration.md) omits this side effect entirely.


Issue 3: _Deferred suffix collision risk in GenerateDeferredPlaceholderTestId

File: TUnit.Engine/Services/TestIdentifierService.cs

vsb.Append("_Deferred");

A method named MyTest_Deferred(int x) without DeferEnumeration generates the same ID as MyTest(int x) with DeferEnumeration = true. Low probability, deterministic, and silent — one registration silently overwrites the other. At minimum, document this as a known limitation; ideally use a suffix that cannot appear in a valid C# method name (e.g., a non-identifier character).


Observation: FsCheckPropertyAttribute.DeferEnumeration silently accepted

File: TUnit.FsCheck/FsCheckPropertyAttribute.cs

/// Not used - FsCheck generates its own data during test execution.
public bool DeferEnumeration { get; set; }

Unlike [Arguments], this has a live setter, so setting DeferEnumeration = true here could actually propagate into HasDeferredDataSource and produce a placeholder for FsCheck tests — which is a logical contradiction (FsCheck has no discovery-time rows to defer). Same fix as Issue 1 is warranted here.


Positive notes

  • Dual-mode (source-gen + reflection) implementation is correct; snapshot test proves the generated attribute initializer.
  • HasDeferredDataSource uses early-return loops — no unnecessary allocations.
  • Expanding placeholders before InitializeEventReceivers in ExecuteTests is the correct ordering so children get proper hook/lifecycle counting.
  • DeferredTestExpander using Filter: null with documented rationale is correct; re-filtering by the placeholder's filter would silently drop rows with different IDs.
  • Error handling in ExpandDeferredPlaceholdersAsync marks the placeholder as Failed when the data source throws — good, the IDE node gets a visible result.
  • AOT annotations ([UnconditionalSuppressMessage]) are in the right places.
  • Public API snapshots updated for all 4 TFMs.
  • Docs page and sidebar entry included.

The breaking-change acknowledgment in the PR description is appropriate. The main asks are Issue 1 (silent setter, should fix before merge) and Issue 2 (count inflation, at minimum document it prominently in the defer-enumeration docs page).

…w sources (PR #6197 review)

Address review feedback on #6197:

- Placeholder is now a true container: on successful expansion it is NOT reported
  as its own (passed) result. Previously every deferred test added +1 to the test
  count and showed green even if its children failed. The children carry the real
  results (nested via ParentTestId); the placeholder reports a result only if the
  expansion itself throws, so that failure stays visible.
- TUnit.FsCheck: `DeferEnumeration` hardcoded to false (get => false; set {}) like
  [Arguments] — FsCheck has no discovery-time rows to defer, so a placeholder for
  an FsCheck test would be a contradiction.
- Document on IDataSourceAttribute that single-row sources ([Arguments]) ignore the
  flag (a read-only property isn't viable — attribute named-argument syntax requires
  a setter).
- Update engine test expectations (10/30 instead of 11/31) and the docs note.
@thomhurst

Copy link
Copy Markdown
Owner Author

Thanks for the review — addressed in 43443ff.

Issue 2 (count inflation + misleading green placeholder) — fixed. The placeholder is now treated as a true container: on successful expansion it is no longer reported as its own result, so it doesn't count toward totals and can't show green while its children fail. The children carry the real results (nested via ParentTestId). A result is reported only if the expansion itself throws, so that failure stays visible. (Per-row data errors still surface as their own failed child via the standard data-generation-error path.) Engine tests now assert exactly the case count (10 / 30), and the docs note explains the container semantics. Verified in both modes: Deferred → 10, all → 40, throwing source → total 1, failed 1 (just the failed child, no green placeholder).

Observation (FsCheck live setter) — fixed. FsCheckPropertyAttribute.DeferEnumeration is now hardcoded false (get => false; set {}) like [Arguments], since FsCheck has no discovery-time rows to defer.

Issue 1 (read-only property) — partially declined, with a fix. A read-only bool DeferEnumeration { get; } isn't viable: attribute named-argument syntax ([MethodDataSource(..., DeferEnumeration = true)]) requires the property to be settable, and the member is shared via the interface — so making it get-only would make the feature uncompilable on the attributes that legitimately use it. Throwing in the setter is also risky: the source generator round-trips named args into generated attribute initializers, so a throw would run at module/attribute construction and could break discovery. So I kept the no-op false on the single-row sources (intentional per the design — there's genuinely nothing to defer for one row) and documented it on the interface instead. An analyzer warning for DeferEnumeration = true on a single-row source is a reasonable future addition if this proves to be a footgun in practice.

Issue 3 (_Deferred suffix collision) — kept, with rationale. This matches the existing GenerateFailedTestId convention (_DataGenerationError suffix), which carries the identical theoretical risk. A real collision needs a contrived clash — a method literally named Foo_Deferred(int x) and a non-deferred Foo(int x) with matching parameters. Diverging only this one suffix to a non-identifier character would make it inconsistent with the sibling method; I'd rather change both together (or neither) in a follow-up if this is a concern.

@claude

claude Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Follow-up review: post-fix state (commit 43443ff)

Thanks for the quick turnaround. The three fixes address all the blocking issues from the previous review. Here's a summary of the current state and a few minor remaining notes.

What's fixed

Issue 2 (count inflation) — fully resolved. ExpandDeferredPlaceholdersAsync now removes the placeholder from the list and adds its children without reporting the placeholder as a passed result. Engine tests now assert exactly 10/30 (not 11/31), and the docs describe the placeholder as a container. The ordering is also correct: InProgress and Failed are only emitted when expansion itself throws, so the placeholder node gets a visible failure result in that case.

FsCheck observation — resolved. DeferEnumeration is now get => false; set {}, matching [Arguments].

Issue 1 (silent setter) — rationale accepted. The author's explanation that attribute named-argument syntax ([MethodDataSource(..., DeferEnumeration = true)]) requires a settable property is correct — a get-only interface member would break compilation on implementors. The added <remarks> on IDataSourceAttribute.DeferEnumeration documents the no-op behavior clearly.


Minor remaining observations (non-blocking)

testList.Remove(placeholder) is O(n) per placeholder.
In ExpandDeferredPlaceholdersAsync, the placeholders list is iterated and each one is removed via testList.Remove(...) (linear scan). With N deferred tests in a list of M tests this is O(N*M). In practice, deferred tests are a user opt-in for large-data scenarios and the number of placeholder nodes (not data rows) is small, so this won't matter at runtime. Just noting it for completeness — a HashSet<DeferredEnumerationExecutableTest> + testList.RemoveAll(x => ...) pattern would be O(N+M) if it ever becomes an issue.

InProgress sent after both timestamps are set in the error path.
In the catch block:

placeholder.StartTime = DateTimeOffset.UtcNow;
placeholder.EndTime = DateTimeOffset.UtcNow;
placeholder.SetResult(TestState.Failed, ex);
await _messageBus.InProgress(placeholder.Context);   // ← emitted after End is already set
await _messageBus.Failed(placeholder.Context, ex, placeholder.StartTime.GetValueOrDefault());

MTP generally expects InProgressFailed with StartTime < EndTime. Here both timestamps are identical and InProgress is sent after the test logically ended. This is an error-only path (data source threw at expansion time), so it won't affect normal runs, and the Failed message that follows will still make the test visible as a failure. It's cosmetically odd but not user-impacting.


Overall

The implementation is correct and the main concerns are addressed. The _Deferred suffix decision (keeping it consistent with _DataGenerationError) is reasonable — changing just one in isolation would be inconsistent, and the collision scenario requires a contrived name clash. The PR is in good shape.

…ror-path message order (PR #6197 review)

Non-blocking follow-up review notes:
- Remove all deferred placeholders from the test list in one RemoveAll pass
  (O(N+M)) instead of testList.Remove per placeholder (O(N*M)).
- In the expansion-error path, emit InProgress before stamping EndTime/result so
  the ordering is InProgress -> Failed with Start <= End.
No behavior change (counts unchanged: 40 passing, error case total 1/failed 1).
@thomhurst

Copy link
Copy Markdown
Owner Author

Thanks — both minor notes addressed in 52fd97f (no behavior change; counts still 40 passing / error case total 1, failed 1):

  • O(N·M) placeholder removal → now a single testList.RemoveAll(static t => t is DeferredEnumerationExecutableTest) pass (O(N+M)) before the expansion loop.
  • Error-path message orderInProgress is now emitted before EndTime/SetResult, so the sequence is InProgressFailed with Start ≤ End.

_Deferred suffix left as-is per the agreed rationale (consistency with _DataGenerationError).

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: feat: add DeferEnumeration to defer data-source expansion to runtime (#5833)

Overall, this is a well-structured feature that mirrors xUnit's DisableDiscoveryEnumeration cleanly. The architecture — placeholder at discovery, expand at runtime via DeferredTestExpander, children flow through the normal pipeline — is sound. The handling of the Repeat interaction and the error-surface path are especially nice. A few issues worth discussing:


1. IsForExecution: false during expansion may have unintended side-effects

In DeferredTestExpander.ExpandAsync:

var buildingContext = new TestBuildingContext(IsForExecution: false, Filter: null, IgnoreDeferral: true);

The comment explains the intent — Filter: null bypasses re-filtering, which is correct. But IsForExecution: false is semantically wrong: these tests are being built for execution. IsForExecution likely gates other execution-specific behaviour in TestBuilder (event invocations, pre-execution hooks, etc.) beyond just filtering. Setting it to false for an expansion that feeds directly into the execution pipeline is a hidden invariant violation.

Suggested approach: separate the filtering concern from the execution flag. Either pass IsForExecution: true, Filter: null (no filter = no re-filtering, which is the intended behaviour), or add a dedicated bool BypassFilter to TestBuildingContext so the intent is explicit and orthogonal to the execution flag. This mirrors how IgnoreDeferral was added — a precise flag for a precise concern.


2. Failed expansion not counted in test totals

In TestSessionCoordinator.ExpandDeferredPlaceholdersAsync, all placeholders are removed from testList with RemoveAll before expansion. When a placeholder's expansion throws:

testList.RemoveAll(static t => t is DeferredEnumerationExecutableTest); // placeholder gone
// ...
catch (Exception ex)
{
    await _messageBus.Failed(placeholder.Context, ex, ...); // visible in output...
    // ...but not added back to testList
}

InitializeTestCounts runs after expansion and operates on testList, so the failed placeholder never enters the counter. The failure is visible in the output but the run summary will show N failed with the placeholder not counted. For a single failing deferred test with 0 children, that means the summary reads "0 total" even though something failed.

Suggested fix: Add the failed placeholder back into testList in the catch block before the method returns, or use a separate tracking list that InitializeTestCounts can include. Re-inserting the placeholder into testList also ensures any downstream lifecycle management (cleanup, assembly teardown) sees it.


3. No-op setter on [Arguments] is exposed in the public API

// ArgumentsAttribute:
public bool DeferEnumeration { get => false; set { } }

The public API snapshots show this as a full { get; set; } property, so users who write [Arguments(1, DeferEnumeration = true)] get silent incorrect behaviour — no error, no warning, no effect. The interface XML doc does mention the no-op, but that's only visible to users who read it carefully.

The correct fix is an analyzer warning (similar to how TUnit already has data-source analyzers). A warning on setting DeferEnumeration = true for a single-row source like [Arguments] or [EmptyDataSource] would be far more discoverable than a comment in the XML doc.


4. Sequential expansion adds startup latency for multi-deferred suites

foreach (var placeholder in placeholders)
{
    var children = await _deferredTestExpander.ExpandAsync(placeholder, cancellationToken);
    testList.AddRange(children);
}

Expansions are sequential. If a suite has 10 deferred tests, each enumerating a large source, startup latency is 10× a single expansion. Since each ExpandAsync is independent, they could be parallelised with Task.WhenAll (collecting results into a concurrent bag and then adding all to testList after). This is a performance concern, not a correctness one, but it somewhat undermines the feature's goal of reducing overhead.


5. Missing coverage for ClassDataSource deferral

HasDeferredDataSource correctly checks both metadata.DataSources and metadata.ClassDataSources, but the engine tests and test project only exercise the method-data-source path. A test (or at least a DeferEnumerationTests.cs sample) covering [ClassDataSource] with DeferEnumeration = true would confirm the class-data path works end-to-end and prevent it from silently regressing.


Minor / positive notes

  • The _Deferred suffix in GenerateDeferredPlaceholderTestId guarantees no collision with real expanded row IDs — this is clean, though promoting it to a named constant would make its uniqueness guarantee more visible.
  • DeferredEnumerationExecutableTest.CreateInstanceAsync / InvokeTestAsync throwing with clear messages is an excellent safety guard.
  • The source generator explicitly threading DeferEnumeration rather than relying on round-tripping named arguments is the right call for this code path.
  • The docs are thorough, the trade-offs are clearly documented, and the warning block about [DependsOn] compatibility is appreciated.

…regate result

The IDE run of a deferred test's placeholder node showed "Inconclusive: Test has
not run" because the placeholder produced no result when its children were the
real results. Reporting it always-Passed (the original behaviour) is wrong too —
it would show green even when a child fails.

Now the placeholder is reported as a container: InProgress when expansion starts,
then a final result after the children run that aggregates them — passed only if
every case passes, failed if any case fails, skipped if all were skipped. This
resolves the IDE node without masking child failures. The placeholder therefore
adds one entry to flat result counts (TRX/console) per deferred test; engine
tests and docs updated to reflect this.
@thomhurst

Copy link
Copy Markdown
Owner Author

Good catch — running the placeholder in the IDE showed "Inconclusive: Test has not run" because, after the previous fix, the placeholder node produced no result (its children were the only results). Fixed in ee04fed.

The placeholder is now reported as a container with an aggregate result:

  • InProgress when expansion starts (so the node the user ran shows as running and the cases nest under it);
  • after the cases execute, a final result that aggregates them — passed only if every case passes, failed if any case fails, skipped if all were skipped.

This resolves the "not run" state without the misleading-green problem (reporting it always-passed would hide child failures). The cost is that the placeholder counts as one extra entry in flat reporters (TRX/console) — a 10-row deferred source reports 11 (10 cases + 1 container) — which is now documented. Engine tests updated (11 / 31; error case = failed case + failed container = 2).

@thomhurst if you re-pull and rebuild, the placeholder node should now resolve to pass/fail in the IDE rather than "not run" — could you confirm it behaves as expected on your end? I can verify console/TRX counts here but can't drive Rider directly.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up review (commit ee04fed)

Good progress across the three fix commits — most of the concerns from the previous reviews have been addressed. Below is a status check plus remaining items.


✅ Resolved

  • No-op setter on [Arguments] / FsCheckPropertyAttribute — hardcoded get => false; set {} now in both, matching EmptyDataSourceAttribute. Consistent.
  • Failed-expansion error path message orderingInProgress emitted before EndTime/result stamp (commit 52fd97f976).
  • Single-pass placeholder removalRemoveAll in one pass instead of per-placeholder Remove.
  • Placeholder aggregate result — the placeholder now reports InProgress → aggregate outcome (pass/fail/skip) after its children run, resolving the "not run" IDE state (commit ee04fedffd).

⚠️ Still open: IsForExecution: false during expansion

DeferredTestExpander.ExpandAsync still builds with IsForExecution: false:

var buildingContext = new TestBuildingContext(IsForExecution: false, Filter: null, IgnoreDeferral: true);

These tests are being built for execution, not for discovery. IsForExecution gates event-receiver invocation and other execution-specific behaviour in TestBuilder. Passing false here means any code that conditionally runs on IsForExecution: true will be silently skipped for expanded children. The Filter: null concern (no re-filtering of children) is separate and already correct — but it should be expressed orthogonally:

// Preferred: execution semantics, no filter applied
var buildingContext = new TestBuildingContext(IsForExecution: true, Filter: null, IgnoreDeferral: true);

Or add a dedicated bool BypassFilter to TestBuildingContext so the two concerns don't share a flag. Either way, verify what TestBuilder gates on IsForExecution before shipping — if nothing execution-critical is gated there, document that; if something is, this is a real bug.


⚠️ Still open: _Deferred suffix collision

GenerateDeferredPlaceholderTestId appends the literal string _Deferred to the method name. A user method named MyTest_Deferred(int x) without DeferEnumeration produces the same ID as MyTest(int x) with DeferEnumeration = true. This is silent (one registration overwrites the other) and deterministic (reproducible crash).

At minimum, add a comment in GenerateDeferredPlaceholderTestId and DeferEnumerationExecutableTest that _Deferred is a reserved method-name suffix. Ideally use a character that cannot appear in a valid C# identifier (e.g. \0 or ·) — or a constant that names its uniqueness guarantee.


Minor: Missing ClassDataSource deferral coverage

HasDeferredDataSource checks both metadata.DataSources and metadata.ClassDataSources, but the engine tests and sample project only exercise the method-data path. A single test with [ClassDataSource(..., DeferEnumeration = true)] would confirm end-to-end correctness of the class-data path and prevent silent regression.


Count inflation (by-design, but document it)

The current design adds +1 to Passed/Total counts per deferred test (the placeholder container). The commit message for ee04fedffd acknowledges this, and the engine tests now expect 11/31 rather than 10/30. The defer-enumeration.md docs page should call this out explicitly in a note or warning block — users comparing test counts across runs (CI dashboards, quality gates) will otherwise see unexplained +N counts after enabling the feature.


Overall

The architecture is solid and the iterative fixes have addressed the highest-risk issues. The IsForExecution: false question is the most important remaining item to verify before merge; the others are polish.

…ution and _Deferred id (PR #6197 review)

Follow-up review items:
- Add a class-level (constructor) data source deferral sample + engine test, since
  HasDeferredDataSource checks metadata.ClassDataSources but nothing exercised it.
  (5 instances x 1 method = 5 cases + 1 placeholder = 6.)
- Comment in DeferredTestExpander: IsForExecution:false matches the runtime-built-test
  precedent (TestRegistry/BuildTestsAsync) and only gates a Filter-dependent pre-filter
  optimisation that is a no-op with Filter:null; execution registration is done
  explicitly via RegisterTestsAsync(isForExecution:true).
- Strengthen GenerateDeferredPlaceholderTestId docs with the structural proof that the
  _Deferred placeholder id cannot collide with any real test id (real ids always carry
  numeric data-index segments around the method name that the placeholder omits).
- Docs: call out the +1 flat-count (placeholder container) in the Trade-offs block.
@thomhurst

Copy link
Copy Markdown
Owner Author

Thanks — addressed in 30500cc.

IsForExecution: false during expansion — verified correct, no change. IsForExecution is referenced in exactly one place in the build path (TestBuilder.cs:120): if (buildingContext.IsForExecution && buildingContext.Filter != null) — the pre-filter optimisation. Since the expander passes Filter: null, that branch is skipped regardless of the flag's value, so false vs true is behaviorally identical here. It's not what gates event receivers — execution registration is done explicitly via RegisterTestsAsync(isForExecution: true). false also matches the existing runtime-built-test precedent (TestRegistry.ProcessPendingDynamicTests, TestBuilderPipeline.BuildTestsAsync). Added a comment documenting this.

Class-data deferral coverage — added. New DeferEnumerationClassDataTests sample ([MethodDataSource(..., DeferEnumeration = true)] on the class) + engine test, exercising the metadata.ClassDataSources branch: discovery shows 1 node, run expands to 6 (5 instances + placeholder).

Count inflation — documented prominently. Now called out in the Trade-offs warning block of the docs, not just the info note.

_Deferred collision — strengthened the proof, kept the suffix. I dug into the exact id shapes and the collision can't actually occur: every real id from GenerateTestId is ...(ctorParams).{classIdx}.{classLoop}.Method(params).{methodIdx}.{methodLoop}.{repeat} — the method name is always wrapped in numeric data-index segments. The placeholder id is ...Method(params)_Deferred with none of those segments, so the shapes can never be equal. The scenario in the review (MyTest_Deferred vs MyTest deferred) produces ...MyTest_Deferred(int).i.j.k vs ...MyTest(int)_Deferred — different. I added this as a <remarks> proof rather than switching to a non-identifier delimiter, since a non-ASCII/control char in test ids risks breaking TRX serialization and treenode filters for a collision that can't happen. Happy to revisit if you'd still prefer the delimiter change.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final review: post-fix state (commit 30500cc)

All items raised in the previous three review rounds have now been addressed. This is a thorough, iterative fix cycle and the PR is in good shape.

All previous issues resolved

Issue 1 (silent no-op setter)[Arguments], EmptyDataSourceAttribute, and FsCheckPropertyAttribute all use get => false; set {}, consistent with each other. The IDataSourceAttribute.DeferEnumeration XML doc documents the no-op behavior clearly. The rationale for keeping a settable property (attribute named-argument syntax requires it; a get-only interface member would break compilation on implementing types) is correct and well-explained.

Issue 2 (count inflation / misleading green placeholder) — Resolved in two stages. The placeholder no longer reports a spurious Passed result; it now reports as a container with an aggregate outcome (pass/fail/skip) that correctly reflects its children. Count inflation is prominently documented in the Trade-offs warning block of defer-enumeration.md with a concrete example (10-row source = 11 results).

Issue 3 (_Deferred suffix collision) — The structural proof is now in the <remarks> on GenerateDeferredPlaceholderTestId: real test IDs always carry numeric data-index segments around the method name that the placeholder omits, so the two shapes can never be equal. This is convincing. Keeping the suffix consistent with _DataGenerationError is the right call.

IsForExecution: false during expansion — Verified correct. TestBuilder.cs:120 is the sole reference: if (buildingContext.IsForExecution && buildingContext.Filter != null) — a pre-filter optimization that is a no-op when Filter: null regardless of the flag. Execution registration is done explicitly via RegisterTestsAsync(isForExecution: true). The added comment in DeferredTestExpander explains this clearly and cites the TestRegistry/BuildTestsAsync precedent.

O(N·M) placeholder removal — Fixed to a single RemoveAll pass.

Error-path message ordering — Fixed: InProgress is now emitted before EndTime/SetResult.

ClassDataSource deferral coverage — Added DeferEnumerationClassDataTests (5 instances + 1 placeholder = 6) with a corresponding engine test, exercising the metadata.ClassDataSources path end-to-end.


One minor observation (non-blocking)

The expansion loop in ExpandDeferredPlaceholdersAsync is still sequential — each placeholder is expanded one at a time. For the typical use case this is fine (placeholder counts are small; it's the data rows that are large, and those enumerate concurrently later). If a future scenario involves many deferred tests with expensive data-source methods, a Task.WhenAll-based parallel expansion would reduce startup latency. This is not a concern for merge — just something to keep in mind if the feature sees heavier use.


Overall

The architecture is sound, the feature works correctly in both source-gen and reflection modes, all edge cases (repeat, class-level data source, throwing data source) are covered by engine tests, and the docs clearly explain the trade-offs. The PR is ready to merge.

intellitect-bot pushed a commit to IntelliTect/EssentialCSharp.Web that referenced this pull request Jun 9, 2026
Updated [TUnit](https://github.com/thomhurst/TUnit) from 1.51.0 to
1.53.0.

<details>
<summary>Release notes</summary>

_Sourced from [TUnit's
releases](https://github.com/thomhurst/TUnit/releases)._

## 1.53.0

<!-- Release notes generated using configuration in .github/release.yml
at v1.53.0 -->

## What's Changed
### Other Changes
* feat(assertions): return typed value from IsAssignableTo<T> (#​6184)
by @​thomhurst in thomhurst/TUnit#6187
* fix: stop doubling backslashes in source-gen emitted FilePath (breaks
HTML report source links) by @​thomhurst in
thomhurst/TUnit#6193
* feat(assertions): add ContainsKey().And.Value drill-in for
dictionaries (#​6185) by @​thomhurst in
thomhurst/TUnit#6188
* fix(tests): snapshot ExecutionLog under lock to fix parallel race by
@​thomhurst in thomhurst/TUnit#6194
* fix(engine): run lifecycle hooks before test class construction
(#​6192) by @​thomhurst in thomhurst/TUnit#6195
* feat(assertions): inference-friendly pinned overload for covariant
[AssertionExtension] with own generic (#​5922) by @​thomhurst in
thomhurst/TUnit#6196
* feat: add DeferEnumeration to defer data-source expansion to runtime
(#​5833) by @​thomhurst in thomhurst/TUnit#6197
### Dependencies
* chore(deps): update tunit to 1.51.0 by @​thomhurst in
thomhurst/TUnit#6186
* chore(deps): update microsoft.testing to 18.8.0 by @​thomhurst in
thomhurst/TUnit#6191
* chore(deps): update aspire to 13.4.3 by @​thomhurst in
thomhurst/TUnit#6198


**Full Changelog**:
thomhurst/TUnit@v1.51.0...v1.53.0

Commits viewable in [compare
view](thomhurst/TUnit@v1.51.0...v1.53.0).
</details>

Updated [TUnit.AspNetCore](https://github.com/thomhurst/TUnit) from
1.51.0 to 1.53.0.

<details>
<summary>Release notes</summary>

_Sourced from [TUnit.AspNetCore's
releases](https://github.com/thomhurst/TUnit/releases)._

## 1.53.0

<!-- Release notes generated using configuration in .github/release.yml
at v1.53.0 -->

## What's Changed
### Other Changes
* feat(assertions): return typed value from IsAssignableTo<T> (#​6184)
by @​thomhurst in thomhurst/TUnit#6187
* fix: stop doubling backslashes in source-gen emitted FilePath (breaks
HTML report source links) by @​thomhurst in
thomhurst/TUnit#6193
* feat(assertions): add ContainsKey().And.Value drill-in for
dictionaries (#​6185) by @​thomhurst in
thomhurst/TUnit#6188
* fix(tests): snapshot ExecutionLog under lock to fix parallel race by
@​thomhurst in thomhurst/TUnit#6194
* fix(engine): run lifecycle hooks before test class construction
(#​6192) by @​thomhurst in thomhurst/TUnit#6195
* feat(assertions): inference-friendly pinned overload for covariant
[AssertionExtension] with own generic (#​5922) by @​thomhurst in
thomhurst/TUnit#6196
* feat: add DeferEnumeration to defer data-source expansion to runtime
(#​5833) by @​thomhurst in thomhurst/TUnit#6197
### Dependencies
* chore(deps): update tunit to 1.51.0 by @​thomhurst in
thomhurst/TUnit#6186
* chore(deps): update microsoft.testing to 18.8.0 by @​thomhurst in
thomhurst/TUnit#6191
* chore(deps): update aspire to 13.4.3 by @​thomhurst in
thomhurst/TUnit#6198


**Full Changelog**:
thomhurst/TUnit@v1.51.0...v1.53.0

Commits viewable in [compare
view](thomhurst/TUnit@v1.51.0...v1.53.0).
</details>

Dependabot will resolve any conflicts with this PR as long as you don't
alter it yourself. You can also trigger a rebase manually by commenting
`@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits
that have been made to it
- `@dependabot show <dependency name> ignore conditions` will show all
of the ignore conditions of the specified dependency
- `@dependabot ignore this major version` will close this PR and stop
Dependabot creating any more for this major version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop
Dependabot creating any more for this minor version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop
Dependabot creating any more for this dependency (unless you reopen the
PR or upgrade to it yourself)


</details>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
This was referenced Jun 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature]: xunit's DisableDiscoveryEnumeration

1 participant