#5733 handling all arguments for Fact and Theory

[inyutin-maxim]

Description

handling all arguments for Fact and Theory

Related Issue

Fixes #5733

Fixes #

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update
• Performance improvement
• Refactoring (no functional changes)

Checklist

Required

• I have read the Contributing Guidelines
• If this is a new feature, I started a discussion first and received agreement
• My code follows the project's code style (modern C# syntax, proper naming conventions)
• I have written tests that prove my fix is effective or my feature works

TUnit-Specific Requirements

• Dual-Mode Implementation: If this change affects test discovery/execution, I have implemented it in BOTH:
  • Source Generator path (TUnit.Core.SourceGenerator)
  • Reflection path (TUnit.Engine)
• Snapshot Tests: If I changed source generator output or public APIs:
  • I ran TUnit.Core.SourceGenerator.Tests and/or TUnit.PublicAPI tests
  • I reviewed the .received.txt files and accepted them as .verified.txt
  • I committed the updated .verified.txt files
• Performance: If this change affects hot paths (test discovery, execution, assertions):
  • I minimized allocations and avoided LINQ in hot paths
  • I cached reflection results where appropriate
• AOT Compatibility: If this change uses reflection:
  • I added appropriate [DynamicallyAccessedMembers] annotations
  • I verified the change works with dotnet publish -p:PublishAot=true

Testing

• All existing tests pass (dotnet test)
• I have added tests that cover my changes
• I have tested both source-generated and reflection modes (if applicable)

Additional Notes

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[claude]

Code Review: PR #5734 — "handling all arguments for Fact and Theory"

