[thomhurst]

Summary

Before: ReflectionTestDataCollector had two copies of the generic-class discovery logic: DiscoverGenericTests and DiscoverGenericTestsStreamingAsync. Every fix in this area (#7106, #7107, #7118) had to be made twice, and the copies had already drifted apart (cancellation checks, failure collection, error-message text).

After: one implementation. Behavior does not change.

Cause

DiscoverGenericTestsStreamingAsync was only reachable from CollectTestsStreamingAsync. That method is not part of ITestDataCollector and nothing calls it; it has been unused since the streaming interface member was removed (around #2872). So the "second copy" was dead code that still needed maintaining. The streaming chain also included DiscoverTestsInAssemblyStreamingAsync, DiscoverDynamicTestsStreamingAsync, ExecuteDynamicTestBuilderStreamingAsync and CreateFailedTestMetadataForDynamicTest, which were all unreachable too.

Fix

• Removed the unreachable streaming discovery chain (about 570 lines).
• In DiscoverGenericTests, the [GenerateGenericTest] loop and the class-row loop each built tests for one concrete instantiation with nearly identical code. Both now call a new AddTestsForInstantiation helper. The only differences between them were the class row sources and the method-row skip. Those are now parameters, and the skip already does nothing when methodRowTypeArguments is null, which is always the case on the class-row path. Exception handling is unchanged: tests added before a failure are kept, as before.
• GroupClassRowsByTypeArgumentsAsync lost its CancellationToken parameter, which was only ever passed CancellationToken.None on the live path.

Tests

• TUnit.Engine.Tests (Reflection mode, net10.0): Issue7106Tests 31/31, Issue7107Tests 28/28, GenericConstraintValidationTests 21/21, GenericMethodWithDataSourceTests 4/4.
• Discovery equivalence: ran TUnit.TestProject --reflection --list-tests on origin/main and on this branch. The two lists of 18,924 lines are identical once GUIDs and numbers from random data sources are normalized.
• Discovery perf (the same --list-tests --reflection over the whole TestProject, 10 alternating runs after warmup): median 1649 ms on main vs 1668 ms on this branch, which is within run-to-run noise (each set spans roughly 1450 to 1950 ms). The live path does the same work. The only change on it is one extra static call per instantiation.
• Source-generated mode is untouched.

Note for open PRs: #7149, #7154 and #7079 also edit this file. Any changes they make to DiscoverGenericTestsStreamingAsync can be dropped when rebasing.

Fixes #7167

Summary by CodeRabbit

• Bug Fixes
  • Improved reflection-based test discovery for generic classes and methods.
  • Prevented duplicate test cases when type information is inferred from method rows.
  • Improved handling and reporting of invalid generic test instantiations.
  • Ensured dynamically discovered tests are fully resolved before appearing in results.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-20T05:19:32.429617Z	00c74ed	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: d08bd00c-b1e4-4148-9fe1-4236047662e8

📥 Commits

Reviewing files that changed from the base of the PR and between c4b9977 and 00c74ed.

📒 Files selected for processing: 1

• src/TUnit.Engine/Discovery/ReflectionTestDataCollector.cs

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 1 remain after this review.

📝 Walkthrough

Reflection test discovery removes the streaming assembly, generic-test, and dynamic-test paths. Generic instantiation handling now uses a shared helper. Class-row grouping no longer accepts or checks a cancellation token.

Changes: Reflection discovery

Layer / File(s)	Summary
Generic instantiation handling src/TUnit.Engine/Discovery/ReflectionTestDataCollector.cs	Generic test and class-row instantiations use AddTestsForInstantiation. Class-row grouping no longer accepts or checks a cancellation token.
Streaming assembly discovery removal src/TUnit.Engine/Discovery/ReflectionTestDataCollector.cs	The streaming assembly and generic discovery methods are removed, along with the streaming-specific type-filtering distinction.
Streaming dynamic discovery removal src/TUnit.Engine/Discovery/ReflectionTestDataCollector.cs	The streaming dynamic discovery and builder methods, and the individual failure metadata factory, are removed.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Refactor

Suggested reviewers: theluckystrike

Merge Risk: ⚪ Minimal · up to 00c74

This refactor removes unused streaming discovery code and merges duplicated generic-instantiation logic without changing how tests are discovered or how failures are reported. No merge-blocking risk was found.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 21.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 1 files.	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 accurately identifies the refactor and the deduplication of reflection generic discovery, which is the primary change.
Linked Issues check		Issue #7167 is addressed. The diff removes the unused streaming discovery chain and leaves one live reflection path in DiscoverGenericTests. Both [GenerateGenericTest] instantiations and class-row…
Out of Scope Changes check		The changes are limited to ReflectionTestDataCollector. They remove the unreachable streaming discovery and dynamic-builder chain, consolidate generic instantiation handling, and remove a parameter …

Full details: Linked Issues check

Explanation

Issue #7167 is addressed. The diff removes the unused streaming discovery chain and leaves one live reflection path in DiscoverGenericTests. Both [GenerateGenericTest] instantiations and class-row instantiations call AddTestsForInstantiation. The helper handles concrete type creation, method-row duplicate skipping, method instantiation, and failed metadata. The reported reflection test results, normalized discovery-list comparison, and before/after hot-path measurements support the stated behavior and performance requirements.

Full details: Out of Scope Changes check

Explanation

The changes are limited to ReflectionTestDataCollector. They remove the unreachable streaming discovery and dynamic-builder chain, consolidate generic instantiation handling, and remove a parameter that only received CancellationToken.None. These changes directly support Issue #7167. No unrelated change is shown.

• Fix all pre-merge checks with AI

✨ Finishing Touches: 💡 1

📝 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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{A rabbit checks the test rows twice,
Then folds the stream away from sight.
Generic types now share one path,
Dynamic builders leave the math.
Discovery rests, the burrow’s bright.}

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Confidence Score: 5/5

[Low impact] The PR appears safe to merge.

Summary

Removes the unused streaming discovery chain and shares generic test construction through AddTestsForInstantiation.

• Generic test types now share one path for building their tests.
• Reflection discovery no longer carries a separate streaming path.

_{Reviews (1) · Last reviewed commit: "refactor(engine): remove dead streaming ..." · Reviewed by Greptile}

[github-actions]

Review

This is a clean dedup. I found no blocking issues.

• Dead code removal: CollectTestsStreamingAsync and its chain no longer appear anywhere in the PR head. No remaining references exist, and it was not part of ITestDataCollector. Removing it means each future generic-discovery fix only needs to be made once.
• AddTestsForInstantiation: It merges the [GenerateGenericTest] loop and the class-row loop. IsInferredFromMethodRows is a no-op when methodRowTypeArguments is null, so the class-row path behaves as before. Tests added before a throw are kept, as before.
• Dropped CancellationToken: Only CancellationToken.None was ever passed on the live path, so this changes nothing.
• Reflection-only change: It touches only the reflection collector. The source-generator path is unaffected, so the dual-mode rule doesn't require changes elsewhere.

Suggestion (optional): The classRowSources: null and methodRowTypeArguments: null arguments at the call sites are easy to misread. Named arguments are already used at one site, so using them at both would be more consistent.

I didn't run the tests myself. The PR description reports Issue7106, Issue7107, constraint and method-datasource suites passing, plus a discovery-equivalence check, which is good evidence.

LGTM.