Enable ConditonalFact and ConditionalTheory for XUnit v3

[jkotas]

dotnet/runtime has many thousands of tests that use these. We do not want to be changing all of them to alternatives.

[jkotas]

Context: dotnet/runtime#120234 (comment)

[agocke]

This is definitely required, but I'm not sure it's sufficient. I haven't dug in yet, but I'm not sure the old mechanism actually skips tests in xunit3. It might just apply a category but not skip.

[Youssef1313]

I think that mechanism adds a trait, regardless of whether it's xunit 2 or 3, and it's then responsibility when running the tests to filter by traits. e.g:

https://github.com/dotnet/runtime/blob/a12d21630fffc479582fc914f444e2c83d327e3b/src/tests/Common/tests.targets#L47

[Youssef1313]

Is it very hard for dotnet/runtime to push forward to using the more standard xunit.v3 features?

[jkotas]

Is it very hard for dotnet/runtime to push forward to using the more standard xunit.v3 features?

Do you mean Assert.Skip?

Two reasons:

• We use ConditionalFacts and ConditionalTheory on thousands of tests. We do not want to couple large scale cleanups with xunit.3 migration.
• Specifying the test pre-conditions in an attribute reads better than including the test pre-conditions in the test implementation.

[Youssef1313]

Do you mean Assert.Skip?

Yup. IIRC, the primary reason we didn't originally include these features in Arcade when I chatted with @ViktorHofer is that xunit.v3 has built-in equivalents that should be used instead. But if at this point it's too big to move, and readability is better as-is, I'm fine including the Arcade changes (if @ViktorHofer is okay with that as well).

[Youssef1313]

Also not that at least the parameterless constructor of the [ConditionalFact] attribute isn't clearner or more readable, because at the end you have to throw SkipTestException inside the test method with the right condition. For example: https://github.com/dotnet/runtime/blob/a12d21630fffc479582fc914f444e2c83d327e3b/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/TimeZoneInfoTests.cs#L2738-L2741

I haven't looked at the other constructor overloads, but probably they do encode the condition already and there is no need to throw SkipTestException in the test method.

[akoeplinger]

xunit v3 also supports SkipType+SkipWhen properties on Fact/Theory now so I think we could set those instead in our ConditionalFact/ConditionalTheory and get rid of the custom discoverers? they only support a single property name though, but I think passing multiple is somewhat rare.

Also not that at least the parameterless constructor of the [ConditionalFact] attribute isn't clearner or more readable, because at the end you have to throw SkipTestException inside the test method with the right condition

Yes I agree, it doesn't make sense to still use the parameterless ctor now that we have Assert.Skip.

[jkotas]

I think we could set those instead in our ConditionalFact/ConditionalTheory and get rid of the custom discoverers?

It requires you to set reason, so it would be wordier.

[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]

would turn into

[Fact(Skip="Depends on RemoteExecutor", SkipType=typeof(RemoteExecutor), SkipUnless=nameof(RemoteExecutor.IsSupported))]

[jkotas]

it doesn't make sense to still use the parameterless ctor now that we have Assert.Skip.

+1

[akoeplinger]

It requires you to set reason

I meant we would keep our ConditionalFact attribute but just let it set SkipType/SkipWhen and it could set some dummy Skip reason.

[agocke]

xunit v3 also supports SkipType+SkipWhen properties on Fact/Theory now so I think we could set those instead in our ConditionalFact/ConditionalTheory and get rid of the custom discoverers?

Agreed. The main problem with the discoverer-based approach is that you have to pass these filters manually. The best part of the new system will be that we can just run the exe and get the results. Having to set special command line args to prevent the testing from failing partially defeats the point.