-
Notifications
You must be signed in to change notification settings - Fork 280
[MTP] Improve performance of validating command line options #5655
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 6 commits
d88d0ce
1443d5e
85ffb88
39d6db5
78e96f8
975d2d6
86c987c
2e2ee02
07a4700
6c344de
3ba2fb0
e1369e7
780fd3b
fe102f2
cae2a1b
97c8299
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,14 @@ | ||
// Copyright (c) Microsoft Corporation. All rights reserved. | ||
// Licensed under the MIT license. See LICENSE file in the project root for full license information. | ||
|
||
using System; | ||
using System.Collections.Generic; | ||
using System.Diagnostics.CodeAnalysis; | ||
using System.Globalization; | ||
using System.Linq; | ||
using System.Text; | ||
using System.Threading.Tasks; | ||
|
||
using Microsoft.Testing.Platform.Extensions; | ||
using Microsoft.Testing.Platform.Extensions.CommandLine; | ||
using Microsoft.Testing.Platform.Helpers; | ||
|
@@ -101,40 +109,69 @@ | |
Dictionary<ICommandLineOptionsProvider, IReadOnlyCollection<CommandLineOption>> extensionOptionsByProvider, | ||
Dictionary<ICommandLineOptionsProvider, IReadOnlyCollection<CommandLineOption>> systemOptionsByProvider) | ||
{ | ||
IEnumerable<string> allExtensionOptions = extensionOptionsByProvider.Values.SelectMany(x => x).Select(x => x.Name).Distinct(); | ||
IEnumerable<string> allSystemOptions = systemOptionsByProvider.Values.SelectMany(x => x).Select(x => x.Name).Distinct(); | ||
|
||
IEnumerable<string> invalidReservedOptions = allSystemOptions.Intersect(allExtensionOptions); | ||
if (invalidReservedOptions.Any()) | ||
// Create a HashSet of all system option names for faster lookup | ||
HashSet<string> systemOptionNames = new(); | ||
foreach (var provider in systemOptionsByProvider) | ||
Check failure on line 114 in src/Platform/Microsoft.Testing.Platform/CommandLine/CommandLineOptionsValidator.cs
|
||
{ | ||
var stringBuilder = new StringBuilder(); | ||
foreach (string reservedOption in invalidReservedOptions) | ||
foreach (var option in provider.Value) | ||
Check failure on line 116 in src/Platform/Microsoft.Testing.Platform/CommandLine/CommandLineOptionsValidator.cs
|
||
Evangelink marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
IEnumerable<string> faultyProviderNames = extensionOptionsByProvider.Where(tuple => tuple.Value.Any(x => x.Name == reservedOption)).Select(tuple => tuple.Key.DisplayName); | ||
stringBuilder.AppendLine(string.Format(CultureInfo.InvariantCulture, PlatformResources.CommandLineOptionIsReserved, reservedOption, string.Join("', '", faultyProviderNames))); | ||
systemOptionNames.Add(option.Name); | ||
} | ||
} | ||
|
||
return ValidationResult.Invalid(stringBuilder.ToTrimmedString()); | ||
StringBuilder? stringBuilder = null; | ||
foreach (var provider in extensionOptionsByProvider) | ||
Check failure on line 123 in src/Platform/Microsoft.Testing.Platform/CommandLine/CommandLineOptionsValidator.cs
|
||
Evangelink marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
foreach (var option in provider.Value) | ||
{ | ||
if (systemOptionNames.Contains(option.Name)) | ||
{ | ||
stringBuilder ??= new StringBuilder(); | ||
stringBuilder.AppendLine(string.Format(CultureInfo.InvariantCulture, | ||
Youssef1313 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
PlatformResources.CommandLineOptionIsReserved, | ||
option.Name, | ||
provider.Key.DisplayName)); | ||
} | ||
} | ||
} | ||
|
||
return ValidationResult.Valid(); | ||
return stringBuilder?.Length > 0 | ||
? ValidationResult.Invalid(stringBuilder.ToTrimmedString()) | ||
: ValidationResult.Valid(); | ||
} | ||
|
||
private static ValidationResult ValidateOptionsAreNotDuplicated( | ||
Dictionary<ICommandLineOptionsProvider, IReadOnlyCollection<CommandLineOption>> extensionOptionsByProvider) | ||
{ | ||
IEnumerable<string> duplications = extensionOptionsByProvider.Values.SelectMany(x => x) | ||
.Select(x => x.Name) | ||
.GroupBy(x => x) | ||
.Where(x => x.Skip(1).Any()) | ||
.Select(x => x.Key); | ||
// Use a dictionary to track option names and their providers | ||
Dictionary<string, List<ICommandLineOptionsProvider>> optionNameToProviders = new(); | ||
foreach (var kvp in extensionOptionsByProvider) | ||
Youssef1313 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
var provider = kvp.Key; | ||
Youssef1313 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
foreach (var option in kvp.Value) | ||
Youssef1313 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
string name = option.Name; | ||
if (!optionNameToProviders.TryGetValue(name, out var providers)) | ||
Youssef1313 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
providers = new List<ICommandLineOptionsProvider>(); | ||
optionNameToProviders[name] = providers; | ||
} | ||
Youssef1313 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
providers.Add(provider); | ||
} | ||
} | ||
|
||
// Check for duplications | ||
StringBuilder? stringBuilder = null; | ||
foreach (string duplicatedOption in duplications) | ||
foreach (var kvp in optionNameToProviders) | ||
{ | ||
IEnumerable<string> faultyProvidersDisplayNames = extensionOptionsByProvider.Where(tuple => tuple.Value.Any(x => x.Name == duplicatedOption)).Select(tuple => tuple.Key.DisplayName); | ||
stringBuilder ??= new(); | ||
stringBuilder.AppendLine(string.Format(CultureInfo.InvariantCulture, PlatformResources.CommandLineOptionIsDeclaredByMultipleProviders, duplicatedOption, string.Join("', '", faultyProvidersDisplayNames))); | ||
if (kvp.Value.Count > 1) | ||
{ | ||
string duplicatedOption = kvp.Key; | ||
stringBuilder ??= new(); | ||
IEnumerable<string> faultyProvidersDisplayNames = kvp.Value.Select(p => p.DisplayName); | ||
stringBuilder.AppendLine(string.Format(CultureInfo.InvariantCulture, PlatformResources.CommandLineOptionIsDeclaredByMultipleProviders, duplicatedOption, string.Join("', '", faultyProvidersDisplayNames))); | ||
} | ||
} | ||
|
||
return stringBuilder?.Length > 0 | ||
|
@@ -147,10 +184,28 @@ | |
Dictionary<ICommandLineOptionsProvider, IReadOnlyCollection<CommandLineOption>> extensionOptionsByProvider, | ||
Dictionary<ICommandLineOptionsProvider, IReadOnlyCollection<CommandLineOption>> systemOptionsByProvider) | ||
{ | ||
// Create a HashSet of all valid option names for faster lookup | ||
HashSet<string> validOptionNames = new(); | ||
foreach (var provider in extensionOptionsByProvider) | ||
{ | ||
foreach (var option in provider.Value) | ||
{ | ||
validOptionNames.Add(option.Name); | ||
} | ||
} | ||
|
||
foreach (var provider in systemOptionsByProvider) | ||
{ | ||
foreach (var option in provider.Value) | ||
{ | ||
validOptionNames.Add(option.Name); | ||
} | ||
} | ||
|
||
StringBuilder? stringBuilder = null; | ||
foreach (CommandLineParseOption optionRecord in parseResult.Options) | ||
{ | ||
if (!extensionOptionsByProvider.Union(systemOptionsByProvider).Any(tuple => tuple.Value.Any(x => x.Name == optionRecord.Name))) | ||
if (!validOptionNames.Contains(optionRecord.Name)) | ||
{ | ||
stringBuilder ??= new(); | ||
stringBuilder.AppendLine(string.Format(CultureInfo.InvariantCulture, PlatformResources.CommandLineUnknownOption, optionRecord.Name)); | ||
|
@@ -166,7 +221,7 @@ | |
CommandLineParseResult parseResult, | ||
Dictionary<string, (ICommandLineOptionsProvider Provider, CommandLineOption Option)> providerAndOptionByOptionName) | ||
{ | ||
StringBuilder stringBuilder = new(); | ||
StringBuilder? stringBuilder = null; | ||
foreach (IGrouping<string, CommandLineParseOption> groupedOptions in parseResult.Options.GroupBy(x => x.Name)) | ||
{ | ||
// getting the arguments count for an option. | ||
|
@@ -181,19 +236,22 @@ | |
|
||
if (arity > option.Arity.Max && option.Arity.Max == 0) | ||
{ | ||
stringBuilder ??= new(); | ||
stringBuilder.AppendLine(string.Format(CultureInfo.InvariantCulture, PlatformResources.CommandLineOptionExpectsNoArguments, optionName, provider.DisplayName, provider.Uid)); | ||
} | ||
else if (arity < option.Arity.Min) | ||
{ | ||
stringBuilder ??= new(); | ||
stringBuilder.AppendLine(string.Format(CultureInfo.InvariantCulture, PlatformResources.CommandLineOptionExpectsAtLeastArguments, optionName, provider.DisplayName, provider.Uid, option.Arity.Min)); | ||
} | ||
else if (arity > option.Arity.Max) | ||
{ | ||
stringBuilder ??= new(); | ||
stringBuilder.AppendLine(string.Format(CultureInfo.InvariantCulture, PlatformResources.CommandLineOptionExpectsAtMostArguments, optionName, provider.DisplayName, provider.Uid, option.Arity.Max)); | ||
} | ||
} | ||
|
||
return stringBuilder.Length > 0 | ||
return stringBuilder?.Length > 0 | ||
? ValidationResult.Invalid(stringBuilder.ToTrimmedString()) | ||
: ValidationResult.Valid(); | ||
} | ||
|
@@ -254,7 +312,22 @@ | |
} | ||
|
||
private static string ToTrimmedString(this StringBuilder stringBuilder) | ||
#pragma warning disable RS0030 // Do not use banned APIs | ||
=> stringBuilder.ToString().TrimEnd(Environment.NewLine.ToCharArray()); | ||
#pragma warning restore RS0030 // Do not use banned APIs | ||
{ | ||
// Use a more efficient approach to trim without creating unnecessary intermediate strings | ||
string result = stringBuilder.ToString(); | ||
int end = result.Length; | ||
|
||
// Find the last non-whitespace char | ||
while (end > 0) | ||
{ | ||
char c = result[end - 1]; | ||
if (c != '\r' && c != '\n') | ||
{ | ||
break; | ||
} | ||
Check failure on line 327 in src/Platform/Microsoft.Testing.Platform/CommandLine/CommandLineOptionsValidator.cs
|
||
Youssef1313 marked this conversation as resolved.
Show resolved
Hide resolved
Youssef1313 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
end--; | ||
} | ||
|
||
return end == result.Length ? result : result.Substring(0, end); | ||
} | ||
} |
Uh oh!
There was an error while loading. Please reload this page.