[kenny-ish]

Fixes #7076

#7111 now scopes class- and method-level [Arguments] rows to the test they produced (TestBuilder.ScopeRowAttributesToTest), which covers what this PR originally changed, so I reworked it on top of main as discussed.

What main still missed: the failed-discovery metadata paths in ReflectionTestDataCollector (CreateFailedTestMetadata, CreateFailedMethodGenericMetadata and CreateFailedTestMetadataForDynamicBuilder) returned method.GetCustomAttributes() as is, so a test whose discovery failed still carried every [Arguments] row, including each row's Skip, DisplayName and Categories. They now go through ScopeRowAttributesToTest with no producing row, which drops the row-scoped data sources the same way the normal path does.

Tests:

• Issue7076Tests (all engine modes): per-row Skip and DisplayName for method rows, class rows and inherited rows, the class constructor receiving the method data source, and the three failed-metadata factories.
• TestVariantTests.CreateTestVariant_DoesNotInheritParentRowSettings: a variant does not inherit the parent row's DisplayName.

With main's ReflectionTestDataCollector, Failed_Metadata_Filters_Method_DataSources fails; the other tests already pass on current main and are kept as regression coverage for #7076.

Summary by CodeRabbit

• Bug Fixes

  • Fixed argument-row handling so skip settings and display names apply only to their intended rows, including class-level and inherited arguments.
  • Improved class constructor data-source handling.
  • Corrected failed-test metadata so data sources from non-generated rows are not reported as applicable.
  • Ensured test variants do not inherit the parent row’s display name or argument settings.

• Tests

  • Added regression coverage for argument rows, constructor data sources, failed-test metadata, and test variants.

[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-17T13:55:53.055146Z	8a937e1	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: a3f2e7b4-e3ea-460e-9aac-3c2fb3514cb4

📥 Commits

Reviewing files that changed from the base of the PR and between 4528ac7 and 2b49da9.

📒 Files selected for processing: 1

• tests/TUnit.Engine.Tests/Issue7076Tests.cs

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

📝 Walkthrough

Reflection discovery now scopes argument-level settings to the matching row. Failed metadata excludes row data-source attributes. Regression tests cover method, class, inherited, constructor, metadata, and variant behavior.

Changes: Row argument handling

Layer / File(s)	Summary
Filter attributes on failed metadata src/TUnit.Engine/Discovery/ReflectionTestDataCollector.cs	Generic method resolution, ordinary discovery, and dynamic builder failures now use filtered method attributes without a data row.
Verify row-specific argument behavior tests/TUnit.TestProject/Bugs/_7076/ArgumentsRowSkipTests.cs, tests/TUnit.Engine.Tests/Issue7076Tests.cs	Tests verify row-specific display names and skip behavior for method, class, and inherited arguments.
Verify constructor and variant data handling tests/TUnit.TestProject/Bugs/_7076/ArgumentsRowSkipTests.cs, tests/TUnit.Engine.Tests/Issue7076Tests.cs, tests/TUnit.TestProject/TestVariantTests.cs	Tests verify method data passed to class constructors, filtered failed metadata, and test variant display settings.

Priority: ➖ Normal

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

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: thomhurst

Merge Risk: ⚪ Minimal · up to 2b49d

No actionable merge-blocking risk was found in the reviewed changes.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 12.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 25 functions across 8 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 clearly and concisely describes the main engine change: removing row-scoped data sources from failed test metadata.
Linked Issues check		Issue #7076 requires a reflection-mode [Arguments] row with Skip to affect only that row. The reviewed head retains the normal row-scoping behavior on main and adds filtering for failed-discover…
Out of Scope Changes check		The production change removes row-scoped data-source attributes from failed metadata. The tests cover the linked issue and related metadata and variant regressions. The reviewed changes show no unrela…

Full details: Linked Issues check

Explanation

Issue #7076 requires a reflection-mode [Arguments] row with Skip to affect only that row. The reviewed head retains the normal row-scoping behavior on main and adds filtering for failed-discovery metadata in CreateFailedTestMetadata, CreateFailedMethodGenericMetadata, and CreateFailedTestMetadataForDynamicBuilder. The added regression tests cover method, class, and inherited rows, including row-specific skip behavior.

Full details: Out of Scope Changes check

Explanation

The production change removes row-scoped data-source attributes from failed metadata. The tests cover the linked issue and related metadata and variant regressions. The reviewed changes show no unrelated feature or API change.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• 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 hops where row names gleam
One marked row skips, two rows stream
Constructors carry data true
Failed metadata sheds rows too
Variants keep their names in view
Thump, thump—the tests all pass anew!}

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

