Xunit 2.8.1 broke compatibility with skipping tests from fixtures

[adamreeve]

I reported this at https://github.com/xunit/xunit/issues/2965 but was told it's an issue for SkippableFact. Is this something that can be fixed or do we need to find another way to achieve this?

Test class:

using Xunit;

namespace XunitTest;

public class UnitTest1 : IClassFixture<UnitTest1.MyFixture>
{
    class MyFixture
    {
        public MyFixture()
        {
            bool runTests = false;
            Skip.If(!runTests, "Skipping");
        }
    }

    [SkippableFact]
    public void Test1()
    {
        Assert.Equal(1, 1);
    }
}

csproj:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFramework>net8.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>

    <IsPackable>false</IsPackable>
    <IsTestProject>true</IsTestProject>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.10.0" />
    <PackageReference Include="xunit" Version="2.8.1" />
    <PackageReference Include="xunit.runner.visualstudio" Version="2.8.1">
      <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
      <PrivateAssets>all</PrivateAssets>
    </PackageReference>
    <PackageReference Include="xunit.skippablefact" Version="1.4.13" />
  </ItemGroup>

</Project>

With Xunit 2.8.0, running dotnet test gives:

$ dotnet test
  Determining projects to restore...
  Restored /home/adam/dev/tmp/XunitTest/XunitTest.csproj (in 196 ms).
  XunitTest -> /home/adam/dev/tmp/XunitTest/bin/Debug/net8.0/XunitTest.dll
Test run for /home/adam/dev/tmp/XunitTest/bin/Debug/net8.0/XunitTest.dll (.NETCoreApp,Version=v8.0)
Microsoft (R) Test Execution Command Line Tool Version 17.8.0 (x64)
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
[xUnit.net 00:00:00.12]     XunitTest.UnitTest1.Test1 [SKIP]
  Skipped XunitTest.UnitTest1.Test1 [1 ms]

Skipped! - Failed:     0, Passed:     0, Skipped:     1, Total:     1, Duration: < 1 ms - XunitTest.dll (net8.0)

With 2.8.1 I get:

$ dotnet test
  Determining projects to restore...
  Restored /home/adam/dev/tmp/XunitTest/XunitTest.csproj (in 211 ms).
  XunitTest -> /home/adam/dev/tmp/XunitTest/bin/Debug/net8.0/XunitTest.dll
Test run for /home/adam/dev/tmp/XunitTest/bin/Debug/net8.0/XunitTest.dll (.NETCoreApp,Version=v8.0)
Microsoft (R) Test Execution Command Line Tool Version 17.8.0 (x64)
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
[xUnit.net 00:00:00.13]     XunitTest.UnitTest1.Test1 [FAIL]
  Failed XunitTest.UnitTest1.Test1 [1 ms]
  Error Message:
   Class fixture type 'XunitTest.UnitTest1+MyFixture' threw in its constructor
---- Xunit.SkipException : Skipping
  Stack Trace:

----- Inner Stack Trace -----
   at XunitTest.UnitTest1.MyFixture..ctor() in /home/adam/dev/tmp/XunitTest/UnitTest1.cs:line 12
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)

Failed!  - Failed:     1, Passed:     0, Skipped:     0, Total:     1, Duration: < 1 ms - XunitTest.dll (net8.0)

[AArnott]

Interesting. I never considered skipping from a fixture, so that wasn't an intentional feature. I hope we can get it working again for you, but I don't anticipate having time to investigate soon. Do you have interest in investigating this?

[adamreeve]

I don't really have time to investigate this either. We can work around it by adding a constructor to the test class and skipping from there instead, so I'm happy if you want to close this as not supported, but I thought it was worth reporting at least.

[bradwilson]

More discussion back in the original issue, but suffice to say that I believe this isn't a bug in SkippableFact. Throwing from fixtures and having it propagate down into the test itself was an unintentional bug that got fixed in 2.8.1. Throwing fixtures should've always prevented the test from running, and that's now the case in 2.8.1 (the previously bug was that it only failed if you accepted that fixture from the constructor of the unit test).