The PR addresses a real bug (#5733) where the xUnit-to-TUnit migrator was silently dropping named arguments on [Fact] and [Theory] other than Skip (e.g. [Fact(DisplayName = "...")] was migrated to [Test] without a [DisplayName("...")]). The general direction — looping over all named arguments rather than handling only Skip — is correct. However, the implementation has several correctness and architectural concerns.

---

Critical Bug: Blind Attribute Name Forwarding

The new code treats every named argument as if its name directly maps to a TUnit attribute name:

additionalAttributes.Add(new AdditionalAttribute(
    argument.NameEquals!.Name.Identifier.ValueText, ...));

This is not safe as a general strategy. xUnit and TUnit do not share the same attribute argument namespace. For example:

• [Fact(Timeout = 5000)] → emits [Timeout(5000)] — but xUnit's Timeout is in milliseconds and TUnit's [Timeout] may have a different signature or not exist at all.
• Any unrecognized xUnit argument would emit a broken/invalid attribute silently.

Suggested fix: Use an explicit allowlist/mapping table (similar to how NUnitTwoPhaseAnalyzer maps TestName → DisplayName):

private static readonly Dictionary<string, string> FactArgToTUnitAttr = new()
{
    { "Skip", "Skip" },
    { "DisplayName", "DisplayName" },
    // Timeout has no direct TUnit equivalent — needs TODO comment
};

For unknown arguments, emit a // TODO: TUnit migration — unrecognized argument '{name}' could not be converted comment rather than silently dropping or blindly forwarding.

---

Silent Data Loss: Positional Arguments Discarded

if (argument.NameEquals is null) { continue; }

Positional arguments are silently dropped with no warning. While xUnit's Fact/Theory don't normally take positional args, any edge case (subclass attributes, unusual invocations) will be silently broken. A // TODO diagnostic comment would be safer.

---

Stale Comments (Misleading for Future Maintainers)

Two comments now contradict the code:

1. The final return block still reads // Remove the Skip argument but now applies to all arguments, not just Skip.
2. The early-return null-ArgumentList path has // Remove any arguments but there are no arguments to remove — // No arguments — plain [Test] would be accurate.

These will confuse future maintainers.

---

Missing Tests

No test cases were added (confirmed by the PR checklist). The following are needed at minimum:

• [Fact(DisplayName = "My Test")] → [Test] + [DisplayName("My Test")]
• [Theory(DisplayName = "My Theory")] → [Test] + [DisplayName("My Theory")]
• Regression: existing Skip behavior still works

Without tests, the next refactor could silently reintroduce the original bug.

---

Minor: Eager List Allocation

new List<AdditionalAttribute>(3) is allocated for every [Fact]/[Theory] attribute, even when all arguments are positional (and thus skipped). Lazy allocation (only create the list when a known-good argument is found) would avoid unnecessary allocations in the common case. Low priority, but worth noting given TUnit's performance focus.

---

Summary

Bug fix direction	Correct
Implementation safety	Needs explicit argument mapping — blind forwarding risks emitting invalid TUnit attributes
Silent failures	Positional args and unknown named args are dropped without any diagnostic
Test coverage	None added
Code clarity	Stale comments mislead future maintainers

Recommended before merging:

1. Add an explicit string → string argument-to-attribute mapping with an unknown-arg fallback (TODO comment)
2. Handle positional args with a TODO comment rather than silently skipping
3. Fix the two stale comments
4. Add test cases for DisplayName on both Fact and Theory

[claude]

Code Review: PR #5734 (Updated) — "handling all arguments for Fact and Theory"

This is a follow-up review comparing against my previous comments.

---

What Was Fixed ✅

All critical concerns from the previous review have been addressed:

1. Blind attribute forwarding eliminated — The new explicit mapping dictionary _xUnitFactAndTheoryToTUnitArguments with TryGetValue is the right approach. Unknown xUnit arguments are now silently dropped rather than blindly emitted as invalid TUnit attributes.

2. Tests added — DisplayNamed_Test_Attributes_Can_Be_Fixed and Timeout_Test_Attributes_Can_Be_Fixed cover both Fact and Theory variants. The regression test for Skip behaviour was already present.

3. Eager allocation reduced — Using [] collection expression instead of new List<AdditionalAttribute>(3) removes the unnecessary pre-allocation.

4. Stale "Remove the Skip argument" comment — Gone now that the block was rewritten.

---

Timeout Semantics Verified ✅

I checked TimeoutAttribute.cs — TUnit's [Timeout(int timeoutInMilliseconds)] uses milliseconds, matching xUnit's Timeout property. The migration is semantically correct and the test's use of 3600 is valid.

---

Minor Remaining Items

1. Positional arguments silently dropped (low priority)

xUnit's Fact/Theory don't take positional args in practice, but a TODO comment would protect future maintainers and custom subclasses:

2. Empty list vs null inconsistency (cosmetic)

When ArgumentList is non-null but no arguments match, the returned result has AdditionalAttributes = [] (empty list). The rest of the codebase (NUnit analyzer, the other xUnit path at line 958) normalizes to null when empty:

This is not a bug — MigrationTransformer.cs:607 correctly guards with != null && Count > 0. But for consistency with the established pattern, you could apply the same normalization. Not a blocker.

---

Summary

The previous critical concerns are resolved. The implementation is now correct and safe. The remaining items are minor — a TODO comment for dropped positional args would be a good defensive addition, but is not required for correctness.

Good work addressing the feedback.

[claude]

Code Review: PR #5734 (Updated) — "handling all arguments for Fact and Theory"

This is a follow-up review comparing against my previous comments.

---

What Was Fixed

All critical concerns from the previous review have been addressed:

1. Blind attribute forwarding eliminated -- The new explicit mapping dictionary _xUnitFactAndTheoryToTUnitArguments with TryGetValue is the right approach. Unknown xUnit arguments are now silently dropped rather than blindly emitted as invalid TUnit attributes.

2. Tests added -- DisplayNamed_Test_Attributes_Can_Be_Fixed and Timeout_Test_Attributes_Can_Be_Fixed cover both Fact and Theory variants. The regression test for Skip behaviour was already present.

3. Eager allocation reduced -- Using [] collection expression instead of new List<AdditionalAttribute>(3) removes the unnecessary pre-allocation.

4. Stale "Remove the Skip argument" comment -- Gone now that the block was rewritten.

---

Timeout Semantics Verified

I checked TimeoutAttribute.cs -- TUnit's [Timeout(int timeoutInMilliseconds)] uses milliseconds, matching xUnit's Timeout property. The migration is semantically correct and the test's use of 3600 is valid.

---

Minor Remaining Items

1. Positional arguments silently dropped (low priority)

xUnit's Fact/Theory don't take positional args in practice, but a TODO comment would protect future maintainers handling custom subclasses:

if (argument.NameEquals is null)
{
    // TODO: positional arguments on Fact/Theory are not supported -- skipping
    continue;
}

2. Empty list vs null inconsistency (cosmetic)

When ArgumentList is non-null but no arguments match, the returned result has AdditionalAttributes = [] (empty list). The rest of the codebase (NUnit analyzer, the other xUnit path at line 958) normalizes to null when empty:

AdditionalAttributes = additionalAttributes.Count > 0 ? additionalAttributes : null

This is not a bug -- MigrationTransformer.cs:607 correctly guards with != null && Count > 0. But for consistency with the established pattern, the same normalization could be applied. Not a blocker.

---

Summary

The previous critical concerns are resolved. The implementation is now correct and safe. The remaining items are minor -- a TODO comment for dropped positional args would be a good defensive addition, but is not required for correctness.

Good work addressing the feedback.

[thomhurst]

Thanks @inyutin-maxim !