Merged PR 748328: Upgrade XUnit

The purposes of upgrading XUnit are twofolds:
1. To use the latest greatest XUnit packages.
2. As an attempt to relieve hanging XUnit pips.

For (2), we also do other changes:
- Split Ninja UTs to multiple "smaller" pips, where each pip should run faster than the big one.
- Limit the number of xunit pips that can run in parallel by using semaphore. We suspect that the hanging XUnit pips are caused by too many XUnit pips running in parallel.

Example of successful validation: https://dev.azure.com/mseng/Domino/_build/results?buildId=23035493&view=results

Related work items: #2111327

.azdo/linux/job-selfhost.yml

@@ -1,7 +1,9 @@
 parameters:
 - name: BxlCommonArgs
   type: string
-  default: --shared-comp /ado /cacheMiss:"[Bxl.Selfhost.Linux]" /logObservedFileAccesses /cacheConfigFilePath:Out/CacheConfig.json /logoutput:FullOutputOnError /logsToRetain:10 /exp:lazysodeletion- # /p:[Sdk.BuildXL]xunitSemaphoreCount=20
+  # We pass xunit semaphore `/p:[Sdk.BuildXL]xunitSemaphoreCount=8` to limit the number of parallel xunit pips.
+  # Too many xunit pips running in parallel can cause the long running ones to hang. 
+  default: --shared-comp /ado /cacheMiss:"[Bxl.Selfhost.Linux]" /logObservedFileAccesses /cacheConfigFilePath:Out/CacheConfig.json /logoutput:FullOutputOnError /logsToRetain:10 /exp:lazysodeletion- /p:[Sdk.BuildXL]xunitSemaphoreCount=8
 - name: BxlExtraArgs
   type: string
 - name: ValidationName

Public/Sdk/Public/Managed/Testing/XUnit/xunit.dsc

@@ -8,10 +8,6 @@ import {isDotNetCore} from "Sdk.Managed.Shared";
 
 export const xunitConsolePackage = importFrom("xunit.runner.console").Contents.all;
 
-// This package is published by Dotnet Arcade and contains some important fixes we need, when
-// running on .NETCoreApp 3.0, see: https://github.com/dotnet/arcade/tree/master/src/Microsoft.DotNet.XUnitConsoleRunner
-export const xunitNetCoreConsolePackage = importFrom("Microsoft.DotNet.XUnitConsoleRunner").Contents.all;
-
 /**
  * Evaluate (i.e. schedule) xUnit test runner invocation with specified arguments.
  */

Public/Sdk/Public/Managed/Testing/XUnit/xunitframework.dsc

@@ -63,15 +63,15 @@ function getTargetFramework(): Framework {
     } 
 }
 
-@@public
-export const additionalNetCoreRuntimeContent = isDotNetCore(qualifier.targetFramework) ?
-    [
+const additionalNetCoreRuntimeContent = isDotNetCore(qualifier.targetFramework)
+    ? [
         // Unfortunately xUnit console runner comes as a precompiled assembly for .NET Core, we could either go and package it
         // into a self-contained deployment or treat it as a framework-dependent deployment as intended, let's do the latter
         ...(getXunitConsoleRuntimeConfigNetCoreAppFiles()),
         xunitConsolePackage.getFile(r`/tools/netcoreapp2.0/xunit.runner.utility.netcoreapp10.dll`),
-        xunitNetCoreConsolePackage.getFile(r`/lib/netcoreapp2.0/xunit.console.dll`)
-    ] : [];
+        xunitConsolePackage.getFile(r`/tools/netcoreapp2.0/xunit.console.dll`)
+      ] 
+    : [];
     
 // For the DotNetCore run we need to copy a bunch more files:
 function additionalRuntimeContent(args: Managed.TestArguments) : Deployment.DeployableItem[] {
@@ -137,7 +137,6 @@ function wrapInUntrackedCmd(executeArguments: Transformer.ExecuteArguments) : Tr
                 ]),
                 Cmd.argument(Artifact.input(executeArguments.tool.exe))
             ].prependWhenMerged(),
-
             dependencies: staticDirectoryContents,
             tags: ["test", "telemetry:xUnitUntracked"]
         });

Public/Sdk/SelfHost/BuildXL/BuildXLSdk.dsc

@@ -672,11 +672,11 @@ function processArguments(args: Arguments, targetType: Csc.TargetType) : Argumen
 
     let ruleset : File = undefined;
 
