Move unsupported target conditions to IL in mcc tests

[tomeksowi]

CLRTestTargetUnsupported for not-windows added in #111887 to jit64_2.csproj is not respected when CLR tests are built with BuildAllTestsAsStandalone=true.

Part of #84834, cc @dotnet/samsung

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

[jkotas]

Can we fix the test scripts instead of adding these redundant annotations?

[tomeksowi]

Can we fix the test scripts instead of adding these redundant annotations?

That would require adding [ConditionalFact] to each test method which would be equally redundant, unless there's some way to mass-annotate that I don't know of.

I removed the CLRTestTargetUnsupported from the merged target project, it's not necessary anymore.

EDIT: tests failing, it's probably necessary, I'll try [ConditionalFact]s

[tomeksowi]

I removed the CLRTestTargetUnsupported from the merged target project, it's not necessary anymore.

EDIT: tests failing, it's probably necessary, I'll try [ConditionalFact]s

[ConditionalFact]s look messy in IL. The test failures could be side-stepped with <RequiresProcessIsolation> but that defeats the purpose of a merged test target. I reverted CLRTestTargetUnsupported in the merged target. I ran out of ideas to make it less redundant, it probably has to stay this way.

[jkotas]

@jkoritzinsky Is there a way to make CLRTestTargetUnsupported work for BuildAllTestsAsStandalone=true?

[jkoritzinsky]

I'd recommend using ConditionalFact as I want us to move to the attributes overall. There is a nice syntax for IL that I found that we've used for a few tests. Attributes will work for all cases.

[tomeksowi]

I'd recommend using ConditionalFact as I want us to move to the attributes overall.

I tried ConditionalFact but apparently it needs XUnitWrapperGenerator to pick up the attributes and generate Main method.

The only thing I got working so far with and without BuildAllTestsAsStandalone=true and without a redundant CLRTestTargetUnsupported tag in ilproj is to make your own Main():

.method public static int32 Main()
{
  .entrypoint
  .maxstack  1

  call       bool [TestLibrary]TestLibrary.PlatformDetection::get_IsWindows()
  brtrue.s DoTest
  ldc.i4 100
  ret

DoTest:
  call       int32 MCCTest.MyClass::Test()
  ret
}

The ConditionalFacts in IL CLR tests have a redundant condition in the build project which we're trying to avoid, e.g.

runtime/src/tests/JIT/Methodical/xxobj/sizeof/sizeof32_Target_64Bit_and_arm.il

Lines 41 to 50 in 42423ef

	// [ConditionalFact(typeof(TestLibrary.PlatformDetection), nameof(TestLibrary.PlatformDetection.IsNotX86Process))]
	.custom instance void [Microsoft.DotNet.XUnitExtensions]Xunit.ConditionalFactAttribute::.ctor(class [System.Runtime]System.Type,
	string[]) = ( 01 00 61 54 65 73 74 4C 69 62 72 61 72 79 2E 50 // ..aTestLibrary.P
	6C 61 74 66 6F 72 6D 44 65 74 65 63 74 69 6F 6E // latformDetection
	2C 20 54 65 73 74 4C 69 62 72 61 72 79 2C 20 56 // , TestLibrary, V
	65 72 73 69 6F 6E 3D 30 2E 30 2E 30 2E 30 2C 20 // ersion=0.0.0.0,
	43 75 6C 74 75 72 65 3D 6E 65 75 74 72 61 6C 2C // Culture=neutral,
	20 50 75 62 6C 69 63 4B 65 79 54 6F 6B 65 6E 3D // PublicKeyToken=
	6E 75 6C 6C 01 00 00 00 0F 49 73 4E 6F 74 58 38 // null.....IsNotX8
	36 50 72 6F 63 65 73 73 00 00 ) // 6Process..

runtime/src/tests/JIT/Methodical/xxobj/sizeof/sizeof32_Target_64Bit_and_arm_r.ilproj

Line 5 in 42423ef

<CLRTestTargetUnsupported Condition="'$(TargetArchitecture)' == 'x86'">true</CLRTestTargetUnsupported>

There is a nice syntax for IL that I found that we've used for a few tests. Attributes will work for all cases.

Can you point me to these tests that avoid the above-mentioned problems?

[filipnavara]

There is a nice syntax for IL

For reference,

// [ConditionalFact(typeof(TestLibrary.PlatformDetection), nameof(TestLibrary.PlatformDetection.Is32BitProcess))]
.custom instance void [Microsoft.DotNet.XUnitExtensions]Xunit.ConditionalFactAttribute::.ctor(class [System.Runtime]System.Type, string[]) =
{
    type ([TestLibrary]TestLibrary.PlatformDetection)
    string[1] ('Is32BitProcess')
}

[tomeksowi]

// [ConditionalFact(typeof(TestLibrary.PlatformDetection), nameof(TestLibrary.PlatformDetection.Is32BitProcess))]
.custom instance void [Microsoft.DotNet.XUnitExtensions]Xunit.ConditionalFactAttribute::.ctor(class [System.Runtime]System.Type, string[]) =
{
    type ([TestLibrary]TestLibrary.PlatformDetection)
    string[1] ('Is32BitProcess')
}

Thanks, it looks nicer. Still the problem was no Main() is generated off of the attribute, I don't see it working anywhere else in CLR tests without the condition doubled in .ilproj.

[tomeksowi]

Still the problem was no Main() is generated off of the attribute

'ReferenceXUnitWrapperGenerator' was not set for IL, this change at least puts a project reference:

--- a/src/tests/Directory.Build.targets
+++ b/src/tests/Directory.Build.targets
@@ -493,7 +493,7 @@
     </ItemGroup>
   </Target>

-  <PropertyGroup Condition="'$(Language)' == 'C#' and '$(_CLRTestNeedsToRun)' == 'true' and ('$(BuildAsStandalone)' == 'true' or '$(RequiresProcessIsolation)' == 'true' or '$(InMergedTestDirectory)' == 'true' or '$(IsMergedTestRunnerAssembly)' == 'true')">
+  <PropertyGroup Condition="('$(Language)' == 'C#' or '$(Language)' == 'IL') and '$(_CLRTestNeedsToRun)' == 'true' and ('$(BuildAsStandalone)' == 'true' or '$(RequiresProcessIsolation)' == 'true' or '$(InMergedTestDirectory)' == 'true' or '$(IsMergedTestRunnerAssembly)' == 'true')">
     <ReferenceXUnitWrapperGenerator Condition="'$(ReferenceXUnitWrapperGenerator)' == ''">true</ReferenceXUnitWrapperGenerator>
   </PropertyGroup>

Still, the wrapper generator is not called.

[jkoritzinsky]

The wrapper generator won't be called for IL. You'll need to add a Main method into each IL project that manually calls the different cases.

[am11]

LGTM