Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support xunit.v3 in Microsoft.DotNet.XUnitExtensions #15668

Merged
merged 7 commits into from
Mar 28, 2025

Conversation

Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented Mar 26, 2025

Part of #15654

There are some types that used to be present in Microsoft.DotNet.XUnitExtensions but no longer present in Microsoft.DotNet.XUnit3Extensions:

  1. ConditionalFactAttribute and ConditionalTheoryAttribute:
    • If using the parameterless constructor of the attribute, use FactAttribute and TheoryAttribute instead, and replace throw new SkipTestException(...) with Assert.Skip(...).
    • @ViktorHofer Do you know for the non-parameterless constructor? Is there a native replacement in xunit.v3 for that? Or maybe we should keep the attributes for this case? 😕
  2. ParallelTheoryAttribute: No replacement for that currently. I can't find any usages of it with grep.app. If requested by someone, we can work on adding it.

Manually tested scenarios:

ConditionalClassAttribute

namespace TestForArcadev3;

[ConditionalClass(typeof(UnitTest1), nameof(MyCondition))]
public class UnitTest1
{
    public static bool MyCondition => true;

    [Fact]
    public void Test1() => Assert.Fail("UnitTest1");
}

[ConditionalClass(typeof(UnitTest2), nameof(MyCondition))]
public class UnitTest2
{
    public static bool MyCondition => false;

    [Fact]
    public void Test1() => Assert.Fail("UnitTest2");
}

Running with --filter-query /[category!=failing]:

  1. UnitTest1.Test1 is run and fails as expected.
  2. UnitTest2.Test1 isn't run.

DotNetOnlyFact

namespace TestForArcadev3;

public class UnitTest1
{
    [DotNetOnlyFact]
    public void Test1() => Assert.Fail("UnitTest1");
}
  1. dotnet test -f net8.0: Test is run and fails.
  2. dotnet test -f net472: Test isn't run and executable finishes with exit code 8 (zero tests ran)

@akoeplinger
Copy link
Member

Do you know for the non-parameterless constructor? Is there a native replacement in xunit.v3 for that? Or maybe we should keep the attributes for this case? 😕

yeah I think we should keep it. that said, now that I look at it I wonder why ConditionalFact/Theory don't use the same approach as https://github.com/dotnet/arcade/blob/main/src/Microsoft.DotNet.XUnitExtensions/src/Attributes/LinuxOnlyFactAttribute.cs where we set the Skip property in the ctor instead of the current complicated Discoverer + failing trait. Was that not available in earlier xunit versions?

@Youssef1313
Copy link
Member Author

@akoeplinger I don't know about the history. But there is a fundamental difference between both approaches. Using Skip will cause the test to always be skipped when the condition isn't met (and the test will be reported in test results). However, using failing trait depends on whether or not you filter by category!=failing:

  1. If category!=failing filter is present, I think the test won't run and will also not be reported in test results at all.
  2. If the filter isn't present, the test will be run regardless.

I'd tend to avoid making fundamental/behavioral changes as part of a simple migration to xunit.v3.

@akoeplinger
Copy link
Member

Yeah. It's just that we've been thinking of removing the need for the failing trait since it's confusing: #15195

I agree we can tackle that separately.

@Youssef1313
Copy link
Member Author

@akoeplinger Thanks! Is this good to merge soon then (probably with a second review)?

@akoeplinger akoeplinger merged commit 4bb7855 into main Mar 28, 2025
11 checks passed
@akoeplinger akoeplinger deleted the dev/ygerges/xunit.v3 branch March 28, 2025 11:25
@Youssef1313 Youssef1313 mentioned this pull request Mar 28, 2025
18 tasks
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.

3 participants