[github-actions]

Review

The fix looks correct and well-scoped. Reflection mode now matches the source generator (GetTestAttributes) by leaving method-level IDataSourceAttributes out of the shared attribute list. TestBuilder appends the per-case data source, so each row's Skip, DisplayName and Categories apply only to that row. This also removes the duplicate attribute instances per test. I checked the other consumers of GetAllAttributes (event-receiver building, class-constructor lookup, TestRegistry) and found none that depend on method-level data sources being in the list. Class-level and assembly-level behaviour is unchanged.

The tests cover both the Skip and DisplayName cases and run in all modes via InvokableTestBase.

Minor suggestions, none blocking:

• The loop is allocation-friendly. If GetCustomAttributes() is hot, attributes.Capacity could be pre-sized, but I wouldn't bother.
• Consider adding a case for a method with mixed [Arguments] and [MethodDataSource] sources. It would confirm that per-row settings stay isolated across different source types.
• The PR checklist leaves the snapshot items unchecked. That's fine, since the generator is unchanged.

LGTM.

[greptile-apps]

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge; no new actionable issue was found.

Summary

Failed-discovery metadata now drops row-scoped attributes when no row produced the test.

• Failed tests no longer inherit argument rows that produced no test.

_{Reviews (4) · Last reviewed commit: "test: assert the failed-metadata factori..." · Reviewed by Greptile}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8a937e19f9

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[thomhurst]

Automated code review. The method-level fix looks right and matches the source generator's GetTestAttributes filter. The comments below cover related gaps; the first four are in code outside this diff, so they're anchored on this change with file references.

[github-actions]

Review

The fix is sound. Method-level data source attributes no longer leak into every row's attribute list, so [Arguments(Skip=...)], DisplayName and Categories stay on their own row. Reflection mode and the source generator now produce the same attribute list. I read the diff and the other consumers of IDataSourceAttribute in the engine, but I did not build the repo or run any tests.

Points to check

1. The description doesn't match the code. It says class-level and assembly-level attributes are unchanged. The reflection loops filter IDataSourceAttribute on all three levels, and GetTestAttributes now applies the .Where after the Concat, so class-level data sources are dropped too. That is probably intended, and TestBuilder now appends ClassDataSourceAttribute, which is why the snapshots lose the class-level ClassDataSource entries. But it is a wider behaviour change than described. Please update the description, and confirm that nothing else reads class-level data sources from TestDetails attributes. ClassConstructorHelper.GetDataSourceAttribute and InitializeAttributesAsync take the first or any IDataSourceAttribute in the list, so they now depend on TestBuilder having appended the right one.

2. Duplicated filtering. The same foreach … is not IDataSourceAttribute loop is written three times in ReflectionAttributeExtractor, and TestRegistry has a fourth filter. A small local helper like AddNonDataSource(list, IEnumerable<Attribute>) would remove the repetition. The rule "data sources are supplied per case by TestBuilder" would then live in one place instead of drifting between the generator, the extractor and the registry.

3. CreateTestVariant. It now strips all data source attributes from the variant's attribute list. Please confirm the variant path supplies its own data source, so that variants of parameterised tests still get their arguments. A regression test for that would help.

