diff --git a/src/Platform/Microsoft.Testing.Platform/CommandLine/CommandLineOptionsValidator.cs b/src/Platform/Microsoft.Testing.Platform/CommandLine/CommandLineOptionsValidator.cs index 9ea02b9a03..88f1a28e72 100644 --- a/src/Platform/Microsoft.Testing.Platform/CommandLine/CommandLineOptionsValidator.cs +++ b/src/Platform/Microsoft.Testing.Platform/CommandLine/CommandLineOptionsValidator.cs @@ -101,40 +101,79 @@ private static ValidationResult ValidateExtensionOptionsDoNotContainReservedOpti Dictionary> extensionOptionsByProvider, Dictionary> systemOptionsByProvider) { - IEnumerable allExtensionOptions = extensionOptionsByProvider.Values.SelectMany(x => x).Select(x => x.Name).Distinct(); - IEnumerable allSystemOptions = systemOptionsByProvider.Values.SelectMany(x => x).Select(x => x.Name).Distinct(); + // Create a HashSet of all system option names for faster lookup + var systemOptionNames = new HashSet(); + foreach (KeyValuePair> provider in systemOptionsByProvider) + { + foreach (CommandLineOption option in provider.Value) + { + systemOptionNames.Add(option.Name); + } + } - IEnumerable invalidReservedOptions = allSystemOptions.Intersect(allExtensionOptions); - if (invalidReservedOptions.Any()) + // Aggregate reserved options by name and track all offending providers + var reservedOptionToProviderNames = new Dictionary>(); + foreach (KeyValuePair> kvp in extensionOptionsByProvider) { - var stringBuilder = new StringBuilder(); - foreach (string reservedOption in invalidReservedOptions) + foreach (CommandLineOption option in kvp.Value) { - IEnumerable 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))); + if (systemOptionNames.Contains(option.Name)) + { + if (!reservedOptionToProviderNames.TryGetValue(option.Name, out HashSet? providerNames)) + { + providerNames = []; + reservedOptionToProviderNames[option.Name] = providerNames; + } + + providerNames.Add(kvp.Key.DisplayName); + } } + } - return ValidationResult.Invalid(stringBuilder.ToTrimmedString()); + StringBuilder? stringBuilder = null; + foreach (KeyValuePair> kvp in reservedOptionToProviderNames) + { + stringBuilder ??= new StringBuilder(); + stringBuilder.AppendLine(string.Format(CultureInfo.InvariantCulture, PlatformResources.CommandLineOptionIsReserved, kvp.Key, string.Join("', '", kvp.Value))); } - return ValidationResult.Valid(); + return stringBuilder?.Length > 0 + ? ValidationResult.Invalid(stringBuilder.ToTrimmedString()) + : ValidationResult.Valid(); } private static ValidationResult ValidateOptionsAreNotDuplicated( Dictionary> extensionOptionsByProvider) { - IEnumerable 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 distinct providers + var optionNameToProviders = new Dictionary>(); + foreach (KeyValuePair> kvp in extensionOptionsByProvider) + { + ICommandLineOptionsProvider provider = kvp.Key; + foreach (CommandLineOption option in kvp.Value) + { + string name = option.Name; + if (!optionNameToProviders.TryGetValue(name, out HashSet? providers)) + { + providers = []; + optionNameToProviders[name] = providers; + } + providers.Add(provider); + } + } + + // Check for duplications StringBuilder? stringBuilder = null; - foreach (string duplicatedOption in duplications) + foreach (KeyValuePair> kvp in optionNameToProviders) { - IEnumerable 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 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 +186,28 @@ private static ValidationResult ValidateNoUnknownOptions( Dictionary> extensionOptionsByProvider, Dictionary> systemOptionsByProvider) { + // Create a HashSet of all valid option names for faster lookup + var validOptionNames = new HashSet(); + foreach (KeyValuePair> provider in extensionOptionsByProvider) + { + foreach (CommandLineOption option in provider.Value) + { + validOptionNames.Add(option.Name); + } + } + + foreach (KeyValuePair> provider in systemOptionsByProvider) + { + foreach (CommandLineOption 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 +223,7 @@ private static ValidationResult ValidateOptionsArgumentArity( CommandLineParseResult parseResult, Dictionary providerAndOptionByOptionName) { - StringBuilder stringBuilder = new(); + StringBuilder? stringBuilder = null; foreach (IGrouping groupedOptions in parseResult.Options.GroupBy(x => x.Name)) { // getting the arguments count for an option. @@ -181,19 +238,22 @@ private static ValidationResult ValidateOptionsArgumentArity( 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 +314,13 @@ private static async Task ValidateConfigurationAsync( } 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 + { + // Trim trailing CR/LF characters directly from the StringBuilder to avoid extra allocations + while (stringBuilder.Length > 0 && stringBuilder[stringBuilder.Length - 1] is '\r' or '\n') + { + stringBuilder.Length--; + } + + return stringBuilder.ToString(); + } } diff --git a/test/UnitTests/Microsoft.Testing.Platform.UnitTests/CommandLine/CommandLineHandlerTests.cs b/test/UnitTests/Microsoft.Testing.Platform.UnitTests/CommandLine/CommandLineHandlerTests.cs index 37ecded06a..e4d597437f 100644 --- a/test/UnitTests/Microsoft.Testing.Platform.UnitTests/CommandLine/CommandLineHandlerTests.cs +++ b/test/UnitTests/Microsoft.Testing.Platform.UnitTests/CommandLine/CommandLineHandlerTests.cs @@ -231,6 +231,91 @@ public async Task ParseAndValidateAsync_UnknownOption_ReturnsFalse() Assert.AreEqual("Unknown option '--x'", result.ErrorMessage); } + [TestMethod] + public async Task ParseAndValidateAsync_MultipleUnknownOptions_ReportsAll() + { + // Arrange + string[] args = ["--x", "--y"]; + CommandLineParseResult parseResult = CommandLineParser.Parse(args, new SystemEnvironment()); + + ICommandLineOptionsProvider[] extensionCommandLineProvider = + [ + new ExtensionCommandLineProviderMockUnknownOption() + ]; + + // Act + ValidationResult result = await CommandLineOptionsValidator.ValidateAsync(parseResult, _systemCommandLineOptionsProviders, + extensionCommandLineProvider, new Mock().Object); + + // Assert + Assert.IsFalse(result.IsValid); + Assert.Contains("Unknown option '--x'", result.ErrorMessage); + Assert.Contains("Unknown option '--y'", result.ErrorMessage); + } + + [TestMethod] + public async Task ParseAndValidateAsync_MultipleReservedOptionsFromDifferentProviders_ReturnsFalse() + { + // Arrange + string[] args = []; + CommandLineParseResult parseResult = CommandLineParser.Parse(args, new SystemEnvironment()); + ICommandLineOptionsProvider[] extensionCommandLineProvider = + [ + new ExtensionCommandLineProviderMockReservedOptions(), + new ExtensionCommandLineProviderMockWithNamedOption("help", "Provider2") + ]; + + // Act + ValidationResult result = await CommandLineOptionsValidator.ValidateAsync(parseResult, _systemCommandLineOptionsProviders, + extensionCommandLineProvider, new Mock().Object); + + // Assert + Assert.IsFalse(result.IsValid); + Assert.Contains("Option '--help' is reserved and cannot be used by providers: 'help'", result.ErrorMessage); + } + + [TestMethod] + public async Task ParseAndValidateAsync_DuplicateOptionWithDistinctProviderNames_ReportsAllProviders() + { + // Arrange + string[] args = []; + CommandLineParseResult parseResult = CommandLineParser.Parse(args, new SystemEnvironment()); + ICommandLineOptionsProvider[] extensionCommandLineOptionsProviders = + [ + new ExtensionCommandLineProviderMockWithNamedOption("userOption", "ProviderOne"), + new ExtensionCommandLineProviderMockWithNamedOption("userOption", "ProviderTwo") + ]; + + // Act + ValidationResult result = await CommandLineOptionsValidator.ValidateAsync(parseResult, _systemCommandLineOptionsProviders, + extensionCommandLineOptionsProviders, new Mock().Object); + + // Assert + Assert.IsFalse(result.IsValid); + Assert.Contains("Option '--userOption' is declared by multiple extensions: 'ProviderOne', 'ProviderTwo'", result.ErrorMessage); + } + + [TestMethod] + public async Task ParseAndValidateAsync_ValidOptionsWithManyProviders_ReturnsTrue() + { + // Arrange + string[] args = ["--option1", "--option2", "--option3"]; + CommandLineParseResult parseResult = CommandLineParser.Parse(args, new SystemEnvironment()); + ICommandLineOptionsProvider[] extensionCommandLineOptionsProviders = + [ + new ExtensionCommandLineProviderMockWithNamedOption("option1", "Provider1"), + new ExtensionCommandLineProviderMockWithNamedOption("option2", "Provider2"), + new ExtensionCommandLineProviderMockWithNamedOption("option3", "Provider3") + ]; + + // Act + ValidationResult result = await CommandLineOptionsValidator.ValidateAsync(parseResult, _systemCommandLineOptionsProviders, + extensionCommandLineOptionsProviders, new Mock().Object); + + // Assert + Assert.IsTrue(result.IsValid); + } + [TestMethod] public async Task ParseAndValidateAsync_InvalidValidConfiguration_ReturnsFalse() { @@ -435,4 +520,39 @@ public IReadOnlyCollection GetCommandLineOptions() => public Task ValidateOptionArgumentsAsync(CommandLineOption commandOption, string[] arguments) => ValidationResult.ValidTask; } + + private sealed class ExtensionCommandLineProviderMockWithNamedOption : ICommandLineOptionsProvider + { + private readonly string _option; + + public ExtensionCommandLineProviderMockWithNamedOption(string optionName, string displayName) + { + _option = optionName; + DisplayName = displayName; + Uid = $"TestMock_{displayName}"; + } + + public string Uid { get; } + + /// + public string Version { get; } = AppVersion.DefaultSemVer; + + /// + public string DisplayName { get; } + + /// + public string Description { get; } = "Test extension command line provider"; + + /// + public Task IsEnabledAsync() => Task.FromResult(true); + + public IReadOnlyCollection GetCommandLineOptions() => + [ + new(_option, "Show command line option.", ArgumentArity.ZeroOrOne, false) + ]; + + public Task ValidateCommandLineOptionsAsync(ICommandLineOptions commandLineOptions) => ValidationResult.ValidTask; + + public Task ValidateOptionArgumentsAsync(CommandLineOption commandOption, string[] arguments) => ValidationResult.ValidTask; + } }