-	// Required for v2.x Roslyn compilers
-	// Flow analysis is used to infer the nullability of variables within executable code. The inferred nullability of a variable is independent of the variable's declared nullability.
-	// Method calls are analyzed even when they are conditionally omitted. For instance, `Debug.Assert` in release mode.
-	features = features.push("flow-analysis");
-	ruleset = f`BuildXL.Recommend.Required.Error.ruleset`;
+    // Required for v2.x Roslyn compilers
+    // Flow analysis is used to infer the nullability of variables within executable code. The inferred nullability of a variable is independent of the variable's declared nullability.
+    // Method calls are analyzed even when they are conditionally omitted. For instance, `Debug.Assert` in release mode.
+    features = features.push("flow-analysis");
+    ruleset = f`BuildXL.Recommend.Required.Error.ruleset`;
 
     args = Object.merge<Arguments>(
         {
@@ -972,6 +972,9 @@ function processTestArguments(args: Managed.TestArguments) : Managed.TestArgumen
             ...addIf(isFullFramework,
                 importFrom("System.Runtime.Serialization.Primitives").pkg
             ),
+            ...addIf(isFullFramework,
+                NetFx.System.Collections.Concurrent.dll,
+                NetFx.System.ObjectModel.dll)
         ],
         skipTestRun: !targetFrameworkMatchesCurrentHost,
         runTestArgs: {

Public/Sdk/SelfHost/BuildXL/Testing/QTest/qtestFramework.dsc

@@ -9,7 +9,7 @@ import * as Qtest        from "BuildXL.Tools.QTest";
 export declare const qualifier : Managed.TargetFrameworks.All;
 const qTestContents = importFrom("CB.QTest").Contents.all;
 
-const isDotNetCore = qualifier.targetFramework.startsWith("netcoreapp");
+// const isDotNetCore = Shared.isDotNetCore(qualifier.targetFramework); // qualifier.targetFramework.startsWith("netcoreapp");
 
 @@public
 export const qTestTool: Transformer.ToolDefinition = Context.getCurrentHost().os === "win" && {
@@ -43,7 +43,7 @@ export interface TestRunArguments extends Managed.TestRunArguments, Qtest.QTestA
 export function getFramework(frameworkToWrap: Managed.TestFramework) : Managed.TestFramework {
     return {
         compileArguments: (args: Managed.Arguments) => {
-            if (isDotNetCore) {
+            if (Shared.isDotNetCore(qualifier.targetFramework)) {
                 args = Object.merge<Managed.Arguments>(
                     args,
                     {
@@ -59,13 +59,13 @@ export function getFramework(frameworkToWrap: Managed.TestFramework) : Managed.T
         },
         additionalRuntimeContent: (args: Managed.Arguments) => [ 
             ...(frameworkToWrap.additionalRuntimeContent ? frameworkToWrap.additionalRuntimeContent(args) : []), 
-            ...(isDotNetCore ? [
+            ...(Shared.isDotNetCore(qualifier.targetFramework) ? [
                 // hand picking files to avoid collisions with xunit assemblies specified elsewhere
                 ...importFrom("xunit.runner.visualstudio").Contents.all.getFiles([
-                    r`build/netcoreapp1.0/xunit.runner.reporters.netcoreapp10.dll`,
-                    r`build/netcoreapp1.0/xunit.runner.visualstudio.dotnetcore.testadapter.deps.json`,
-                    r`build/netcoreapp1.0/xunit.runner.visualstudio.dotnetcore.testadapter.dll`,
-                    r`build/netcoreapp1.0/xunit.runner.visualstudio.props`
+                    r`build/net6.0/xunit.runner.reporters.netcoreapp10.dll`,
+                    r`build/net6.0/xunit.runner.visualstudio.dotnetcore.testadapter.deps.json`,
+                    r`build/net6.0/xunit.runner.visualstudio.dotnetcore.testadapter.dll`,
+                    r`build/net6.0/xunit.runner.visualstudio.props`
                 ]),
                 ] : []),
             f`xunit.runner.json`
@@ -106,15 +106,20 @@ function getQTestDotNetFramework() : Qtest.QTestDotNetFramework {
     }
 }
 
-function runTest(args : TestRunArguments) : File[] {  
+function runTest(args : TestRunArguments) : File[] {
+
+    // Extracting a variable, because the type checker can't analyze a dotted names properly.
+    const targetFramework = qualifier.targetFramework;
+
     const testMethod = Environment.getStringValue("[UnitTest]Filter.testMethod");
     const testClass  = Environment.getStringValue("[UnitTest]Filter.testClass");
      
     let additionalOptions = undefined;
     let filterArgs = [];
     let rootTestAdapterPath = importFrom("xunit.runner.visualstudio").Contents.all;
-    let testAdapterPath =  d`${rootTestAdapterPath}/build/_common`;
-    
+    let testAdapterFolder = Shared.isDotNetCore(targetFramework) ? "net6.0" : "net462";
+    let testAdapterPath =  d`${rootTestAdapterPath}/build/${testAdapterFolder}`;
+
     // when testmethod or testclass ignore limitGroups and skipGroups arguments
     if (testMethod || testClass) {
         // vstest doesn't support a class filter for xunit runner. 
@@ -163,9 +168,6 @@ function runTest(args : TestRunArguments) : File[] {
     let qTestRuntimeDependencies = undefined;
     let qTestEnvironmentVariables = undefined;
 
-    // Extracting a variable, because the type checker can't analyze a dotted names properly.
-    const targetFramework = qualifier.targetFramework;
-
     if (Shared.isDotNetCore(targetFramework)) {
         const dotNetTool = importFrom("Sdk.Managed.Frameworks").Helpers.getDotNetToolTemplate(targetFramework);
         qTestRuntimeDependencies = [

Public/Src/FrontEnd/SdkTesting/Helper/Ambients/AmbientAssert.cs

@@ -94,11 +94,11 @@ namespace BuildXL.FrontEnd.Script.Testing.Helper.Ambients
 
                 if (expectedEqual)
                 {
-                    throw new EqualException(expectedString, actualString);
+                    throw EqualException.ForMismatchedValues(expectedString, actualString);
                 }
                 else
                 {
-                    throw new NotEqualException(expectedString, actualString);
+                    throw NotEqualException.ForEqualValues(expectedString, actualString);
                 }
             }
 

Public/Src/FrontEnd/UnitTests/Ninja/Test.BuildXL.FrontEnd.Ninja.dsc

@@ -12,6 +12,7 @@ namespace Test.Ninja {
                 // These tests require Detours to run itself, so we won't detour the test runner process itself
                 runWithUntrackedDependencies: true
             },
+            parallelBucketCount: 12
         },
         assemblyName: "Test.BuildXL.FrontEnd.Ninja",
         sources: globR(d`.`, "*.cs"),

Public/Src/Pips/UnitTests/Pips/EnvironmentVariableLayoutTests.cs

@@ -26,7 +26,11 @@ namespace Test.BuildXL.Pips
             // This test ensures the consistency.
             var pipData = PipData.Invalid;
             var envVar = new EnvironmentVariable(StringId.UnsafeCreateFrom(42), pipData, isPassThrough: true);
-            Assert.Equal(pipData, envVar.Value);
+
+            // Don't use XUnit Equals method for pip data comparison because it will treat pip data as a collection.
+            // Note that pip data implements IEnumerable, and XUnit will use collection comparison logic. This will cause
+            // System.NullReferenceException because invalid pip data may have some null entries.
+            XAssert.SimpleEqual(pipData, envVar.Value);
 
             var pipDataEntry = new PipDataEntry(PipDataFragmentEscaping.NoEscaping, PipDataEntryType.NestedDataHeader, 42);
             pipData = PipData.CreateInternal(
@@ -34,7 +38,7 @@ namespace Test.BuildXL.Pips
                 PipDataEntryList.FromEntries(new[] {pipDataEntry}),
                 StringId.UnsafeCreateFrom(1));
             envVar = new EnvironmentVariable(StringId.UnsafeCreateFrom(42), pipData, isPassThrough: false);
-            Assert.Equal(pipData, envVar.Value);
+            XAssert.SimpleEqual(pipData, envVar.Value);
         }
 
         [Fact]

Public/Src/Utilities/UnitTests/TestUtilities.XUnit/XAssert.cs

@@ -79,6 +79,21 @@ namespace Test.BuildXL.TestUtilities.Xunit
             Assert.Equal(expected, actual);
         }
 
+        /// <summary>
+        /// Compares values without using XUnit Equals method.
+        /// </summary>
+        /// <remarks>
+        /// When T implements IEnumerable, XUnit Equals method will try to do the equality checking by enumerating the entry first.
+        /// </remarks>
+        public static void SimpleEqual<T>(T expected, T actual)
+        {
+            bool areEqual = expected.Equals(actual);
+            if (!areEqual)
+            {
+                throw global::Xunit.Sdk.EqualException.ForMismatchedValues(expected, actual);
+            }
+        }
+
         /// <nodoc/>
         public static void AreEqual<T>(T expected, T actual, [StringSyntax(StringSyntaxAttribute.CompositeFormat)] string format, params object[] args)
         {
@@ -199,7 +214,7 @@ namespace Test.BuildXL.TestUtilities.Xunit
                 var userMessage = format != null
                     ? GetMessage(format, args) + Environment.NewLine + equalityMessage
                     : equalityMessage;
-                throw new global::Xunit.Sdk.AssertActualExpectedException(
+                throw global::Xunit.Sdk.EqualException.ForMismatchedValues(
                     ArrayToString(expected),
                     ArrayToString(actual),
                     userMessage);
@@ -225,7 +240,7 @@ namespace Test.BuildXL.TestUtilities.Xunit
                 var userMessage = format != null
                     ? GetMessage(format, args) + Environment.NewLine + equalityMessage
                     : equalityMessage;
-                throw new global::Xunit.Sdk.AssertActualExpectedException(
+                throw global::Xunit.Sdk.EqualException.ForMismatchedValues(
                     SetToString(expectedSet),
                     SetToString(actualSet),
                     userMessage);

cg/nuget/cgmanifest.json

@@ -1180,15 +1180,6 @@
         }
       }
     },
-    {
-      "Component": {
-        "Type": "NuGet",
-        "NuGet": {
-          "Name": "Microsoft.DotNet.XUnitConsoleRunner",
-          "Version": "2.5.1-beta.19270.4"
-        }
-      }
-    },
     {
       "Component": {
         "Type": "NuGet",
@@ -4672,30 +4663,12 @@
         }
       }
     },
-    {
-      "Component": {
-        "Type": "NuGet",
-        "NuGet": {
-          "Name": "xunit.analyzers",
-          "Version": "0.10.0"
-        }
-      }
-    },
     {
       "Component": {
         "Type": "NuGet",
         "NuGet": {
           "Name": "xunit.assert",
-          "Version": "2.4.1-ms"
-        }
-      }
-    },
-    {
-      "Component": {
-        "Type": "NuGet",
-        "NuGet": {
-          "Name": "xunit.core",
-          "Version": "2.4.1-ms"
+          "Version": "2.5.3"
         }
       }
     },
@@ -4704,7 +4677,7 @@
         "Type": "NuGet",
         "NuGet": {
           "Name": "xunit.extensibility.core",
-          "Version": "2.4.1"
+          "Version": "2.5.3"
         }
       }
     },
@@ -4713,7 +4686,7 @@
         "Type": "NuGet",
         "NuGet": {
           "Name": "xunit.extensibility.execution",
-          "Version": "2.4.1"
+          "Version": "2.5.3"
         }
       }
     },
@@ -4722,25 +4695,7 @@
         "Type": "NuGet",
         "NuGet": {
           "Name": "xunit.runner.console",
-          "Version": "2.4.1"
-        }
-      }
-    },
-    {
-      "Component": {
-        "Type": "NuGet",
-        "NuGet": {
-          "Name": "xunit.runner.reporters",
-          "Version": "2.4.1-pre.build.4059"
-        }
-      }
-    },
-    {
-      "Component": {
-        "Type": "NuGet",
-        "NuGet": {
-          "Name": "xunit.runner.utility",
-          "Version": "2.4.1"
+          "Version": "2.5.3"
         }
       }
     },
@@ -4749,7 +4704,7 @@
         "Type": "NuGet",
         "NuGet": {
           "Name": "xunit.runner.visualstudio",
-          "Version": "2.4.1"
+          "Version": "2.5.3"
         }
       }
     }

config.dsc

@@ -229,16 +229,11 @@ config({
 
                 // xUnit
                 { id: "xunit.abstractions", version: "2.0.3" },
-                { id: "xunit.analyzers", version: "0.10.0" },
-                { id: "xunit.assert", version: "2.4.1-ms" },
-                { id: "xunit.core", version: "2.4.1-ms" },
-                { id: "xunit.extensibility.core", version: "2.4.1" },
-                { id: "xunit.extensibility.execution", version: "2.4.1" },
-                { id: "xunit.runner.console", version: "2.4.1" },
-                { id: "Microsoft.DotNet.XUnitConsoleRunner", version: "2.5.1-beta.19270.4" },
-                { id: "xunit.runner.reporters", version: "2.4.1-pre.build.4059" },
-                { id: "xunit.runner.utility", version: "2.4.1" },
-                { id: "xunit.runner.visualstudio", version: "2.4.1", dependentPackageIdsToSkip: ["Microsoft.NET.Test.Sdk"]  },
+                { id: "xunit.assert", version: "2.5.3" },
+                { id: "xunit.extensibility.core", version: "2.5.3" },
+                { id: "xunit.extensibility.execution", version: "2.5.3" },
+                { id: "xunit.runner.console", version: "2.5.3" },
+                { id: "xunit.runner.visualstudio", version: "2.5.3" },
 
                 // microsoft test platform
                 { id: "Microsoft.TestPlatform.TestHost", version: "16.4.0"},