Skip to content

Commit ac8d185

Browse files
narasamdyanarasamdya
committed
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
1 parent bc5c6c4 commit ac8d185

File tree

11 files changed

+67
-95
lines changed

11 files changed

+67
-95
lines changed

‎.azdo/linux/job-selfhost.yml‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
parameters:
22
- name: BxlCommonArgs
33
type: string
4-
default: --shared-comp /ado /cacheMiss:"[Bxl.Selfhost.Linux]" /logObservedFileAccesses /cacheConfigFilePath:Out/CacheConfig.json /logoutput:FullOutputOnError /logsToRetain:10 /exp:lazysodeletion- # /p:[Sdk.BuildXL]xunitSemaphoreCount=20
4+
# We pass xunit semaphore `/p:[Sdk.BuildXL]xunitSemaphoreCount=8` to limit the number of parallel xunit pips.
5+
# Too many xunit pips running in parallel can cause the long running ones to hang.
6+
default: --shared-comp /ado /cacheMiss:"[Bxl.Selfhost.Linux]" /logObservedFileAccesses /cacheConfigFilePath:Out/CacheConfig.json /logoutput:FullOutputOnError /logsToRetain:10 /exp:lazysodeletion- /p:[Sdk.BuildXL]xunitSemaphoreCount=8
57
- name: BxlExtraArgs
68
type: string
79
- name: ValidationName

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

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,6 @@ import {isDotNetCore} from "Sdk.Managed.Shared";
88

99
export const xunitConsolePackage = importFrom("xunit.runner.console").Contents.all;
1010

11-
// This package is published by Dotnet Arcade and contains some important fixes we need, when
12-
// running on .NETCoreApp 3.0, see: https://github.com/dotnet/arcade/tree/master/src/Microsoft.DotNet.XUnitConsoleRunner
13-
export const xunitNetCoreConsolePackage = importFrom("Microsoft.DotNet.XUnitConsoleRunner").Contents.all;
14-
1511
/**
1612
* Evaluate (i.e. schedule) xUnit test runner invocation with specified arguments.
1713
*/

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

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -63,15 +63,15 @@ function getTargetFramework(): Framework {
6363
}
6464
}
6565

66-
@@public
67-
export const additionalNetCoreRuntimeContent = isDotNetCore(qualifier.targetFramework) ?
68-
[
66+
const additionalNetCoreRuntimeContent = isDotNetCore(qualifier.targetFramework)
67+
? [
6968
// Unfortunately xUnit console runner comes as a precompiled assembly for .NET Core, we could either go and package it
7069
// into a self-contained deployment or treat it as a framework-dependent deployment as intended, let's do the latter
7170
...(getXunitConsoleRuntimeConfigNetCoreAppFiles()),
7271
xunitConsolePackage.getFile(r`/tools/netcoreapp2.0/xunit.runner.utility.netcoreapp10.dll`),
73-
xunitNetCoreConsolePackage.getFile(r`/lib/netcoreapp2.0/xunit.console.dll`)
74-
] : [];
72+
xunitConsolePackage.getFile(r`/tools/netcoreapp2.0/xunit.console.dll`)
73+
]
74+
: [];
7575

7676
// For the DotNetCore run we need to copy a bunch more files:
7777
function additionalRuntimeContent(args: Managed.TestArguments) : Deployment.DeployableItem[] {
@@ -137,7 +137,6 @@ function wrapInUntrackedCmd(executeArguments: Transformer.ExecuteArguments) : Tr
137137
]),
138138
Cmd.argument(Artifact.input(executeArguments.tool.exe))
139139
].prependWhenMerged(),
140-
141140
dependencies: staticDirectoryContents,
142141
tags: ["test", "telemetry:xUnitUntracked"]
143142
});

‎Public/Sdk/SelfHost/BuildXL/BuildXLSdk.dsc‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -672,11 +672,11 @@ function processArguments(args: Arguments, targetType: Csc.TargetType) : Argumen
672672

673673
let ruleset : File = undefined;
674674

