From d3e8b473c1c92f83386f61c3a4fb2e53211f8b26 Mon Sep 17 00:00:00 2001 From: baradgur Date: Tue, 11 Apr 2023 21:03:33 +0600 Subject: [PATCH 1/5] SuggestionStore::GetCompletions to check exit code of invoked application Fix #2147 --- src/System.CommandLine.Suggest/SuggestionStore.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/System.CommandLine.Suggest/SuggestionStore.cs b/src/System.CommandLine.Suggest/SuggestionStore.cs index ac93972eb1..d624d66f88 100644 --- a/src/System.CommandLine.Suggest/SuggestionStore.cs +++ b/src/System.CommandLine.Suggest/SuggestionStore.cs @@ -44,7 +44,7 @@ public string GetCompletions(string exeFileName, string suggestionTargetArgument Task readToEndTask = process.StandardOutput.ReadToEndAsync(); - if (readToEndTask.Wait(timeout)) + if (readToEndTask.Wait(timeout) && process.HasExited && process.ExitCode == 0) { result = readToEndTask.Result; } From 1c23816992ea9ff8bbfe8764c15747d6e1f94cac Mon Sep 17 00:00:00 2001 From: baradgur Date: Thu, 13 Apr 2023 02:31:30 +0600 Subject: [PATCH 2/5] SuggestionStore::GetCompletions to check exit code of invoked application fixed Fix #2147 --- src/System.CommandLine.Suggest/SuggestionStore.cs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/System.CommandLine.Suggest/SuggestionStore.cs b/src/System.CommandLine.Suggest/SuggestionStore.cs index d624d66f88..b92246c8e1 100644 --- a/src/System.CommandLine.Suggest/SuggestionStore.cs +++ b/src/System.CommandLine.Suggest/SuggestionStore.cs @@ -42,11 +42,9 @@ public string GetCompletions(string exeFileName, string suggestionTargetArgument { process.Start(); - Task readToEndTask = process.StandardOutput.ReadToEndAsync(); - - if (readToEndTask.Wait(timeout) && process.HasExited && process.ExitCode == 0) + if (process.WaitForExit(timeout) && process.ExitCode == 0) { - result = readToEndTask.Result; + result = process.StandardOutput.ReadToEnd(); } else { From 65a92f50f6995912b11e45166aa4d1a15999febf Mon Sep 17 00:00:00 2001 From: baradgur Date: Thu, 13 Apr 2023 15:00:48 +0600 Subject: [PATCH 3/5] added tests to SuggestionStore::GetCompletions Fix #2147 --- .../DotnetSuggestEndToEndTests.cs | 142 ++++++++---------- .../SuggestionStoreTests.cs | 45 ++++++ .../TestsWithTestApps.cs | 74 +++++++++ .../WaitAndFailTestApp/Program.cs | 25 +++ .../WaitAndFailTestApp.csproj | 12 ++ .../dotnet-suggest.Tests.csproj | 11 ++ 6 files changed, 229 insertions(+), 80 deletions(-) create mode 100644 src/System.CommandLine.Suggest.Tests/SuggestionStoreTests.cs create mode 100644 src/System.CommandLine.Suggest.Tests/TestsWithTestApps.cs create mode 100644 src/System.CommandLine.Suggest.Tests/WaitAndFailTestApp/Program.cs create mode 100644 src/System.CommandLine.Suggest.Tests/WaitAndFailTestApp/WaitAndFailTestApp.csproj diff --git a/src/System.CommandLine.Suggest.Tests/DotnetSuggestEndToEndTests.cs b/src/System.CommandLine.Suggest.Tests/DotnetSuggestEndToEndTests.cs index 7e08fc8d65..27c4e9f672 100644 --- a/src/System.CommandLine.Suggest.Tests/DotnetSuggestEndToEndTests.cs +++ b/src/System.CommandLine.Suggest.Tests/DotnetSuggestEndToEndTests.cs @@ -3,8 +3,6 @@ using FluentAssertions; using System.CommandLine.Tests.Utility; -using System.IO; -using System.Linq; using System.Text; using Xunit.Abstractions; using static System.Environment; @@ -12,75 +10,24 @@ namespace System.CommandLine.Suggest.Tests { - public class DotnetSuggestEndToEndTests : IDisposable + public class DotnetSuggestEndToEndTests : TestsWithTestApps { - private readonly ITestOutputHelper _output; - private readonly FileInfo _endToEndTestApp; - private readonly FileInfo _dotnetSuggest; - private readonly (string, string)[] _environmentVariables; - private readonly DirectoryInfo _dotnetHostDir = DotnetMuxer.Path.Directory; - private static string _testRoot; - - public DotnetSuggestEndToEndTests(ITestOutputHelper output) + public DotnetSuggestEndToEndTests(ITestOutputHelper output) : base(output) { - _output = output; - - // delete sentinel files for EndToEndTestApp in order to trigger registration when it's run - var sentinelsDir = new DirectoryInfo(Path.Combine(Path.GetTempPath(), "system-commandline-sentinel-files")); - - if (sentinelsDir.Exists) - { - var sentinels = sentinelsDir.GetFiles("*EndToEndTestApp*"); - - foreach (var sentinel in sentinels) - { - sentinel.Delete(); - } - } - - var currentDirectory = Path.Combine( - Directory.GetCurrentDirectory(), - "TestAssets"); - - _endToEndTestApp = new DirectoryInfo(currentDirectory) - .GetFiles("EndToEndTestApp".ExecutableName()) - .SingleOrDefault(); - - _dotnetSuggest = new DirectoryInfo(currentDirectory) - .GetFiles("dotnet-suggest".ExecutableName()) - .SingleOrDefault(); - - PrepareTestHomeDirectoryToAvoidPolluteBuildMachineHome(); - - _environmentVariables = new[] { - ("DOTNET_ROOT", _dotnetHostDir.FullName), - ("INTERNAL_TEST_DOTNET_SUGGEST_HOME", _testRoot)}; - } - - public void Dispose() - { - if (_testRoot != null && Directory.Exists(_testRoot)) - { - Directory.Delete(_testRoot, recursive: true); - } - } - - private static void PrepareTestHomeDirectoryToAvoidPolluteBuildMachineHome() - { - _testRoot = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName()); - Directory.CreateDirectory(_testRoot); } [ReleaseBuildOnlyFact] public void Test_app_supplies_suggestions() { var stdOut = new StringBuilder(); - + + Output.WriteLine($"_endToEndTestApp.FullName: {EndToEndTestApp.FullName}"); + Process.RunToCompletion( - _endToEndTestApp.FullName, + EndToEndTestApp.FullName, "[suggest:1] \"a\"", stdOut: value => stdOut.AppendLine(value), - environmentVariables: _environmentVariables); + environmentVariables: EnvironmentVariables); stdOut.ToString() .Should() @@ -92,11 +39,11 @@ public void Dotnet_suggest_provides_suggestions_for_app() { // run "dotnet-suggest register" in explicit way Process.RunToCompletion( - _dotnetSuggest.FullName, - $"register --command-path \"{_endToEndTestApp.FullName}\"", - stdOut: s => _output.WriteLine(s), - stdErr: s => _output.WriteLine(s), - environmentVariables: _environmentVariables).Should().Be(0); + DotnetSuggest.FullName, + $"register --command-path \"{EndToEndTestApp.FullName}\"", + stdOut: s => Output.WriteLine(s), + stdErr: s => Output.WriteLine(s), + environmentVariables: EnvironmentVariables).Should().Be(0); var stdOut = new StringBuilder(); var stdErr = new StringBuilder(); @@ -104,14 +51,14 @@ public void Dotnet_suggest_provides_suggestions_for_app() var commandLineToComplete = "a"; Process.RunToCompletion( - _dotnetSuggest.FullName, - $"get -e \"{_endToEndTestApp.FullName}\" --position {commandLineToComplete.Length} -- \"{commandLineToComplete}\"", + DotnetSuggest.FullName, + $"get -e \"{EndToEndTestApp.FullName}\" --position {commandLineToComplete.Length} -- \"{commandLineToComplete}\"", stdOut: value => stdOut.AppendLine(value), stdErr: value => stdErr.AppendLine(value), - environmentVariables: _environmentVariables); + environmentVariables: EnvironmentVariables); - _output.WriteLine($"stdOut:{NewLine}{stdOut}{NewLine}"); - _output.WriteLine($"stdErr:{NewLine}{stdErr}{NewLine}"); + Output.WriteLine($"stdOut:{NewLine}{stdOut}{NewLine}"); + Output.WriteLine($"stdErr:{NewLine}{stdErr}{NewLine}"); stdErr.ToString() .Should() @@ -127,11 +74,11 @@ public void Dotnet_suggest_provides_suggestions_for_app_with_only_commandname() { // run "dotnet-suggest register" in explicit way Process.RunToCompletion( - _dotnetSuggest.FullName, - $"register --command-path \"{_endToEndTestApp.FullName}\"", - stdOut: s => _output.WriteLine(s), - stdErr: s => _output.WriteLine(s), - environmentVariables: _environmentVariables).Should().Be(0); + DotnetSuggest.FullName, + $"register --command-path \"{EndToEndTestApp.FullName}\"", + stdOut: s => Output.WriteLine(s), + stdErr: s => Output.WriteLine(s), + environmentVariables: EnvironmentVariables).Should().Be(0); var stdOut = new StringBuilder(); var stdErr = new StringBuilder(); @@ -139,14 +86,14 @@ public void Dotnet_suggest_provides_suggestions_for_app_with_only_commandname() var commandLineToComplete = "a "; Process.RunToCompletion( - _dotnetSuggest.FullName, - $"get -e \"{_endToEndTestApp.FullName}\" --position {commandLineToComplete.Length} -- \"{commandLineToComplete}\"", + DotnetSuggest.FullName, + $"get -e \"{EndToEndTestApp.FullName}\" --position {commandLineToComplete.Length} -- \"{commandLineToComplete}\"", stdOut: value => stdOut.AppendLine(value), stdErr: value => stdErr.AppendLine(value), - environmentVariables: _environmentVariables); + environmentVariables: EnvironmentVariables); - _output.WriteLine($"stdOut:{NewLine}{stdOut}{NewLine}"); - _output.WriteLine($"stdErr:{NewLine}{stdErr}{NewLine}"); + Output.WriteLine($"stdOut:{NewLine}{stdOut}{NewLine}"); + Output.WriteLine($"stdErr:{NewLine}{stdErr}{NewLine}"); stdErr.ToString() .Should() @@ -156,5 +103,40 @@ public void Dotnet_suggest_provides_suggestions_for_app_with_only_commandname() .Should() .Be($"--apple{NewLine}--banana{NewLine}--cherry{NewLine}--durian{NewLine}--help{NewLine}--version{NewLine}-?{NewLine}-h{NewLine}/?{NewLine}/h{NewLine}"); } + + [ReleaseBuildOnlyFact] + public void Dotnet_suggest_fails_to_provide_suggestions_because_app_faulted() + { + // run "dotnet-suggest register" in explicit way + Process.RunToCompletion( + DotnetSuggest.FullName, + $"register --command-path \"{WaitAndFailTestApp.FullName}\"", + stdOut: s => Output.WriteLine(s), + stdErr: s => Output.WriteLine(s), + environmentVariables: EnvironmentVariables).Should().Be(0); + + var stdOut = new StringBuilder(); + var stdErr = new StringBuilder(); + + var commandLineToComplete = "a"; + + Process.RunToCompletion( + DotnetSuggest.FullName, + $"get -e \"{WaitAndFailTestApp.FullName}\" --position {commandLineToComplete.Length} -- \"{commandLineToComplete}\"", + stdOut: value => stdOut.AppendLine(value), + stdErr: value => stdErr.AppendLine(value), + environmentVariables: EnvironmentVariables); + + Output.WriteLine($"stdOut:{NewLine}{stdOut}{NewLine}"); + Output.WriteLine($"stdErr:{NewLine}{stdErr}{NewLine}"); + + stdErr.ToString() + .Should() + .BeEmpty(); + + stdOut.ToString() + .Should() + .BeEmpty(); + } } } diff --git a/src/System.CommandLine.Suggest.Tests/SuggestionStoreTests.cs b/src/System.CommandLine.Suggest.Tests/SuggestionStoreTests.cs new file mode 100644 index 0000000000..ba3665bce2 --- /dev/null +++ b/src/System.CommandLine.Suggest.Tests/SuggestionStoreTests.cs @@ -0,0 +1,45 @@ +// Copyright (c) .NET Foundation and contributors. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using System.CommandLine.Tests.Utility; +using FluentAssertions; +using Xunit.Abstractions; +using static System.Environment; + +namespace System.CommandLine.Suggest.Tests +{ + public class SuggestionStoreTests : TestsWithTestApps + { + public SuggestionStoreTests(ITestOutputHelper output) : base(output) + { + } + + [ReleaseBuildOnlyFact] + public void GetCompletions_obtains_suggestions_successfully() + { + var store = new SuggestionStore(); + var completions = store.GetCompletions(EndToEndTestApp.FullName, "[suggest:1] \"a\"", TimeSpan.FromSeconds(1)); + completions.Should().Be($"--apple{NewLine}--banana{NewLine}--durian{NewLine}"); + } + + [ReleaseBuildOnlyFact] + public void GetCompletions_fails_to_obtain_suggestions_because_app_takes_too_long() + { + var store = new SuggestionStore(); + var appHangingTimeSpanArgument = TimeSpan.FromMilliseconds(2000).ToString(); + var completions = store + .GetCompletions(WaitAndFailTestApp.FullName, appHangingTimeSpanArgument, TimeSpan.FromMilliseconds(1000)); + completions.Should().BeEmpty(); + } + + [ReleaseBuildOnlyFact] + public void GetCompletions_fails_to_obtain_suggestions_because_app_exited_with_nonzero_code() + { + var store = new SuggestionStore(); + var appHangingTimeSpanArgument = TimeSpan.FromMilliseconds(0).ToString(); + var completions = store + .GetCompletions(WaitAndFailTestApp.FullName, appHangingTimeSpanArgument, TimeSpan.FromMilliseconds(1000)); + completions.Should().BeEmpty(); + } + } +} diff --git a/src/System.CommandLine.Suggest.Tests/TestsWithTestApps.cs b/src/System.CommandLine.Suggest.Tests/TestsWithTestApps.cs new file mode 100644 index 0000000000..485c9160a2 --- /dev/null +++ b/src/System.CommandLine.Suggest.Tests/TestsWithTestApps.cs @@ -0,0 +1,74 @@ +using System.IO; +using System.Linq; +using Xunit; +using Xunit.Abstractions; + +namespace System.CommandLine.Suggest.Tests; + +[Collection("TestsWithTestApps")] +public class TestsWithTestApps : IDisposable +{ + protected readonly ITestOutputHelper Output; + protected readonly FileInfo EndToEndTestApp; + protected readonly FileInfo WaitAndFailTestApp; + protected readonly FileInfo DotnetSuggest; + protected readonly (string, string)[] EnvironmentVariables; + private readonly DirectoryInfo _dotnetHostDir = DotnetMuxer.Path.Directory; + private static string _testRoot; + + protected TestsWithTestApps(ITestOutputHelper output) + { + Output = output; + + // delete sentinel files for TestApps in order to trigger registration when it's run + var sentinelsDir = new DirectoryInfo(Path.Combine(Path.GetTempPath(), "system-commandline-sentinel-files")); + + if (sentinelsDir.Exists) + { + var sentinels = sentinelsDir + .EnumerateFiles() + .Where(f => f.Name.Contains("EndToEndTestApp") || f.Name.Contains("WaitAndFailTestApp")); + + foreach (var sentinel in sentinels) + { + sentinel.Delete(); + } + } + + var currentDirectory = Path.Combine( + Directory.GetCurrentDirectory(), + "TestAssets"); + + EndToEndTestApp = new DirectoryInfo(currentDirectory) + .GetFiles("EndToEndTestApp".ExecutableName()) + .SingleOrDefault(); + + WaitAndFailTestApp = new DirectoryInfo(currentDirectory) + .GetFiles("WaitAndFailTestApp".ExecutableName()) + .SingleOrDefault(); + + DotnetSuggest = new DirectoryInfo(currentDirectory) + .GetFiles("dotnet-suggest".ExecutableName()) + .SingleOrDefault(); + + PrepareTestHomeDirectoryToAvoidPolluteBuildMachineHome(); + + EnvironmentVariables = new[] { + ("DOTNET_ROOT", _dotnetHostDir.FullName), + ("INTERNAL_TEST_DOTNET_SUGGEST_HOME", _testRoot)}; + } + + public void Dispose() + { + if (_testRoot != null && Directory.Exists(_testRoot)) + { + Directory.Delete(_testRoot, recursive: true); + } + } + + private static void PrepareTestHomeDirectoryToAvoidPolluteBuildMachineHome() + { + _testRoot = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName()); + Directory.CreateDirectory(_testRoot); + } +} \ No newline at end of file diff --git a/src/System.CommandLine.Suggest.Tests/WaitAndFailTestApp/Program.cs b/src/System.CommandLine.Suggest.Tests/WaitAndFailTestApp/Program.cs new file mode 100644 index 0000000000..d1d22b38ab --- /dev/null +++ b/src/System.CommandLine.Suggest.Tests/WaitAndFailTestApp/Program.cs @@ -0,0 +1,25 @@ +using System; +using System.Threading.Tasks; + +namespace WaitAndFailTestApp; + +public class Program +{ + private static TimeSpan defaultWait = TimeSpan.FromMilliseconds(3000); + + //we should not be able to receive any suggestion from this test app, + //so we are not constructing it using CliConfiguration + + static async Task Main(string[] args) + { + var waitPeriod = args.Length > 0 && int.TryParse(args[0], out var millisecondsToWaitParsed) + ? TimeSpan.FromMilliseconds(millisecondsToWaitParsed) + : defaultWait; + + await Task.Delay(waitPeriod); + Environment.ExitCode = 1; + + Console.WriteLine("this 'suggestion' is provided too late and/or with invalid app exit code"); + } +} + diff --git a/src/System.CommandLine.Suggest.Tests/WaitAndFailTestApp/WaitAndFailTestApp.csproj b/src/System.CommandLine.Suggest.Tests/WaitAndFailTestApp/WaitAndFailTestApp.csproj new file mode 100644 index 0000000000..186b884e98 --- /dev/null +++ b/src/System.CommandLine.Suggest.Tests/WaitAndFailTestApp/WaitAndFailTestApp.csproj @@ -0,0 +1,12 @@ + + + + + + + + Exe + net7.0 + + + diff --git a/src/System.CommandLine.Suggest.Tests/dotnet-suggest.Tests.csproj b/src/System.CommandLine.Suggest.Tests/dotnet-suggest.Tests.csproj index e22ead7273..fd18104470 100644 --- a/src/System.CommandLine.Suggest.Tests/dotnet-suggest.Tests.csproj +++ b/src/System.CommandLine.Suggest.Tests/dotnet-suggest.Tests.csproj @@ -13,6 +13,11 @@ + + + + + @@ -67,6 +72,12 @@ + + + + + + From d8adba76ba7dea8f6f69a462eeed968d4f15581d Mon Sep 17 00:00:00 2001 From: baradgur Date: Fri, 14 Apr 2023 18:17:37 +0600 Subject: [PATCH 4/5] use async read for standard output for SuggestionStore::GetCompletions Fix #2147 --- src/System.CommandLine.Suggest/SuggestionStore.cs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/System.CommandLine.Suggest/SuggestionStore.cs b/src/System.CommandLine.Suggest/SuggestionStore.cs index b92246c8e1..0a6c8b6649 100644 --- a/src/System.CommandLine.Suggest/SuggestionStore.cs +++ b/src/System.CommandLine.Suggest/SuggestionStore.cs @@ -4,6 +4,7 @@ using System.ComponentModel; using System.Diagnostics; using System.IO; +using System.Text; using System.Threading.Tasks; namespace System.CommandLine.Suggest @@ -41,10 +42,20 @@ public string GetCompletions(string exeFileName, string suggestionTargetArgument }) { process.Start(); - + process.BeginOutputReadLine(); + + var stringBuilder = new StringBuilder(); + process.OutputDataReceived += (sender, eventArgs) => + { + if (eventArgs.Data != null) + { + stringBuilder.AppendLine(eventArgs.Data); + } + }; + if (process.WaitForExit(timeout) && process.ExitCode == 0) { - result = process.StandardOutput.ReadToEnd(); + result = stringBuilder.ToString(); } else { From 8b8e3b78a8ebb771413f355cf854f6de381f8169 Mon Sep 17 00:00:00 2001 From: baradgur Date: Fri, 14 Apr 2023 20:04:15 +0600 Subject: [PATCH 5/5] stopping async read in SuggestionStore::GetCompletions gracefully Fix #2147 --- src/System.CommandLine.Suggest/SuggestionStore.cs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/System.CommandLine.Suggest/SuggestionStore.cs b/src/System.CommandLine.Suggest/SuggestionStore.cs index 0a6c8b6649..d77e547f5a 100644 --- a/src/System.CommandLine.Suggest/SuggestionStore.cs +++ b/src/System.CommandLine.Suggest/SuggestionStore.cs @@ -41,8 +41,8 @@ public string GetCompletions(string exeFileName, string suggestionTargetArgument StartInfo = processStartInfo }) { + process.Start(); - process.BeginOutputReadLine(); var stringBuilder = new StringBuilder(); process.OutputDataReceived += (sender, eventArgs) => @@ -52,13 +52,17 @@ public string GetCompletions(string exeFileName, string suggestionTargetArgument stringBuilder.AppendLine(eventArgs.Data); } }; - + process.BeginOutputReadLine(); if (process.WaitForExit(timeout) && process.ExitCode == 0) { + process.CancelOutputRead(); result = stringBuilder.ToString(); } else { +#if DEBUG + Program.LogDebug($"Killing the process after a timeout. Process exit code: {process.ExitCode}"); +#endif process.Kill(); } }