4. Failed-metadata paths. The three CreateFailed* factories now call GetAllAttributes (class and assembly attributes included) instead of method.GetCustomAttributes(). This makes them consistent with the other paths, but it changes what failed tests report. It deserves a mention in the description.

Tests: the snapshot updates look consistent. Running the new per-row Skip/DisplayName tests in both source-gen and reflection modes would confirm parity.

Overall this looks good to me once point 1 is confirmed and the description is corrected.

[thomhurst]

Thanks @kenny-ish - Could you take a look at the p1 concern and the merge conflicts?

[kenny-ish]

After #7111, most of this PR is already covered on main: ScopeRowAttributesToTest scopes both class and method rows, class first and then method, so the p1 display-name concern no longer applies. I ran this PR's regression tests on current main (99acef4) and the only failure left is Failed_Metadata_Filters_Method_DataSources: the failed-discovery metadata paths in ReflectionTestDataCollector still use the unfiltered method attributes. I can rework this PR down to that fix plus the #7076 regression tests on top of main, or close it if you'd rather cover it separately. Which would you prefer?

[thomhurst]

Reworking and tests sounds good 😄

[kenny-ish]

Reworked on top of main. The PR now only contains the failed-metadata fix in ReflectionTestDataCollector and the #7076 regression tests; the merge commit takes main's version of everything #7111 already covers. Details are in the updated description.

[github-actions]

Review

The change is small and correct. The three failed-discovery metadata factories in ReflectionTestDataCollector (CreateFailedTestMetadata, CreateFailedMethodGenericMetadata, CreateFailedTestMetadataForDynamicBuilder) now go through a shared GetFailedTestAttributes helper. That helper calls TestBuilder.ScopeRowAttributesToTest(..., null, null), so a failed test no longer carries every [Arguments] row's Skip, DisplayName and Categories. I read ScopeRowAttributesToTest on the PR head. It is internal static, and passing null for both data sources is handled: it only drops the row-scoped attributes.

Tests: Issue7076Tests covers the engine modes and the three factories. Failed_Metadata_Filters_Method_DataSources is the test that fails on main without this fix. I did not run the tests.

Minor suggestions, not blocking:

• Failed_Metadata_Filters_Method_DataSources calls the private factories by reflection with string names. If one is renamed, GetMethod(...)! will throw a NullReferenceException with no useful message. Assert factory.ShouldNotBeNull(name) first. The trailing await Task.CompletedTask is unneeded noise too.
• Making the factories internal and using InternalsVisibleTo would be sturdier than reflecting over private members. The Native AOT and trimming concern doesn't apply, because this is test code.

Approve from my side.

[kenny-ish]

Applied the two test nits from the review: the factory lookup is now asserted with its name before it is invoked, and the test returns Task.CompletedTask instead of awaiting it.

[github-actions]

Review

Small, focused fix. The three failed-discovery factories in ReflectionTestDataCollector now share GetFailedTestAttributes, which calls TestBuilder.ScopeRowAttributesToTest(..., null, null). I checked that the helper returns the original array unchanged when there are no row-scoped attributes and appends nothing when both sources are null. So the failed path now drops [Arguments] rows the same way the normal path does, and it adds no allocation for tests without rows. Routing all three factories through one helper is better than patching each lambda.

Notes (non-blocking):

• Failed_Metadata_Filters_Method_DataSources finds private static factories by name through reflection. A rename would give a clear ShouldNotBeNull(name) failure, so it is acceptable. Making the factories internal (the engine already exposes internals to tests) would give compile-time safety.
• The test is declared as a [Test] in an InvokableTestBase class but never uses testMode, so it runs once per mode with the same result. It is harmless and cheap.
• The new test-project cases cover the regression well, including the class-level and inherited rows from [Bug]: Reflection mode: Arguments(Skip = "...") skips all rows, not just that row #7076.

LGTM.

[thomhurst]

Thanks @kenny-ish !