From ac8d185735df091009e5ffe08f911ffba81b72f4 Mon Sep 17 00:00:00 2001 From: Iman Narasamdya Date: Fri, 27 Oct 2023 20:41:18 +0000 Subject: [PATCH] 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 | 4 +- .../Public/Managed/Testing/XUnit/xunit.dsc | 4 -- .../Managed/Testing/XUnit/xunitframework.dsc | 11 ++-- Public/Sdk/SelfHost/BuildXL/BuildXLSdk.dsc | 13 +++-- .../BuildXL/Testing/QTest/qtestFramework.dsc | 28 +++++----- .../Helper/Ambients/AmbientAssert.cs | 4 +- .../Ninja/Test.BuildXL.FrontEnd.Ninja.dsc | 1 + .../Pips/EnvironmentVariableLayoutTests.cs | 8 ++- .../UnitTests/TestUtilities.XUnit/XAssert.cs | 19 ++++++- cg/nuget/cgmanifest.json | 55 ++----------------- config.dsc | 15 ++--- 11 files changed, 67 insertions(+), 95 deletions(-) diff --git a/.azdo/linux/job-selfhost.yml b/.azdo/linux/job-selfhost.yml index f037e95b3..067d69301 100644 --- a/.azdo/linux/job-selfhost.yml +++ b/.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 diff --git a/Public/Sdk/Public/Managed/Testing/XUnit/xunit.dsc b/Public/Sdk/Public/Managed/Testing/XUnit/xunit.dsc index dbd3b78b9..b851daf47 100644 --- a/Public/Sdk/Public/Managed/Testing/XUnit/xunit.dsc +++ b/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. */ diff --git a/Public/Sdk/Public/Managed/Testing/XUnit/xunitframework.dsc b/Public/Sdk/Public/Managed/Testing/XUnit/xunitframework.dsc index ac45803e4..b1f647b1d 100644 --- a/Public/Sdk/Public/Managed/Testing/XUnit/xunitframework.dsc +++ b/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"] }); diff --git a/Public/Sdk/SelfHost/BuildXL/BuildXLSdk.dsc b/Public/Sdk/SelfHost/BuildXL/BuildXLSdk.dsc index d5122ad20..f51ba8201 100644 --- a/Public/Sdk/SelfHost/BuildXL/BuildXLSdk.dsc +++ b/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( { @@ -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: { diff --git a/Public/Sdk/SelfHost/BuildXL/Testing/QTest/qtestFramework.dsc b/Public/Sdk/SelfHost/BuildXL/Testing/QTest/qtestFramework.dsc index dab844121..2105ff79a 100644 --- a/Public/Sdk/SelfHost/BuildXL/Testing/QTest/qtestFramework.dsc +++ b/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( 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 = [ diff --git a/Public/Src/FrontEnd/SdkTesting/Helper/Ambients/AmbientAssert.cs b/Public/Src/FrontEnd/SdkTesting/Helper/Ambients/AmbientAssert.cs index d71e573c0..a3f66f6e7 100644 --- a/Public/Src/FrontEnd/SdkTesting/Helper/Ambients/AmbientAssert.cs +++ b/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); } } diff --git a/Public/Src/FrontEnd/UnitTests/Ninja/Test.BuildXL.FrontEnd.Ninja.dsc b/Public/Src/FrontEnd/UnitTests/Ninja/Test.BuildXL.FrontEnd.Ninja.dsc index 2f05d4213..82eb22b2e 100644 --- a/Public/Src/FrontEnd/UnitTests/Ninja/Test.BuildXL.FrontEnd.Ninja.dsc +++ b/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"), diff --git a/Public/Src/Pips/UnitTests/Pips/EnvironmentVariableLayoutTests.cs b/Public/Src/Pips/UnitTests/Pips/EnvironmentVariableLayoutTests.cs index 495241d38..9c383ce1f 100644 --- a/Public/Src/Pips/UnitTests/Pips/EnvironmentVariableLayoutTests.cs +++ b/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] diff --git a/Public/Src/Utilities/UnitTests/TestUtilities.XUnit/XAssert.cs b/Public/Src/Utilities/UnitTests/TestUtilities.XUnit/XAssert.cs index 041c027cd..4d5e267e4 100644 --- a/Public/Src/Utilities/UnitTests/TestUtilities.XUnit/XAssert.cs +++ b/Public/Src/Utilities/UnitTests/TestUtilities.XUnit/XAssert.cs @@ -79,6 +79,21 @@ namespace Test.BuildXL.TestUtilities.Xunit Assert.Equal(expected, actual); } + /// + /// Compares values without using XUnit Equals method. + /// + /// + /// When T implements IEnumerable, XUnit Equals method will try to do the equality checking by enumerating the entry first. + /// + public static void SimpleEqual(T expected, T actual) + { + bool areEqual = expected.Equals(actual); + if (!areEqual) + { + throw global::Xunit.Sdk.EqualException.ForMismatchedValues(expected, actual); + } + } + /// public static void AreEqual(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); diff --git a/cg/nuget/cgmanifest.json b/cg/nuget/cgmanifest.json index 2596c76c7..f38add768 100644 --- a/cg/nuget/cgmanifest.json +++ b/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" } } } diff --git a/config.dsc b/config.dsc index ecdcbf94d..e1e227578 100644 --- a/config.dsc +++ b/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"},