675-
// Required for v2.x Roslyn compilers
676-
// 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.
677-
// Method calls are analyzed even when they are conditionally omitted. For instance, `Debug.Assert` in release mode.
678-
features = features.push("flow-analysis");
679-
ruleset = f`BuildXL.Recommend.Required.Error.ruleset`;
675+
// Required for v2.x Roslyn compilers
676+
// 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.
677+
// Method calls are analyzed even when they are conditionally omitted. For instance, `Debug.Assert` in release mode.
678+
features = features.push("flow-analysis");
679+
ruleset = f`BuildXL.Recommend.Required.Error.ruleset`;
680680

681681
args = Object.merge<Arguments>(
682682
{
@@ -972,6 +972,9 @@ function processTestArguments(args: Managed.TestArguments) : Managed.TestArgumen
972972
...addIf(isFullFramework,
973973
importFrom("System.Runtime.Serialization.Primitives").pkg
974974
),
975+
...addIf(isFullFramework,
976+
NetFx.System.Collections.Concurrent.dll,
977+
NetFx.System.ObjectModel.dll)
975978
],
976979
skipTestRun: !targetFrameworkMatchesCurrentHost,
977980
runTestArgs: {

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

Lines changed: 15 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import * as Qtest from "BuildXL.Tools.QTest";
99
export declare const qualifier : Managed.TargetFrameworks.All;
1010
const qTestContents = importFrom("CB.QTest").Contents.all;
1111

12-
const isDotNetCore = qualifier.targetFramework.startsWith("netcoreapp");
12+
// const isDotNetCore = Shared.isDotNetCore(qualifier.targetFramework); // qualifier.targetFramework.startsWith("netcoreapp");
1313

1414
@@public
1515
export const qTestTool: Transformer.ToolDefinition = Context.getCurrentHost().os === "win" && {
@@ -43,7 +43,7 @@ export interface TestRunArguments extends Managed.TestRunArguments, Qtest.QTestA
4343
export function getFramework(frameworkToWrap: Managed.TestFramework) : Managed.TestFramework {
4444
return {
4545
compileArguments: (args: Managed.Arguments) => {
46-
if (isDotNetCore) {
46+
if (Shared.isDotNetCore(qualifier.targetFramework)) {
4747
args = Object.merge<Managed.Arguments>(
4848
args,
4949
{
@@ -59,13 +59,13 @@ export function getFramework(frameworkToWrap: Managed.TestFramework) : Managed.T
5959
},
6060
additionalRuntimeContent: (args: Managed.Arguments) => [
6161
...(frameworkToWrap.additionalRuntimeContent ? frameworkToWrap.additionalRuntimeContent(args) : []),
62-
...(isDotNetCore ? [
62+
...(Shared.isDotNetCore(qualifier.targetFramework) ? [
6363
// hand picking files to avoid collisions with xunit assemblies specified elsewhere
6464
...importFrom("xunit.runner.visualstudio").Contents.all.getFiles([
65-
r`build/netcoreapp1.0/xunit.runner.reporters.netcoreapp10.dll`,
66-
r`build/netcoreapp1.0/xunit.runner.visualstudio.dotnetcore.testadapter.deps.json`,
67-
r`build/netcoreapp1.0/xunit.runner.visualstudio.dotnetcore.testadapter.dll`,
68-
r`build/netcoreapp1.0/xunit.runner.visualstudio.props`
65+
r`build/net6.0/xunit.runner.reporters.netcoreapp10.dll`,
66+
r`build/net6.0/xunit.runner.visualstudio.dotnetcore.testadapter.deps.json`,
67+
r`build/net6.0/xunit.runner.visualstudio.dotnetcore.testadapter.dll`,
68+
r`build/net6.0/xunit.runner.visualstudio.props`
6969
]),
7070
] : []),
7171
f`xunit.runner.json`
@@ -106,15 +106,20 @@ function getQTestDotNetFramework() : Qtest.QTestDotNetFramework {
106106
}
107107
}
108108

109-
function runTest(args : TestRunArguments) : File[] {
109+
function runTest(args : TestRunArguments) : File[] {
110+
111+
// Extracting a variable, because the type checker can't analyze a dotted names properly.
112+
const targetFramework = qualifier.targetFramework;
113+
110114
const testMethod = Environment.getStringValue("[UnitTest]Filter.testMethod");
111115
const testClass = Environment.getStringValue("[UnitTest]Filter.testClass");
112116

113117
let additionalOptions = undefined;
114118
let filterArgs = [];
115119
let rootTestAdapterPath = importFrom("xunit.runner.visualstudio").Contents.all;
116-
let testAdapterPath = d`${rootTestAdapterPath}/build/_common`;
117-
120+
let testAdapterFolder = Shared.isDotNetCore(targetFramework) ? "net6.0" : "net462";
121+
let testAdapterPath = d`${rootTestAdapterPath}/build/${testAdapterFolder}`;
122+
118123
// when testmethod or testclass ignore limitGroups and skipGroups arguments
119124
if (testMethod || testClass) {
120125
// vstest doesn't support a class filter for xunit runner.
@@ -163,9 +168,6 @@ function runTest(args : TestRunArguments) : File[] {
163168
let qTestRuntimeDependencies = undefined;
164169
let qTestEnvironmentVariables = undefined;
165170

166-
// Extracting a variable, because the type checker can't analyze a dotted names properly.
167-
const targetFramework = qualifier.targetFramework;
168-
169171
if (Shared.isDotNetCore(targetFramework)) {
170172
const dotNetTool = importFrom("Sdk.Managed.Frameworks").Helpers.getDotNetToolTemplate(targetFramework);
171173
qTestRuntimeDependencies = [

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -94,11 +94,11 @@ private static EvaluationResult NotEqualOrEqual(Context context, bool expectedEq
9494

9595
if (expectedEqual)
9696
{
97-
throw new EqualException(expectedString, actualString);
97+
throw EqualException.ForMismatchedValues(expectedString, actualString);
9898
}
9999
else
100100
{
101-
throw new NotEqualException(expectedString, actualString);
101+
throw NotEqualException.ForEqualValues(expectedString, actualString);
102102
}
103103
}
104104

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ namespace Test.Ninja {
1212
// These tests require Detours to run itself, so we won't detour the test runner process itself
1313
runWithUntrackedDependencies: true
1414
},
15+
parallelBucketCount: 12
1516
},
1617
assemblyName: "Test.BuildXL.FrontEnd.Ninja",
1718
sources: globR(d`.`, "*.cs"),

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

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,15 +26,19 @@ public void PipDataStoredCorrectly()
2626
// This test ensures the consistency.
2727
var pipData = PipData.Invalid;
2828
var envVar = new EnvironmentVariable(StringId.UnsafeCreateFrom(42), pipData, isPassThrough: true);
29-
Assert.Equal(pipData, envVar.Value);
29+
30+
// Don't use XUnit Equals method for pip data comparison because it will treat pip data as a collection.
31+
// Note that pip data implements IEnumerable, and XUnit will use collection comparison logic. This will cause
32+
// System.NullReferenceException because invalid pip data may have some null entries.
33+
XAssert.SimpleEqual(pipData, envVar.Value);
3034

3135
var pipDataEntry = new PipDataEntry(PipDataFragmentEscaping.NoEscaping, PipDataEntryType.NestedDataHeader, 42);
3236
pipData = PipData.CreateInternal(
3337
pipDataEntry,
3438
PipDataEntryList.FromEntries(new[] {pipDataEntry}),
3539
StringId.UnsafeCreateFrom(1));
3640
envVar = new EnvironmentVariable(StringId.UnsafeCreateFrom(42), pipData, isPassThrough: false);
37-
Assert.Equal(pipData, envVar.Value);
41+
XAssert.SimpleEqual(pipData, envVar.Value);
3842
}
3943

4044
[Fact]

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

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,21 @@ public static void AreEqual(object expected, object actual)
7979
Assert.Equal(expected, actual);
8080
}
8181

82+
/// <summary>
83+
/// Compares values without using XUnit Equals method.
84+
/// </summary>
85+
/// <remarks>
86+
/// When T implements IEnumerable, XUnit Equals method will try to do the equality checking by enumerating the entry first.
87+
/// </remarks>
88+
public static void SimpleEqual<T>(T expected, T actual)
89+
{
90+
bool areEqual = expected.Equals(actual);
91+
if (!areEqual)
92+
{
93+
throw global::Xunit.Sdk.EqualException.ForMismatchedValues(expected, actual);
94+
}
95+
}
96+
8297
/// <nodoc/>
8398
public static void AreEqual<T>(T expected, T actual, [StringSyntax(StringSyntaxAttribute.CompositeFormat)] string format, params object[] args)
8499
{
@@ -199,7 +214,7 @@ public static void AreArraysEqual<T>(T[] expected, T[] actual, bool expectedResu
199214
var userMessage = format != null
200215
? GetMessage(format, args) + Environment.NewLine + equalityMessage
201216
: equalityMessage;
202-
throw new global::Xunit.Sdk.AssertActualExpectedException(
217+
throw global::Xunit.Sdk.EqualException.ForMismatchedValues(
203218
ArrayToString(expected),
204219
ArrayToString(actual),
205220
userMessage);
@@ -225,7 +240,7 @@ public static void AreSetsEqual<T>(IEnumerable<T> expected, IEnumerable<T> actua
225240
var userMessage = format != null
226241
? GetMessage(format, args) + Environment.NewLine + equalityMessage
227242
: equalityMessage;
228-
throw new global::Xunit.Sdk.AssertActualExpectedException(
243+
throw global::Xunit.Sdk.EqualException.ForMismatchedValues(
229244
SetToString(expectedSet),
230245
SetToString(actualSet),
231246
userMessage);

‎cg/nuget/cgmanifest.json‎

Lines changed: 5 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -1180,15 +1180,6 @@
11801180
}
11811181
}
11821182
},
1183-
{
1184-
"Component": {
1185-
"Type": "NuGet",
1186-
"NuGet": {
1187-
"Name": "Microsoft.DotNet.XUnitConsoleRunner",
1188-
"Version": "2.5.1-beta.19270.4"
1189-
}
1190-
}
1191-
},
11921183
{
11931184
"Component": {
11941185
"Type": "NuGet",
@@ -4672,30 +4663,12 @@
46724663
}
46734664
}
46744665
},
4675-
{
4676-
"Component": {
4677-
"Type": "NuGet",
4678-
"NuGet": {
4679-
"Name": "xunit.analyzers",
4680-
"Version": "0.10.0"
4681-
}
4682-
}
4683-
},
46844666
{
46854667
"Component": {
46864668
"Type": "NuGet",
46874669
"NuGet": {
46884670
"Name": "xunit.assert",
4689-
"Version": "2.4.1-ms"
4690-
}
4691-
}
4692-
},
4693-
{
4694-
"Component": {
4695-
"Type": "NuGet",
4696-
"NuGet": {
4697-
"Name": "xunit.core",
4698-
"Version": "2.4.1-ms"
4671+
"Version": "2.5.3"
46994672
}
47004673
}
47014674
},
@@ -4704,7 +4677,7 @@
47044677
"Type": "NuGet",
47054678
"NuGet": {
47064679
"Name": "xunit.extensibility.core",
4707-
"Version": "2.4.1"
4680+
"Version": "2.5.3"
47084681
}
47094682
}
47104683
},
@@ -4713,7 +4686,7 @@
47134686
"Type": "NuGet",
47144687
"NuGet": {
47154688
"Name": "xunit.extensibility.execution",
4716-
"Version": "2.4.1"
4689+
"Version": "2.5.3"
47174690
}
47184691
}
47194692
},
@@ -4722,25 +4695,7 @@
47224695
"Type": "NuGet",
47234696
"NuGet": {
47244697
"Name": "xunit.runner.console",
4725-
"Version": "2.4.1"
4726-
}
4727-
}
4728-
},
4729-
{
4730-
"Component": {
4731-
"Type": "NuGet",
4732-
"NuGet": {
4733-
"Name": "xunit.runner.reporters",
4734-
"Version": "2.4.1-pre.build.4059"
4735-
}
4736-
}
4737-
},
4738-
{
4739-
"Component": {
4740-
"Type": "NuGet",
4741-
"NuGet": {
4742-
"Name": "xunit.runner.utility",
4743-
"Version": "2.4.1"
4698+
"Version": "2.5.3"
47444699
}
47454700
}
47464701
},
@@ -4749,7 +4704,7 @@
47494704
"Type": "NuGet",
47504705
"NuGet": {
47514706
"Name": "xunit.runner.visualstudio",
4752-
"Version": "2.4.1"
4707+
"Version": "2.5.3"
47534708
}
47544709
}
47554710
}

0 commit comments

Comments
 (0)