Skip to content

Commit 2d51781

Browse files
Youssef1313Youssef1313
authored
Fix dotnet test perf for MTP (#50769)
1 parent 5fc3251 commit 2d51781

File tree

10 files changed

+140
-103
lines changed

10 files changed

+140
-103
lines changed

‎src/Cli/dotnet/Commands/Run/CommonRunHelpers.cs‎

Lines changed: 4 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -14,26 +14,11 @@ internal static class CommonRunHelpers
1414
/// specifically Compile, None, and EmbeddedResource items are not globbed by default.
1515
/// See <see cref="Commands.Restore.RestoringCommand.RestoreOptimizationProperties"/> for more details.
1616
/// </summary>
17-
public static Dictionary<string, string> GetGlobalPropertiesFromArgs(string[] args)
17+
public static Dictionary<string, string> GetGlobalPropertiesFromArgs(MSBuildArgs msbuildArgs)
1818
{
19-
var globalProperties = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase)
20-
{
21-
// This property disables default item globbing to improve performance
22-
// This should be safe because we are not evaluating items, only properties
23-
{ Constants.EnableDefaultItems, "false" },
24-
{ Constants.MSBuildExtensionsPath, AppContext.BaseDirectory }
25-
};
26-
27-
var msbuildArgs = MSBuildArgs.AnalyzeMSBuildArguments(args, CommonOptions.PropertiesOption, CommonOptions.RestorePropertiesOption, CommonOptions.MSBuildTargetOption(), CommonOptions.VerbosityOption());
28-
if (msbuildArgs.GlobalProperties is null)
29-
{
30-
return globalProperties;
31-
}
32-
foreach (var kv in msbuildArgs.GlobalProperties)
33-
{
34-
// If the property is already set, we don't override it
35-
globalProperties[kv.Key] = kv.Value;
36-
}
19+
var globalProperties = msbuildArgs.GlobalProperties?.ToDictionary() ?? new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
20+
globalProperties[Constants.EnableDefaultItems] = "false"; // Disable default item globbing to improve performance
21+
globalProperties[Constants.MSBuildExtensionsPath] = AppContext.BaseDirectory;
3722
return globalProperties;
3823
}
3924

