diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 297ee73..789735c 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -44,9 +44,13 @@ jobs: - name: Build run: dotnet build MSStore.CLI.sln --no-restore /p:Configuration=Release - name: Test net10.0 + env: + MSSTORE_RUN_PROCESS_TESTS: 'true' run: dotnet run --project MSStore.CLI.UnitTests -f net10.0 --no-build -c Release --coverage --coverage-output-format cobertura --report-trx --results-directory ./TestResults - name: Test net10.0-windows10.0.17763.0 if: ${{ matrix.os == 'windows-latest' }} + env: + MSSTORE_RUN_PROCESS_TESTS: 'true' run: dotnet run --project MSStore.CLI.UnitTests -f net10.0-windows10.0.17763.0 --no-build -c Release --coverage --coverage-output-format cobertura --report-trx --results-directory ./TestResults - name: Publish test results if: ${{ !cancelled() }} diff --git a/.pipelines/templates/build-and-tests.yaml b/.pipelines/templates/build-and-tests.yaml index 2c009e5..9882dbc 100644 --- a/.pipelines/templates/build-and-tests.yaml +++ b/.pipelines/templates/build-and-tests.yaml @@ -39,6 +39,7 @@ steps: displayName: 'Tests net10.0' env: DISPLAY: :0.0 + MSSTORE_RUN_PROCESS_TESTS: 'true' inputs: command: 'run' projects: '**/*[Tt]est*/*.csproj' @@ -48,6 +49,7 @@ steps: condition: startsWith(variables.AgentOS, 'Windows_NT') env: DISPLAY: :0.0 + MSSTORE_RUN_PROCESS_TESTS: 'true' inputs: command: 'run' projects: '**/*[Tt]est*/*.csproj' diff --git a/MSStore.CLI.UnitTests/AppsCommandUnitTests.cs b/MSStore.CLI.UnitTests/AppsCommandUnitTests.cs index f1a73d3..fb7189a 100644 --- a/MSStore.CLI.UnitTests/AppsCommandUnitTests.cs +++ b/MSStore.CLI.UnitTests/AppsCommandUnitTests.cs @@ -23,8 +23,8 @@ public async Task AppsListCommandShouldReturnZero() "list" ]); - result.Output.Should().ContainAll(FakeApps.Select(a => a.Id)); - result.Output.Should().ContainAll(FakeApps.Select(a => a.PrimaryName)); + result.Error.Should().ContainAll(FakeApps.Select(a => a.Id)); + result.Error.Should().ContainAll(FakeApps.Select(a => a.PrimaryName)); } [TestMethod] diff --git a/MSStore.CLI.UnitTests/BaseCommandLineTest.cs b/MSStore.CLI.UnitTests/BaseCommandLineTest.cs index 8a13f5c..6d2a2f0 100644 --- a/MSStore.CLI.UnitTests/BaseCommandLineTest.cs +++ b/MSStore.CLI.UnitTests/BaseCommandLineTest.cs @@ -802,8 +802,9 @@ protected void SetupBasedOnTestDataProjectSubPath(DirectoryInfo dirInfo, string[ var outputCapture = new OutputCapture(Console.Out); var errorCapture = RefreshAnsiConsole(); - // Only stdout is redirected: the error capture is reached exclusively through - // ErrorAnsiConsole, mirroring how Program.cs keeps the two streams apart. + // Only stdout is redirected: it is reserved for StandardOutput payloads. Human-readable writes + // reach the error capture through either ErrorAnsiConsole or the static console, mirroring how + // Program.cs points both at the same instance. Console.SetOut(outputCapture); AnsiConsole.Console = AnsiConsole.Create(new AnsiConsoleSettings @@ -811,7 +812,7 @@ protected void SetupBasedOnTestDataProjectSubPath(DirectoryInfo dirInfo, string[ Ansi = AnsiSupport.Yes, ColorSystem = ColorSystemSupport.TrueColor, Interactive = InteractionSupport.No, - Out = new CustomAnsiConsoleOutput(outputCapture), + Out = new CustomAnsiConsoleOutput(errorCapture), Enrichment = new ProfileEnrichment { UseDefaultEnrichers = false diff --git a/MSStore.CLI.UnitTests/ConsoleFactoryUnitTests.cs b/MSStore.CLI.UnitTests/ConsoleFactoryUnitTests.cs new file mode 100644 index 0000000..6418113 --- /dev/null +++ b/MSStore.CLI.UnitTests/ConsoleFactoryUnitTests.cs @@ -0,0 +1,104 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using MSStore.CLI.Helpers; +using Spectre.Console; + +namespace MSStore.CLI.UnitTests +{ + /// + /// Covers the console that builds, including the static + /// assignment that the ConsoleReader prompts and the browser launcher + /// confirmation depend on. + /// + [TestClass] + public class ConsoleFactoryUnitTests + { + private const string Marker = "console-factory-marker"; + + private IAnsiConsole _previousConsole = null!; + private TextWriter _previousOut = null!; + private TextWriter _previousError = null!; + private StringWriter _stdOut = null!; + private StringWriter _stdError = null!; + + [TestInitialize] + public void Initialize() + { + _previousConsole = AnsiConsole.Console; + _previousOut = Console.Out; + _previousError = Console.Error; + + // ConsoleFactory captures Console.Out/Console.Error when it builds the AnsiConsoleOutput, so the + // redirection has to be in place first. + _stdOut = new StringWriter(); + _stdError = new StringWriter(); + Console.SetOut(_stdOut); + Console.SetError(_stdError); + } + + [TestCleanup] + public void Cleanup() + { + Console.SetOut(_previousOut); + Console.SetError(_previousError); + AnsiConsole.Console = _previousConsole; + _stdOut.Dispose(); + _stdError.Dispose(); + } + + [TestMethod] + public void CreateAndInstallWritesToStandardErrorForStderr() + { + var console = ConsoleFactory.CreateAndInstall(OutputStream.Stderr); + + console.WriteLine(Marker); + + _stdError.ToString().Should().Contain(Marker); + _stdOut.ToString().Should().NotContain(Marker); + } + + [TestMethod] + public void CreateAndInstallWritesToStandardOutputForStdout() + { + var console = ConsoleFactory.CreateAndInstall(OutputStream.Stdout); + + console.WriteLine(Marker); + + _stdOut.ToString().Should().Contain(Marker); + _stdError.ToString().Should().NotContain(Marker); + } + + [TestMethod] + public void CreateAndInstallInstallsTheConsoleAsTheStaticConsole() + { + var console = ConsoleFactory.CreateAndInstall(OutputStream.Stderr); + + AnsiConsole.Console.Should().BeSameAs(console); + } + + [TestMethod] + public void StaticWritesFollowStderr() + { + // The ConsoleReader prompts and the BrowserLauncher confirmation write through the static + // console, so it has to honour the selected stream too. + ConsoleFactory.CreateAndInstall(OutputStream.Stderr); + + AnsiConsole.WriteLine(Marker); + + _stdError.ToString().Should().Contain(Marker); + _stdOut.ToString().Should().NotContain(Marker); + } + + [TestMethod] + public void StaticWritesFollowStdout() + { + ConsoleFactory.CreateAndInstall(OutputStream.Stdout); + + AnsiConsole.WriteLine(Marker); + + _stdOut.ToString().Should().Contain(Marker); + _stdError.ToString().Should().NotContain(Marker); + } + } +} diff --git a/MSStore.CLI.UnitTests/EmptyCommandUnitTests.cs b/MSStore.CLI.UnitTests/EmptyCommandUnitTests.cs index 34409f9..c045c14 100644 --- a/MSStore.CLI.UnitTests/EmptyCommandUnitTests.cs +++ b/MSStore.CLI.UnitTests/EmptyCommandUnitTests.cs @@ -47,7 +47,7 @@ public async Task InfoCommandShouldReturnZero() var result = await ParseAndInvokeAsync(["info"]); - result.Output.Should().Contain("Current Config"); + result.Error.Should().Contain("Current Config"); } [TestMethod] @@ -57,7 +57,7 @@ public async Task InfoCommandShouldReturnZeroWithCert() var result = await ParseAndInvokeAsync(["info"]); - result.Output.Should().Contain("Current Config"); + result.Error.Should().Contain("Current Config"); } } } \ No newline at end of file diff --git a/MSStore.CLI.UnitTests/FlightsCommandUnitTests.cs b/MSStore.CLI.UnitTests/FlightsCommandUnitTests.cs index b8fb4c2..748dca3 100644 --- a/MSStore.CLI.UnitTests/FlightsCommandUnitTests.cs +++ b/MSStore.CLI.UnitTests/FlightsCommandUnitTests.cs @@ -25,8 +25,8 @@ public async Task FlightsListCommandShouldReturnZero() FakeApps[0].Id! ]); - result.Output.Should().ContainAll(FakeFlights.Select(a => a.FlightId)); - result.Output.Should().ContainAll(FakeFlights.Select(a => a.FriendlyName)); + result.Error.Should().ContainAll(FakeFlights.Select(a => a.FlightId)); + result.Error.Should().ContainAll(FakeFlights.Select(a => a.FriendlyName)); } [TestMethod] diff --git a/MSStore.CLI.UnitTests/OutputStreamProcessTests.cs b/MSStore.CLI.UnitTests/OutputStreamProcessTests.cs new file mode 100644 index 0000000..aad757f --- /dev/null +++ b/MSStore.CLI.UnitTests/OutputStreamProcessTests.cs @@ -0,0 +1,213 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System.Diagnostics; +using System.Runtime.InteropServices; +using MSStore.CLI.Services; + +namespace MSStore.CLI.UnitTests +{ + /// + /// Covers the stream routing configured in , which the in-process harness cannot + /// reach: builds its own consoles and never runs + /// Main. These run the built executable and read stdout and stderr separately. + /// + /// + /// Opt-in through MSSTORE_RUN_PROCESS_TESTS, which CI sets. Running the real executable also runs + /// CreateTelemetryClientAsync, which rewrites telemetrySettings.json whenever the telemetry + /// GUID is missing or older than 24 hours, and ConfigurationManager resolves that path through + /// , which ignores LOCALAPPDATA + /// and HOME on Windows. There is no way to redirect it at a temporary profile, so rather than + /// mutate a developer's real configuration these only run where that is harmless. + /// + [TestClass] + public class OutputStreamProcessTests + { + // Emitted by Program's "Command is {Command}" log, which only reaches the console under --verbose and + // is written through the configured Spectre console, so it lands on whichever stream was selected. + private const string HumanOutputMarker = "Command is"; + + private const string OptInEnvironmentVariable = "MSSTORE_RUN_PROCESS_TESTS"; + + [TestInitialize] + public void SkipUnlessOptedIn() + { + if (string.IsNullOrEmpty(Environment.GetEnvironmentVariable(OptInEnvironmentVariable))) + { + Assert.Inconclusive( + $"Set {OptInEnvironmentVariable} to run these tests. They execute the real CLI, which rewrites the telemetry settings of whoever runs them."); + } + } + + [TestMethod] + public async Task DefaultRoutesHumanReadableOutputToStandardError() + { + var result = await RunCliAsync(["--verbose", "--help"], null); + + result.ExitCode.Should().Be(0); + result.StdErr.Should().Contain(HumanOutputMarker); + result.StdOut.Should().NotContain(HumanOutputMarker); + } + + [DataRow("--output-stream", "stdout")] + [DataRow("--output-stream=stdout", null)] + [DataRow("--output-stream:stdout", null)] + [DataRow("--output-stream", "STDOUT")] + [TestMethod] + public async Task OptionRoutesHumanReadableOutputToStandardOutput(string arg, string? value) + { + string[] args = value == null + ? ["--verbose", arg, "--help"] + : ["--verbose", arg, value, "--help"]; + + var result = await RunCliAsync(args, null); + + // Guards against the command failing for an unrelated reason while the marker still lands on the + // right stream. It does not prove the spelling parsed: --help takes precedence over parse errors, + // so an unrecognized token here would still exit 0. Parser acceptance of each spelling is covered + // in-process by OutputStreamUnitTests, where no help action is involved. + result.ExitCode.Should().Be(0); + result.StdOut.Should().Contain(HumanOutputMarker); + result.StdErr.Should().NotContain(HumanOutputMarker); + } + + [TestMethod] + public async Task EnvironmentVariableRoutesHumanReadableOutputToStandardOutput() + { + var result = await RunCliAsync(["--verbose", "--help"], "stdout"); + + result.ExitCode.Should().Be(0); + result.StdOut.Should().Contain(HumanOutputMarker); + result.StdErr.Should().NotContain(HumanOutputMarker); + } + + [TestMethod] + public async Task OptionOverridesTheEnvironmentVariable() + { + var result = await RunCliAsync(["--verbose", "--output-stream", "stderr", "--help"], "stdout"); + + result.ExitCode.Should().Be(0); + result.StdErr.Should().Contain(HumanOutputMarker); + result.StdOut.Should().NotContain(HumanOutputMarker); + } + + [TestMethod] + public async Task InvalidEnvironmentVariableFallsBackToStandardErrorWithAWarning() + { + var result = await RunCliAsync(["--verbose", "--help"], "1"); + + // A malformed environment variable warns and falls back; it must not fail the command. + result.ExitCode.Should().Be(0); + result.StdErr.Should().Contain(HumanOutputMarker); + result.StdErr.Should().Contain(EnvironmentInfo.OutputStreamEnvironmentVariable); + result.StdOut.Should().NotContain(HumanOutputMarker); + } + + [TestMethod] + public async Task InvalidOptionValueIsRejected() + { + // No --help here: System.CommandLine gives the help action precedence over the parse error, so + // `--output-stream console --help` exits 0. The rejection is only observable without it. + var result = await RunCliAsync(["--verbose", "--output-stream", "console"], null); + + result.ExitCode.Should().NotBe(0); + result.StdErr.Should().Contain("--output-stream"); + } + + [DataRow(null)] + [DataRow("stdout")] + [TestMethod] + public async Task HelpAlwaysGoesToStandardOutput(string? environmentValue) + { + // System.CommandLine writes help through InvocationConfiguration.Output, which --output-stream + // deliberately leaves alone so that `msstore --help | more` keeps working. + var result = await RunCliAsync(["--help"], environmentValue); + + result.ExitCode.Should().Be(0); + result.StdOut.Should().Contain("Usage:"); + } + + private static async Task<(int ExitCode, string StdOut, string StdErr)> RunCliAsync(string[] args, string? outputStreamEnvironmentValue) + { + var startInfo = new ProcessStartInfo(ResolveCliExecutable()) + { + RedirectStandardOutput = true, + RedirectStandardError = true, + UseShellExecute = false, + CreateNoWindow = true + }; + + foreach (var arg in args) + { + startInfo.ArgumentList.Add(arg); + } + + if (outputStreamEnvironmentValue == null) + { + // An ambient value on the machine running the tests must not influence the result. + startInfo.Environment.Remove(EnvironmentInfo.OutputStreamEnvironmentVariable); + } + else + { + startInfo.Environment[EnvironmentInfo.OutputStreamEnvironmentVariable] = outputStreamEnvironmentValue; + } + + using var process = Process.Start(startInfo)!; + + var stdOutTask = process.StandardOutput.ReadToEndAsync(); + var stdErrTask = process.StandardError.ReadToEndAsync(); + + using var timeout = new CancellationTokenSource(TimeSpan.FromMinutes(2)); + try + { + await process.WaitForExitAsync(timeout.Token); + } + catch (OperationCanceledException) + { + process.Kill(entireProcessTree: true); + throw; + } + + return (process.ExitCode, await stdOutTask, await stdErrTask); + } + + /// + /// Locates the CLI built alongside this test assembly. + /// + /// The full path to the executable. + /// + /// A miss is a failure rather than a skip: these are the only tests covering the real + /// stream wiring, so quietly reporting green would drop that coverage the moment + /// the build layout changes. + /// + private static string ResolveCliExecutable() + { + // The test binary lives in /MSStore.CLI.UnitTests/bin//, + // and the CLI is built alongside it under the same configuration and target framework. + var testOutputDirectory = new DirectoryInfo(AppContext.BaseDirectory); + var targetFramework = testOutputDirectory.Name; + var configuration = testOutputDirectory.Parent?.Name; + + var repositoryRoot = testOutputDirectory; + while (repositoryRoot != null && !File.Exists(Path.Combine(repositoryRoot.FullName, "MSStore.CLI.sln"))) + { + repositoryRoot = repositoryRoot.Parent; + } + + if (configuration == null || repositoryRoot == null) + { + Assert.Fail($"Could not locate MSStore.CLI.sln or the build configuration by walking up from '{AppContext.BaseDirectory}'."); + } + + var fileName = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? "msstore.exe" : "msstore"; + var cliPath = Path.Combine(repositoryRoot.FullName, "MSStore.CLI", "bin", configuration, targetFramework, fileName); + + if (!File.Exists(cliPath)) + { + Assert.Fail($"The MSStore.CLI executable was not found at '{cliPath}'. Build MSStore.CLI.sln for configuration '{configuration}' and target framework '{targetFramework}' before running these tests."); + } + + return cliPath; + } + } +} diff --git a/MSStore.CLI.UnitTests/OutputStreamUnitTests.cs b/MSStore.CLI.UnitTests/OutputStreamUnitTests.cs new file mode 100644 index 0000000..eb2bac1 --- /dev/null +++ b/MSStore.CLI.UnitTests/OutputStreamUnitTests.cs @@ -0,0 +1,295 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using MSStore.CLI.Helpers; +using MSStore.CLI.Services; + +namespace MSStore.CLI.UnitTests +{ + [TestClass] + public class OutputStreamUnitTests : BaseCommandLineTest + { + [TestInitialize] + public void Init() + { + FakeLogin(); + AddDefaultFakeAccount(); + AddFakeApps(); + } + + [TestCleanup] + public void ResetOutputStreamEnvironmentVariable() + { + Environment.SetEnvironmentVariable(EnvironmentInfo.OutputStreamEnvironmentVariable, null); + } + + [TestMethod] + public void ResolveDefaultsToStderr() + { + var (stream, warning) = OutputStreamResolver.Resolve([], null); + + stream.Should().Be(OutputStream.Stderr); + warning.Should().BeNull(); + } + + [DataRow("stdout", nameof(OutputStream.Stdout))] + [DataRow("stderr", nameof(OutputStream.Stderr))] + [DataRow("STDOUT", nameof(OutputStream.Stdout))] + [DataRow("StdErr", nameof(OutputStream.Stderr))] + [TestMethod] + public void ResolveReadsTheOptionValueCaseInsensitively(string value, string expected) + { + var (stream, warning) = OutputStreamResolver.Resolve(["publish", "--output-stream", value], null); + + stream.Should().Be(Enum.Parse(expected)); + warning.Should().BeNull(); + } + + [DataRow("--output-stream=stdout")] + [DataRow("--output-stream:stdout")] + [TestMethod] + public void ResolveSupportsInlineValueSeparators(string arg) + { + var (stream, warning) = OutputStreamResolver.Resolve(["publish", arg], null); + + stream.Should().Be(OutputStream.Stdout); + warning.Should().BeNull(); + } + + [TestMethod] + public void ResolveUsesTheLastOccurrenceWhenTheOptionIsRepeated() + { + var (stream, _) = OutputStreamResolver.Resolve( + ["publish", "--output-stream", "stdout", "--output-stream", "stderr"], + null); + + stream.Should().Be(OutputStream.Stderr); + } + + [TestMethod] + public void ResolveIgnoresTheOptionWhenItHasNoValue() + { + var (stream, warning) = OutputStreamResolver.Resolve(["publish", "--output-stream"], null); + + stream.Should().Be(OutputStream.Stderr); + warning.Should().BeNull(); + } + + [TestMethod] + public void ResolveDoesNotMatchOptionsThatMerelyStartWithTheSameText() + { + var (stream, _) = OutputStreamResolver.Resolve(["package", "--output-streamer", "stdout"], null); + + stream.Should().Be(OutputStream.Stderr); + } + + [TestMethod] + public void ResolveDoesNotConfuseTheOptionWithTheOutputDirectoryOption() + { + var (stream, _) = OutputStreamResolver.Resolve(["package", "--output", "C:\\packages"], null); + + stream.Should().Be(OutputStream.Stderr); + } + + [TestMethod] + public void ResolveReadsTheEnvironmentVariableWhenTheOptionIsAbsent() + { + var (stream, warning) = OutputStreamResolver.Resolve(["publish"], "stdout"); + + stream.Should().Be(OutputStream.Stdout); + warning.Should().BeNull(); + } + + [DataRow("stdout", "stderr", nameof(OutputStream.Stderr))] + [DataRow("stderr", "stdout", nameof(OutputStream.Stdout))] + [TestMethod] + public void ResolveLetsTheOptionOverrideTheEnvironmentVariable(string environmentValue, string optionValue, string expected) + { + var (stream, warning) = OutputStreamResolver.Resolve( + ["package", "--output-stream", optionValue], + environmentValue); + + stream.Should().Be(Enum.Parse(expected)); + warning.Should().BeNull(); + } + + [TestMethod] + public void ResolveWarnsAndFallsBackWhenTheEnvironmentVariableIsInvalid() + { + var (stream, warning) = OutputStreamResolver.Resolve(["publish"], "console"); + + stream.Should().Be(OutputStream.Stderr); + warning.Should().Contain("console"); + warning.Should().Contain(EnvironmentInfo.OutputStreamEnvironmentVariable); + } + + [DataRow("")] + [DataRow(" ")] + [TestMethod] + public void ResolveTreatsABlankEnvironmentVariableAsUnset(string environmentValue) + { + var (stream, warning) = OutputStreamResolver.Resolve(["publish"], environmentValue); + + stream.Should().Be(OutputStream.Stderr); + warning.Should().BeNull(); + } + + [TestMethod] + public void ResolveDoesNotWarnForAnInvalidOptionValue() + { + // The parser reports invalid option values, so the resolver just falls through. + var (stream, warning) = OutputStreamResolver.Resolve(["publish", "--output-stream", "console"], null); + + stream.Should().Be(OutputStream.Stderr); + warning.Should().BeNull(); + } + + [DataRow("0")] + [DataRow("1")] + [DataRow("2")] + [TestMethod] + public void ResolveRejectsNumericEnumValues(string environmentValue) + { + // Only the two names are part of the contract, so the underlying numbers must not be accepted. + var (stream, warning) = OutputStreamResolver.Resolve(["publish"], environmentValue); + + stream.Should().Be(OutputStream.Stderr); + warning.Should().Contain(environmentValue); + } + + [DataRow("--output-stream", "stdout")] + [DataRow("--output-stream=stdout", null)] + [TestMethod] + public void ResolveStopsScanningAtTheEndOfOptionsMarker(string arg, string? value) + { + // System.CommandLine treats everything after `--` as a literal argument, so the resolver must too. + string[] args = value == null ? ["package", "--", arg] : ["package", "--", arg, value]; + + var (stream, _) = OutputStreamResolver.Resolve(args, null); + + stream.Should().Be(OutputStream.Stderr); + } + + [TestMethod] + public void ResolveStillReadsTheOptionBeforeTheEndOfOptionsMarker() + { + var (stream, _) = OutputStreamResolver.Resolve( + ["package", "--output-stream", "stdout", "--", "--output-stream=stderr"], + null); + + stream.Should().Be(OutputStream.Stdout); + } + + [TestMethod] + public void ResolveDoesNotConsumeTheEndOfOptionsMarkerAsTheOptionValue() + { + // System.CommandLine reports a missing value here and treats the rest as literals, so the + // resolver must not take `--` as the value and then pick up the trailing token. + var (stream, _) = OutputStreamResolver.Resolve( + ["package", "--output-stream", "--", "--output-stream=stdout"], + null); + + stream.Should().Be(OutputStream.Stderr); + } + + [TestMethod] + public void ResolveKeepsAnEarlierValueWhenTheOptionLaterPrecedesTheEndOfOptionsMarker() + { + var (stream, _) = OutputStreamResolver.Resolve( + ["package", "--output-stream=stdout", "--output-stream", "--", "stderr"], + null); + + stream.Should().Be(OutputStream.Stdout); + } + + [TestMethod] + public void ResolveReadsTheRealEnvironmentVariable() + { + Environment.SetEnvironmentVariable(EnvironmentInfo.OutputStreamEnvironmentVariable, "stdout"); + + var (stream, warning) = OutputStreamResolver.Resolve(["publish"]); + + stream.Should().Be(OutputStream.Stdout); + warning.Should().BeNull(); + } + + [DataRow("stdout")] + [DataRow("stderr")] + [TestMethod] + public async Task OutputStreamOptionIsAcceptedByCommands(string value) + { + var appId = FakeApps[2].Id!; + + var result = await ParseAndInvokeAsync( + [ + "apps", + "get", + appId, + "--output-stream", + value + ]); + + // Machine-readable payloads always go to stdout, whichever stream the human-readable + // output was routed to. + result.Output.Should().Contain($"\"Id\": \"{appId}\","); + } + + [DataRow("--output-stream=stdout")] + [DataRow("--output-stream:stdout")] + [DataRow("--output-stream=stderr")] + [DataRow("--output-stream:stderr")] + [TestMethod] + public async Task InlineOutputStreamOptionFormsAreAcceptedByCommands(string arg) + { + // OutputStreamResolver accepts both inline separators, so the parser has to as well. This is + // asserted here rather than in the process tests because those pass --help, which takes + // precedence over parse errors and would mask an unrecognized token. ParseAndInvokeAsync + // asserts an exit code of 0, so a rejected spelling fails. + var appId = FakeApps[2].Id!; + + var result = await ParseAndInvokeAsync( + [ + "apps", + "get", + appId, + arg + ]); + + result.Output.Should().Contain($"\"Id\": \"{appId}\","); + } + + [TestMethod] + public async Task InvalidOutputStreamOptionValueIsRejectedByTheParser() + { + var result = await ParseAndInvokeAsync( + [ + "apps", + "list", + "--output-stream", + "console" + ], + 1); + + result.Error.Should().Contain("--output-stream"); + } + + [DataRow("0")] + [DataRow("1")] + [TestMethod] + public async Task NumericOutputStreamOptionValueIsRejectedByTheParser(string value) + { + // The parser and OutputStreamResolver have to agree: the resolver rejects the underlying + // numbers, so the option must not silently accept them through the built-in enum converter. + var result = await ParseAndInvokeAsync( + [ + "apps", + "list", + "--output-stream", + value + ], + 1); + + result.Error.Should().Contain("--output-stream"); + } + } +} diff --git a/MSStore.CLI/Commands/Apps/ListCommand.cs b/MSStore.CLI/Commands/Apps/ListCommand.cs index d90083f..bdd1678 100644 --- a/MSStore.CLI/Commands/Apps/ListCommand.cs +++ b/MSStore.CLI/Commands/Apps/ListCommand.cs @@ -67,11 +67,11 @@ public override async Task InvokeAsync(ParseResult parseResult, Cancellatio i++; } - AnsiConsole.Write(table); + _ansiConsole.Write(table); return await _telemetryClient.TrackCommandEventAsync(0, ct); } - AnsiConsole.MarkupLine("Your account has [bold][u]no[/] Managed apps[/]."); + _ansiConsole.MarkupLine("Your account has [bold][u]no[/] Managed apps[/]."); return await _telemetryClient.TrackCommandEventAsync(-1, ct); } } diff --git a/MSStore.CLI/Commands/Flights/ListCommand.cs b/MSStore.CLI/Commands/Flights/ListCommand.cs index c83e625..a6c3f93 100644 --- a/MSStore.CLI/Commands/Flights/ListCommand.cs +++ b/MSStore.CLI/Commands/Flights/ListCommand.cs @@ -79,7 +79,7 @@ public override async Task InvokeAsync(ParseResult parseResult, Cancellatio i++; } - AnsiConsole.Write(table); + _ansiConsole.Write(table); return await _telemetryClient.TrackCommandEventAsync(0, ct); } else diff --git a/MSStore.CLI/Commands/InfoCommand.cs b/MSStore.CLI/Commands/InfoCommand.cs index d4fe0f7..5c9df65 100644 --- a/MSStore.CLI/Commands/InfoCommand.cs +++ b/MSStore.CLI/Commands/InfoCommand.cs @@ -21,9 +21,10 @@ public InfoCommand() { } - public class Handler(IConfigurationManager configurationManager, TelemetryClient telemetryClient, ILogger logger) : AsynchronousCommandLineAction + public class Handler(IConfigurationManager configurationManager, IAnsiConsole ansiConsole, TelemetryClient telemetryClient, ILogger logger) : AsynchronousCommandLineAction { private readonly IConfigurationManager _configurationManager = configurationManager ?? throw new ArgumentNullException(nameof(configurationManager)); + private readonly IAnsiConsole _ansiConsole = ansiConsole ?? throw new ArgumentNullException(nameof(ansiConsole)); private readonly TelemetryClient _telemetryClient = telemetryClient ?? throw new ArgumentNullException(nameof(telemetryClient)); private readonly ILogger _logger = logger ?? throw new ArgumentNullException(nameof(logger)); @@ -79,7 +80,7 @@ public override async Task InvokeAsync(ParseResult parseResult, Cancellatio _logger.LogInformation("Settings File Path: {@SettingsFilePath}", _configurationManager.ConfigPath); } - AnsiConsole.Write(table); + _ansiConsole.Write(table); return await _telemetryClient.TrackCommandEventAsync(0, ct); } diff --git a/MSStore.CLI/Helpers/ConsoleFactory.cs b/MSStore.CLI/Helpers/ConsoleFactory.cs new file mode 100644 index 0000000..ea4acc1 --- /dev/null +++ b/MSStore.CLI/Helpers/ConsoleFactory.cs @@ -0,0 +1,41 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System; +using Spectre.Console; + +namespace MSStore.CLI.Helpers +{ + /// + /// Builds the console that every human-readable write goes through. + /// + internal static class ConsoleFactory + { + /// + /// Creates the console for and installs it as the static + /// . + /// + /// The stream human-readable output should be written to. + /// The console, which is also registered in the service collection. + /// + /// The prompts and the + /// confirmation still reach for the static console. Installing the same instance keeps them on the + /// selected stream instead of Spectre's default stdout console, and leaves stdout to + /// . + /// + public static IAnsiConsole CreateAndInstall(OutputStream outputStream) + { + var useStdout = outputStream == OutputStream.Stdout; + + var console = AnsiConsole.Create(new AnsiConsoleSettings + { + Interactive = (useStdout ? Console.IsOutputRedirected : Console.IsErrorRedirected) ? InteractionSupport.No : InteractionSupport.Yes, + Out = new AnsiConsoleOutput(useStdout ? Console.Out : Console.Error) + }); + + AnsiConsole.Console = console; + + return console; + } + } +} diff --git a/MSStore.CLI/Helpers/OutputStream.cs b/MSStore.CLI/Helpers/OutputStream.cs new file mode 100644 index 0000000..069f601 --- /dev/null +++ b/MSStore.CLI/Helpers/OutputStream.cs @@ -0,0 +1,27 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +namespace MSStore.CLI.Helpers +{ + /// + /// The standard stream that human-readable console output is written to. + /// + /// + /// This never affects machine-readable payloads, which always go to stdout through + /// . + /// + internal enum OutputStream + { + /// + /// Human-readable output goes to standard error. This is the default, and keeps stdout + /// clean so payloads can be piped or captured. + /// + Stderr, + + /// + /// Human-readable output goes to standard output. Useful on Azure DevOps, which renders + /// every stderr line as ##[error]. + /// + Stdout + } +} diff --git a/MSStore.CLI/Helpers/OutputStreamResolver.cs b/MSStore.CLI/Helpers/OutputStreamResolver.cs new file mode 100644 index 0000000..ff9301d --- /dev/null +++ b/MSStore.CLI/Helpers/OutputStreamResolver.cs @@ -0,0 +1,165 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System; +using System.Collections.Generic; +using MSStore.CLI.Services; + +namespace MSStore.CLI.Helpers +{ + /// + /// Resolves which standard stream human-readable output should be written to. + /// + /// + /// + /// The resolution order is --output-stream > + /// > . The flag deliberately wins so that a pipeline-wide environment + /// variable can be overridden on the individual commands that emit a machine-readable payload. + /// + /// + /// has to build the before the command line is + /// parsed, because the host builder needs it in the service collection. The raw arguments are therefore + /// inspected here, the same way --verbose is handled. + /// + /// + internal static class OutputStreamResolver + { + internal const string OptionName = "--output-stream"; + + private const string EndOfOptions = "--"; + + private static readonly char[] InlineValueSeparators = [':', '=']; + + /// + /// Resolves the stream from the raw command line arguments and the environment. + /// + /// The raw command line arguments. + /// The resolved stream, and a warning to surface when the environment variable is malformed. + public static (OutputStream Stream, string? Warning) Resolve(IReadOnlyList args) + { + string? environmentValue; + try + { + environmentValue = Environment.GetEnvironmentVariable(EnvironmentInfo.OutputStreamEnvironmentVariable); + } + catch (Exception) + { + // Reading the environment can throw under restricted hosts. Fall back to the default. + environmentValue = null; + } + + return Resolve(args, environmentValue); + } + + /// + /// Resolves the stream from the raw command line arguments and an explicit environment variable value. + /// + /// The raw command line arguments. + /// The value of the environment variable, or null when it is not set. + /// The resolved stream, and a warning to surface when the environment variable is malformed. + public static (OutputStream Stream, string? Warning) Resolve(IReadOnlyList args, string? environmentValue) + { + ArgumentNullException.ThrowIfNull(args); + + if (TryParse(FindOptionValue(args), out var fromArgs)) + { + return (fromArgs, null); + } + + if (string.IsNullOrWhiteSpace(environmentValue)) + { + return (OutputStream.Stderr, null); + } + + if (TryParse(environmentValue, out var fromEnvironment)) + { + return (fromEnvironment, null); + } + + return ( + OutputStream.Stderr, + $"'{environmentValue}' is not a valid {EnvironmentInfo.OutputStreamEnvironmentVariable} value. Expected '{nameof(OutputStream.Stdout)}' or '{nameof(OutputStream.Stderr)}'. Falling back to '{nameof(OutputStream.Stderr)}'."); + } + + /// + /// Parses a stream name, accepting any casing. + /// + /// The value to parse. + /// The parsed stream. + /// True when the value names a known stream. + /// + /// Only the two names are accepted. would also + /// accept the underlying numeric values, which are not part of the documented contract. + /// + public static bool TryParse(string? value, out OutputStream outputStream) + { + outputStream = OutputStream.Stderr; + + if (string.IsNullOrWhiteSpace(value)) + { + return false; + } + + var name = value.Trim(); + + if (string.Equals(name, nameof(OutputStream.Stdout), StringComparison.OrdinalIgnoreCase)) + { + outputStream = OutputStream.Stdout; + return true; + } + + return string.Equals(name, nameof(OutputStream.Stderr), StringComparison.OrdinalIgnoreCase); + } + + /// + /// Finds the value of the last --output-stream occurrence, supporting both the + /// --output-stream value and --output-stream=value forms. + /// + /// The raw command line arguments. + /// The value, or null when the option is absent. + /// + /// Scanning stops at a standalone --, because System.CommandLine treats everything after it as + /// literal arguments rather than options. + /// + private static string? FindOptionValue(IReadOnlyList args) + { + string? value = null; + + for (var i = 0; i < args.Count; i++) + { + var arg = args[i]; + if (arg == null) + { + continue; + } + + if (string.Equals(arg, EndOfOptions, StringComparison.Ordinal)) + { + break; + } + + if (arg.Length > OptionName.Length + && arg.StartsWith(OptionName, StringComparison.Ordinal) + && Array.IndexOf(InlineValueSeparators, arg[OptionName.Length]) >= 0) + { + value = arg[(OptionName.Length + 1)..]; + } + else if (string.Equals(arg, OptionName, StringComparison.Ordinal) && i + 1 < args.Count) + { + // System.CommandLine treats a following `--` as the end-of-options marker rather than a + // value, and reports the option's value as missing. Stop rather than consuming the marker + // and carrying on past it, which would pick up a later literal the parser never accepts. + if (string.Equals(args[i + 1], EndOfOptions, StringComparison.Ordinal)) + { + break; + } + + value = args[i + 1]; + i++; + } + } + + return value; + } + } +} diff --git a/MSStore.CLI/Helpers/StandardOutput.cs b/MSStore.CLI/Helpers/StandardOutput.cs index 1de2162..ba29eb2 100644 --- a/MSStore.CLI/Helpers/StandardOutput.cs +++ b/MSStore.CLI/Helpers/StandardOutput.cs @@ -12,9 +12,16 @@ internal static class StandardOutput /// /// The text to write. /// + /// /// This deliberately bypasses Spectre.Console's : its renderer /// word-wraps at the console width (falling back to 80 columns when stdout is redirected), which injects /// raw newline characters inside JSON string values and produces invalid JSON. + /// + /// + /// Machine-readable payloads always go to stdout, regardless of --output-stream. Pass + /// --output-stream stderr on these commands when a pipeline-wide + /// MSSTORE_OUTPUT_STREAM=stdout would otherwise interleave human-readable output with the payload. + /// /// public static void WriteLine(string value) { diff --git a/MSStore.CLI/MicrosoftStoreCLI.cs b/MSStore.CLI/MicrosoftStoreCLI.cs index 1efbdbf..5a36e99 100644 --- a/MSStore.CLI/MicrosoftStoreCLI.cs +++ b/MSStore.CLI/MicrosoftStoreCLI.cs @@ -21,6 +21,8 @@ internal class MicrosoftStoreCLI : RootCommand { internal static Option VerboseOption { get; } + internal static Option OutputStreamOption { get; } + static MicrosoftStoreCLI() { VerboseOption = new Option("--verbose", "-v") @@ -28,6 +30,28 @@ static MicrosoftStoreCLI() DefaultValueFactory = _ => false, Description = "Verbose output" }; + + OutputStreamOption = new Option(OutputStreamResolver.OptionName) + { + Description = $"The stream that human-readable progress and status output is written to. Defaults to '{nameof(OutputStream.Stderr)}', which keeps stdout for machine-readable payloads. Use '{nameof(OutputStream.Stdout)}' on Azure DevOps, which reports every stderr line as an error. Also settable through the {EnvironmentInfo.OutputStreamEnvironmentVariable} environment variable, which this option overrides.", + + // The built-in enum converter also accepts the underlying numbers, which OutputStreamResolver + // rejects. That would let "--output-stream 1" parse as Stdout while the resolver, which is what + // actually selects the stream before the host is built, fell back to Stderr. Both sides share + // TryParse so there is a single contract. The arity is ExactlyOne, so a missing value fails to + // parse before this runs. + CustomParser = result => + { + var value = result.Tokens[0].Value; + if (OutputStreamResolver.TryParse(value, out var outputStream)) + { + return outputStream; + } + + result.AddError($"Cannot parse argument '{value}' for option '{OutputStreamResolver.OptionName}'. Expected '{nameof(OutputStream.Stdout)}' or '{nameof(OutputStream.Stderr)}'."); + return OutputStream.Stderr; + } + }; } internal static void WelcomeMessage(IAnsiConsole ansiConsole) diff --git a/MSStore.CLI/Program.cs b/MSStore.CLI/Program.cs index d969440..5f6d1cd 100644 --- a/MSStore.CLI/Program.cs +++ b/MSStore.CLI/Program.cs @@ -52,11 +52,13 @@ public static async Task Main(params string[] args) null); TelemetryConfigurations telemetryConfigurations = await telemetryConfigurationManager.LoadAsync(true, CancellationToken.None); TelemetryClient telemetryClient = await CreateTelemetryClientAsync(telemetryConfigurationManager, telemetryConfigurations); - var ansiConsole = AnsiConsole.Create(new() + var (outputStream, outputStreamWarning) = OutputStreamResolver.Resolve(args); + var ansiConsole = ConsoleFactory.CreateAndInstall(outputStream); + + if (outputStreamWarning != null) { - Interactive = Console.IsErrorRedirected ? InteractionSupport.No : InteractionSupport.Yes, - Out = new AnsiConsoleOutput(Console.Error) - }); + ansiConsole.MarkupLine($":warning: {outputStreamWarning.EscapeMarkup()}"); + } if (args.Contains(MicrosoftStoreCLI.VerboseOption.Name) || args.Any(MicrosoftStoreCLI.VerboseOption.Aliases.Contains)) { @@ -243,6 +245,9 @@ void AddMSCorrelationId(HttpRequestHeaders defaultRequestHeaders) parseError.ShowHelp = true; } + // InvocationConfiguration is left at its defaults on purpose, so --output-stream does not move it: + // help goes to stdout so that `msstore --help | more` works, and parse diagnostics go to stderr + // because they accompany a non-zero exit code. var result = await parseResult.InvokeAsync(parseResult.InvocationConfiguration, lifetime.ApplicationStopping); await host.StopAsync(); diff --git a/MSStore.CLI/Services/EnvironmentInfo.cs b/MSStore.CLI/Services/EnvironmentInfo.cs index 543969e..23d16b5 100644 --- a/MSStore.CLI/Services/EnvironmentInfo.cs +++ b/MSStore.CLI/Services/EnvironmentInfo.cs @@ -28,6 +28,9 @@ internal class EnvironmentInfo // Environment variable for client assertion file path, used for authentication. public static readonly string ClientAssertionFileEnvironmentVariable = "MSSTORE_CLIENT_ASSERTION_FILE"; + // Environment variable that selects the standard stream used for human-readable output. + public static readonly string OutputStreamEnvironmentVariable = "MSSTORE_OUTPUT_STREAM"; + // Cached environment information, loaded only once private static readonly Lazy _cachedEnvironmentInfo = new Lazy(ComputeEnvironmentInfo); diff --git a/MSStore.CLI/StoreHostBuilderExtensions.cs b/MSStore.CLI/StoreHostBuilderExtensions.cs index 8670b3f..2c21d40 100644 --- a/MSStore.CLI/StoreHostBuilderExtensions.cs +++ b/MSStore.CLI/StoreHostBuilderExtensions.cs @@ -109,6 +109,7 @@ public static IHostBuilder ConfigureStoreCLICommands(this IHostBuilder builder) { var command = ActivatorUtilities.CreateInstance(sp); command.Options.Add(MicrosoftStoreCLI.VerboseOption); + command.Options.Add(MicrosoftStoreCLI.OutputStreamOption); command.SetAction((parseResult, ct) => sp.GetRequiredService().InvokeAsync(parseResult, ct)); return command; }); @@ -122,6 +123,7 @@ public static IHostBuilder ConfigureStoreCLICommands(this IHostBuilder builder) { var command = ActivatorUtilities.CreateInstance(sp); command.Options.Add(MicrosoftStoreCLI.VerboseOption); + command.Options.Add(MicrosoftStoreCLI.OutputStreamOption); return command; }); } diff --git a/README.md b/README.md index f01df15..dc87062 100644 --- a/README.md +++ b/README.md @@ -8,6 +8,54 @@ The Microsoft Store Developer Command Line Interface is a cross-platform (Window ## Helpful links * [Documentation](https://aka.ms/msstoredevcli/docs) - Microsoft's official documentation on regards to available commands, installation steps, how to properly setup CI/CD environments, and general guidance. +## Standard output vs. standard error + +By default the CLI splits its output as follows: + +* **stdout** carries the command's result — machine-readable payloads such as the JSON emitted by `submission get`, `apps get` and `submission rollout get`, the package path printed by `package`, and `--help` text. This keeps `msstore submission get ... | ConvertFrom-Json` and `$(msstore package ...)` reliable. +* **stderr** carries everything else meant for a human — progress, status, success messages, tables, prompts and verbose logging. + +`--output-stream stdout` deliberately breaks that separation: it moves the human-readable half onto stdout, where it is interleaved with any payload. + +Two things sit outside the option's scope on purpose, matching the behavior of other CLIs: + +* Machine-readable payloads are always written to stdout, so they are never affected by the option. +* `--help` is always written to stdout, so that `msstore --help | more` works, and command line parse errors are always written to stderr, because they accompany a non-zero exit code. + +### Azure DevOps + +Azure DevOps reports every stderr line as `##[error]`, even when the command succeeded and even when the task sets `failOnStderr: false`. A successful `msstore publish` therefore shows up as a failed or partially failed stage. + +To avoid this, move the human-readable output to stdout: + +```yaml +- script: msstore publish ./MyApp --output-stream stdout + displayName: Publish to the Microsoft Store +``` + +Or set it once for a whole job, so that every `msstore` call picks it up: + +```yaml +variables: + MSSTORE_OUTPUT_STREAM: stdout +``` + +> [!IMPORTANT] +> Machine-readable payloads always go to stdout. When `MSSTORE_OUTPUT_STREAM` is set for a whole job, the human-readable output is interleaved with the payload, which breaks capturing it. Pass `--output-stream stderr` on those specific calls to opt back out — the option always overrides the environment variable: +> +> ```yaml +> variables: +> MSSTORE_OUTPUT_STREAM: stdout +> +> steps: +> - script: msstore publish ./MyApp # human-readable output on stdout +> - script: msstore submission get $(AppId) --output-stream stderr # clean JSON on stdout +> ``` + +### GitHub Actions + +No change is needed. GitHub Actions fails a step based on its exit code alone and never turns stderr into an error annotation, so the default is already correct. + ## Contributing This project welcomes contributions and suggestions. Most contributions require you to agree to a