‎src/Cli/dotnet/Commands/Run/RunCommand.cs‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -409,9 +409,7 @@ static ProjectInstance EvaluateProject(string? projectFilePath, Func<ProjectColl
409409
{
410410
Debug.Assert(projectFilePath is not null || projectFactory is not null);
411411

412-
var globalProperties = msbuildArgs.GlobalProperties?.ToDictionary() ?? new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
413-
globalProperties[Constants.EnableDefaultItems] = "false"; // Disable default item globbing to improve performance
414-
globalProperties[Constants.MSBuildExtensionsPath] = AppContext.BaseDirectory;
412+
var globalProperties = CommonRunHelpers.GetGlobalPropertiesFromArgs(msbuildArgs);
415413

416414
var collection = new ProjectCollection(globalProperties: globalProperties, loggers: binaryLogger is null ? null : [binaryLogger], toolsetDefinitionLocations: ToolsetDefinitionLocations.Default);
417415

‎src/Cli/dotnet/Commands/Test/MTP/MSBuildHandler.cs‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,9 @@
77

88
namespace Microsoft.DotNet.Cli.Commands.Test;
99

10-
internal sealed class MSBuildHandler(BuildOptions buildOptions, TestApplicationActionQueue actionQueue, TerminalTestReporter output)
10+
internal sealed class MSBuildHandler(BuildOptions buildOptions, TerminalTestReporter output)
1111
{
1212
private readonly BuildOptions _buildOptions = buildOptions;
13-
private readonly TestApplicationActionQueue _actionQueue = actionQueue;
1413
private readonly TerminalTestReporter _output = output;
1514

1615
private readonly ConcurrentBag<ParallelizableTestModuleGroupWithSequentialInnerModules> _testApplications = [];
@@ -128,7 +127,7 @@ private void InitializeTestApplications(IEnumerable<ParallelizableTestModuleGrou
128127
}
129128
}
130129

131-
public bool EnqueueTestApplications()
130+
public bool EnqueueTestApplications(TestApplicationActionQueue queue)
132131
{
133132
if (!_areTestingPlatformApplications)
134133
{
@@ -137,7 +136,7 @@ public bool EnqueueTestApplications()
137136

138137
foreach (var testApp in _testApplications)
139138
{
140-
_actionQueue.Enqueue(testApp);
139+
queue.Enqueue(testApp);
141140
}
142141
return true;
143142
}

‎src/Cli/dotnet/Commands/Test/MTP/MSBuildUtility.cs‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
using System.Collections.Concurrent;
55
using System.CommandLine;
66
using Microsoft.Build.Evaluation;
7+
using Microsoft.Build.Evaluation.Context;
78
using Microsoft.Build.Execution;
89
using Microsoft.DotNet.Cli.Commands.Restore;
910
using Microsoft.DotNet.Cli.Commands.Run;
@@ -33,10 +34,14 @@ public static (IEnumerable<ParallelizableTestModuleGroupWithSequentialInnerModul
3334
SolutionAndProjectUtility.GetRootDirectory(solutionFilePath);
3435

3536
FacadeLogger? logger = LoggerUtility.DetermineBinlogger([.. buildOptions.MSBuildArgs], dotnetTestVerb);
36-
var collection = new ProjectCollection(globalProperties: CommonRunHelpers.GetGlobalPropertiesFromArgs([.. buildOptions.MSBuildArgs]), loggers: logger is null ? null : [logger], toolsetDefinitionLocations: ToolsetDefinitionLocations.Default);
3737

38-
ConcurrentBag<ParallelizableTestModuleGroupWithSequentialInnerModules> projects = GetProjectsProperties(collection, solutionModel.SolutionProjects.Select(p => Path.Combine(rootDirectory, p.FilePath)), buildOptions);
38+
var msbuildArgs = MSBuildArgs.AnalyzeMSBuildArguments(buildOptions.MSBuildArgs, CommonOptions.PropertiesOption, CommonOptions.RestorePropertiesOption, CommonOptions.MSBuildTargetOption(), CommonOptions.VerbosityOption());
39+
40+
using var collection = new ProjectCollection(globalProperties: CommonRunHelpers.GetGlobalPropertiesFromArgs(msbuildArgs), loggers: logger is null ? null : [logger], toolsetDefinitionLocations: ToolsetDefinitionLocations.Default);
41+
var evaluationContext = EvaluationContext.Create(EvaluationContext.SharingPolicy.Shared);
42+
ConcurrentBag<ParallelizableTestModuleGroupWithSequentialInnerModules> projects = GetProjectsProperties(collection, evaluationContext, solutionModel.SolutionProjects.Select(p => Path.Combine(rootDirectory, p.FilePath)), buildOptions);
3943
logger?.ReallyShutdown();
44+
collection.UnloadAllProjects();
4045

4146
return (projects, isBuiltOrRestored);
4247
}
@@ -51,11 +56,14 @@ public static (IEnumerable<ParallelizableTestModuleGroupWithSequentialInnerModul
5156
}
5257

5358
FacadeLogger? logger = LoggerUtility.DetermineBinlogger([.. buildOptions.MSBuildArgs], dotnetTestVerb);
54-
var collection = new ProjectCollection(globalProperties: CommonRunHelpers.GetGlobalPropertiesFromArgs([.. buildOptions.MSBuildArgs]), logger is null ? null : [logger], toolsetDefinitionLocations: ToolsetDefinitionLocations.Default);
5559

56-
IEnumerable<ParallelizableTestModuleGroupWithSequentialInnerModules> projects = SolutionAndProjectUtility.GetProjectProperties(projectFilePath, collection, buildOptions);
57-
logger?.ReallyShutdown();
60+
var msbuildArgs = MSBuildArgs.AnalyzeMSBuildArguments(buildOptions.MSBuildArgs, CommonOptions.PropertiesOption, CommonOptions.RestorePropertiesOption, CommonOptions.MSBuildTargetOption(), CommonOptions.VerbosityOption());
5861

62+
using var collection = new ProjectCollection(globalProperties: CommonRunHelpers.GetGlobalPropertiesFromArgs(msbuildArgs), logger is null ? null : [logger], toolsetDefinitionLocations: ToolsetDefinitionLocations.Default);
63+
var evaluationContext = EvaluationContext.Create(EvaluationContext.SharingPolicy.Shared);
64+
IEnumerable<ParallelizableTestModuleGroupWithSequentialInnerModules> projects = SolutionAndProjectUtility.GetProjectProperties(projectFilePath, collection, evaluationContext, buildOptions);
65+
logger?.ReallyShutdown();
66+
collection.UnloadAllProjects();
5967
return (projects, isBuiltOrRestored);
6068
}
6169

@@ -123,7 +131,7 @@ private static bool BuildOrRestoreProjectOrSolution(string filePath, BuildOption
123131
return result == (int)BuildResultCode.Success;
124132
}
125133

126-
private static ConcurrentBag<ParallelizableTestModuleGroupWithSequentialInnerModules> GetProjectsProperties(ProjectCollection projectCollection, IEnumerable<string> projects, BuildOptions buildOptions)
134+
private static ConcurrentBag<ParallelizableTestModuleGroupWithSequentialInnerModules> GetProjectsProperties(ProjectCollection projectCollection, EvaluationContext evaluationContext, IEnumerable<string> projects, BuildOptions buildOptions)
127135
{
128136
var allProjects = new ConcurrentBag<ParallelizableTestModuleGroupWithSequentialInnerModules>();
129137

@@ -132,7 +140,7 @@ private static ConcurrentBag<ParallelizableTestModuleGroupWithSequentialInnerMod
132140
new ParallelOptions { MaxDegreeOfParallelism = buildOptions.DegreeOfParallelism },
133141
(project) =>
134142
{
135-
IEnumerable<ParallelizableTestModuleGroupWithSequentialInnerModules> projectsMetadata = SolutionAndProjectUtility.GetProjectProperties(project, projectCollection, buildOptions);
143+
IEnumerable<ParallelizableTestModuleGroupWithSequentialInnerModules> projectsMetadata = SolutionAndProjectUtility.GetProjectProperties(project, projectCollection, evaluationContext, buildOptions);
136144
foreach (var projectMetadata in projectsMetadata)
137145
{
138146
allProjects.Add(projectMetadata);

‎src/Cli/dotnet/Commands/Test/MTP/MicrosoftTestingPlatformTestCommand.cs‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -49,11 +49,11 @@ private int RunInternal(ParseResult parseResult, bool isHelp)
4949

5050
BuildOptions buildOptions = MSBuildUtility.GetBuildOptions(parseResult, degreeOfParallelism);
5151

52-
var actionQueue = new TestApplicationActionQueue(degreeOfParallelism, buildOptions, testOptions, _output, OnHelpRequested);
53-
5452
bool filterModeEnabled = parseResult.HasOption(MicrosoftTestingPlatformOptions.TestModulesFilterOption);
53+
TestApplicationActionQueue actionQueue;
5554
if (filterModeEnabled)
5655
{
56+
actionQueue = new TestApplicationActionQueue(degreeOfParallelism, buildOptions, testOptions, _output, OnHelpRequested);
5757
var testModulesFilterHandler = new TestModulesFilterHandler(actionQueue, _output);
5858
if (!testModulesFilterHandler.RunWithTestModulesFilter(parseResult))
5959
{
@@ -62,13 +62,18 @@ private int RunInternal(ParseResult parseResult, bool isHelp)
6262
}
6363
else
6464
{
65-
var msBuildHandler = new MSBuildHandler(buildOptions, actionQueue, _output);
65+
var msBuildHandler = new MSBuildHandler(buildOptions, _output);
6666
if (!msBuildHandler.RunMSBuild())
6767
{
6868
return ExitCode.GenericFailure;
6969
}
7070

71-
if (!msBuildHandler.EnqueueTestApplications())
71+
// NOTE: Don't create TestApplicationActionQueue before RunMSBuild.
72+
// The constructor will do Task.Run calls matching the degree of parallelism, and if we did that before the build, that can
73+
// be slowing us down unnecessarily.
74+
// Alternatively, if we can enqueue right after every project evaluation without waiting all evaluations to be done, we can enqueue early.
75+
actionQueue = new TestApplicationActionQueue(degreeOfParallelism, buildOptions, testOptions, _output, OnHelpRequested);
76+
if (!msBuildHandler.EnqueueTestApplications(actionQueue))
7277
{
7378
return ExitCode.GenericFailure;
7479
}

‎src/Cli/dotnet/Commands/Test/MTP/SolutionAndProjectUtility.cs‎

Lines changed: 37 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,10 @@
22
// The .NET Foundation licenses this file to you under the MIT license.
33

44
using System.Diagnostics;
5+
using Microsoft.Build.Definition;
56
using Microsoft.Build.Evaluation;
7+
using Microsoft.Build.Evaluation.Context;
68
using Microsoft.Build.Execution;
7-
using Microsoft.Build.Framework;
89
using Microsoft.DotNet.Cli.Commands.Run;
910
using Microsoft.DotNet.Cli.Commands.Run.LaunchSettings;
1011
using Microsoft.DotNet.Cli.Utils;
@@ -14,7 +15,7 @@ namespace Microsoft.DotNet.Cli.Commands.Test;
1415

1516
internal static class SolutionAndProjectUtility
1617
{
17-
private static readonly string s_computeRunArgumentsTarget = "ComputeRunArguments";
18+
private static readonly string[] s_computeRunArgumentsTarget = [Constants.ComputeRunArguments];
1819
private static readonly Lock s_buildLock = new();
1920

2021
public static (bool SolutionOrProjectFileFound, string Message) TryGetProjectOrSolutionFilePath(string directory, out string projectOrSolutionFilePath, out bool isSolution)
@@ -176,18 +177,36 @@ private static string[] GetSolutionFilterFilePaths(string directory)
176177

177178
private static string[] GetProjectFilePaths(string directory) => Directory.GetFiles(directory, CliConstants.ProjectExtensionPattern, SearchOption.TopDirectoryOnly);
178179

179-
private static ProjectInstance EvaluateProject(ProjectCollection collection, string projectFilePath, string? tfm)
180+
private static ProjectInstance EvaluateProject(ProjectCollection collection, EvaluationContext evaluationContext, string projectFilePath, string? tfm)
180181
{
181182
Debug.Assert(projectFilePath is not null);
182183

183-
var project = collection.LoadProject(projectFilePath);
184+
Dictionary<string, string>? globalProperties = null;
184185
if (tfm is not null)
185186
{
186-
project.SetGlobalProperty(ProjectProperties.TargetFramework, tfm);
187-
project.ReevaluateIfNecessary();
187+
globalProperties = new Dictionary<string, string>(capacity: 1)
188+
{
189+
{ ProjectProperties.TargetFramework, tfm }
190+
};
188191
}
189192

190-
return project.CreateProjectInstance();
193+
// Merge the global properties from the project collection.
194+
// It's unclear why MSBuild isn't considering the global properties defined in the ProjectCollection when
195+
// the collection is passed in ProjectOptions below.
196+
foreach (var property in collection.GlobalProperties)
197+
{
198+
if (!(globalProperties ??= new Dictionary<string, string>()).ContainsKey(property.Key))
199+
{
200+
globalProperties.Add(property.Key, property.Value);
201+
}
202+
}
203+
204+
return ProjectInstance.FromFile(projectFilePath, new ProjectOptions
205+
{
206+
GlobalProperties = globalProperties,
207+
EvaluationContext = evaluationContext,
208+
ProjectCollection = collection,
209+
});
191210
}
192211

193212
public static string GetRootDirectory(string solutionOrProjectFilePath)
@@ -197,10 +216,10 @@ public static string GetRootDirectory(string solutionOrProjectFilePath)
197216
return string.IsNullOrEmpty(fileDirectory) ? Directory.GetCurrentDirectory() : fileDirectory;
198217
}
199218

200-
public static IEnumerable<ParallelizableTestModuleGroupWithSequentialInnerModules> GetProjectProperties(string projectFilePath, ProjectCollection projectCollection, BuildOptions buildOptions)
219+
public static IEnumerable<ParallelizableTestModuleGroupWithSequentialInnerModules> GetProjectProperties(string projectFilePath, ProjectCollection projectCollection, EvaluationContext evaluationContext, BuildOptions buildOptions)
201220
{
202221
var projects = new List<ParallelizableTestModuleGroupWithSequentialInnerModules>();
203-
ProjectInstance projectInstance = EvaluateProject(projectCollection, projectFilePath, null);
222+
ProjectInstance projectInstance = EvaluateProject(projectCollection, evaluationContext, projectFilePath, null);
204223

205224
var targetFramework = projectInstance.GetPropertyValue(ProjectProperties.TargetFramework);
206225
var targetFrameworks = projectInstance.GetPropertyValue(ProjectProperties.TargetFrameworks);
@@ -209,7 +228,7 @@ public static IEnumerable<ParallelizableTestModuleGroupWithSequentialInnerModule
209228

210229
if (!string.IsNullOrEmpty(targetFramework) || string.IsNullOrEmpty(targetFrameworks))
211230
{
212-
if (GetModuleFromProject(projectInstance, projectCollection.Loggers, buildOptions) is { } module)
231+
if (GetModuleFromProject(projectInstance, buildOptions) is { } module)
213232
{
214233
projects.Add(new ParallelizableTestModuleGroupWithSequentialInnerModules(module));
215234
}
@@ -234,10 +253,10 @@ public static IEnumerable<ParallelizableTestModuleGroupWithSequentialInnerModule
234253
{
235254
foreach (var framework in frameworks)
236255
{
237-
projectInstance = EvaluateProject(projectCollection, projectFilePath, framework);
256+
projectInstance = EvaluateProject(projectCollection, evaluationContext, projectFilePath, framework);
238257
Logger.LogTrace($"Loaded inner project '{Path.GetFileName(projectFilePath)}' has '{ProjectProperties.IsTestingPlatformApplication}' = '{projectInstance.GetPropertyValue(ProjectProperties.IsTestingPlatformApplication)}' (TFM: '{framework}').");
239258

240-
if (GetModuleFromProject(projectInstance, projectCollection.Loggers, buildOptions) is { } module)
259+
if (GetModuleFromProject(projectInstance, buildOptions) is { } module)
241260
{
242261
projects.Add(new ParallelizableTestModuleGroupWithSequentialInnerModules(module));
243262
}
@@ -248,10 +267,10 @@ public static IEnumerable<ParallelizableTestModuleGroupWithSequentialInnerModule
248267
List<TestModule>? innerModules = null;
249268
foreach (var framework in frameworks)
250269
{
251-
projectInstance = EvaluateProject(projectCollection, projectFilePath, framework);
270+
projectInstance = EvaluateProject(projectCollection, evaluationContext, projectFilePath, framework);
252271
Logger.LogTrace($"Loaded inner project '{Path.GetFileName(projectFilePath)}' has '{ProjectProperties.IsTestingPlatformApplication}' = '{projectInstance.GetPropertyValue(ProjectProperties.IsTestingPlatformApplication)}' (TFM: '{framework}').");
253272

254-
if (GetModuleFromProject(projectInstance, projectCollection.Loggers, buildOptions) is { } module)
273+
if (GetModuleFromProject(projectInstance, buildOptions) is { } module)
255274
{
256275
innerModules ??= new List<TestModule>();
257276
innerModules.Add(module);
@@ -268,7 +287,7 @@ public static IEnumerable<ParallelizableTestModuleGroupWithSequentialInnerModule
268287
return projects;
269288
}
270289

271-
private static TestModule? GetModuleFromProject(ProjectInstance project, ICollection<ILogger>? loggers, BuildOptions buildOptions)
290+
private static TestModule? GetModuleFromProject(ProjectInstance project, BuildOptions buildOptions)
272291
{
273292
_ = bool.TryParse(project.GetPropertyValue(ProjectProperties.IsTestProject), out bool isTestProject);
274293
_ = bool.TryParse(project.GetPropertyValue(ProjectProperties.IsTestingPlatformApplication), out bool isTestingPlatformApplication);
@@ -286,7 +305,7 @@ public static IEnumerable<ParallelizableTestModuleGroupWithSequentialInnerModule
286305
RunProperties runProperties;
287306
if (isTestingPlatformApplication)
288307
{
289-
runProperties = GetRunProperties(project, loggers);
308+
runProperties = GetRunProperties(project);
290309

291310
// dotnet run throws the same if RunCommand is null or empty.
292311
// In dotnet test, we are additionally checking that RunCommand is not dll.
@@ -326,7 +345,7 @@ public static IEnumerable<ParallelizableTestModuleGroupWithSequentialInnerModule
326345

327346
return new TestModule(runProperties, PathUtility.FixFilePath(projectFullPath), targetFramework, isTestingPlatformApplication, launchSettings, project.GetPropertyValue(ProjectProperties.TargetPath), rootVariableName);
328347

329-
static RunProperties GetRunProperties(ProjectInstance project, ICollection<ILogger>? loggers)
348+
static RunProperties GetRunProperties(ProjectInstance project)
330349
{
331350
// Build API cannot be called in parallel, even if the projects are different.
332351
// Otherwise, BuildManager in MSBuild will fail:
@@ -336,7 +355,7 @@ static RunProperties GetRunProperties(ProjectInstance project, ICollection<ILogg
336355
{
337356
if (!project.Build(s_computeRunArgumentsTarget, loggers: null))
338357
{
339-
throw new GracefulException(CliCommandStrings.RunCommandEvaluationExceptionBuildFailed, s_computeRunArgumentsTarget);
358+
throw new GracefulException(CliCommandStrings.RunCommandEvaluationExceptionBuildFailed, s_computeRunArgumentsTarget[0]);
340359
}
341360
}
342361

0 commit comments

Comments
 (0)