Handle correctly waiting for process exit on Unix systems #3410
Conversation
The default failure 'System.ComponentModel.Win32Exception: No such file or directory' is not really helpful to understand what's going-on.
| @@ -599,6 +599,11 @@ protected static void ExecuteApplication(string path, string args, out string st | |||
| throw new ArgumentException("Executable path must not be null or whitespace.", nameof(path)); | |||
| } | |||
|
|
|||
| if (!File.Exists(path)) | |||
There was a problem hiding this comment.
Unrelated to this PR, but when trying to debug I noticed that without this check we end up with error like System.ComponentModel.Win32Exception: No such file or directory which isn't really helpful.
| namespace Microsoft.TestPlatform.AcceptanceTests; | ||
|
|
||
| [TestClass] | ||
| public class ProcessTests : AcceptanceTestBase |
There was a problem hiding this comment.
I couldn't really find in which existing type this would fit best so I created this one, feel free to suggest alternative.
There was a problem hiding this comment.
ProcessesInteractionTests?
|
|
||
| [TestClass] | ||
| public class ProcessTests : AcceptanceTestBase | ||
| { |
There was a problem hiding this comment.
I think that it would be nice to add one more test where we have a specific test adapter (not sure if that's the right "level") that would be hanging so we can prove that the cancellation are correctly done.
If you share the feeling, I'd be happy to be helped to add it.
| } | ||
| catch (Exception ex) when (ex is InvalidOperationException or TaskCanceledException) | ||
| { | ||
| } |
There was a problem hiding this comment.
can be useful a log here?
There was a problem hiding this comment.
Maybe yes. I will have two separate logs depending on the exception type.
There was a problem hiding this comment.
Actually, I can't because this project references no other project (so no access to EqtTrace). The signature is public so I can't even take some extra parameter.
| var runtimeConfigJson = Path.Combine(Path.GetDirectoryName(assemblyPath), "SimpleTestProjectMessedUpTargetFramework.runtimeconfig.json"); | ||
| var fileContent = File.ReadAllText(runtimeConfigJson); | ||
| var updatedContent = fileContent.Replace("\"version\": \"2.1.0\"", "\"version\": \"42\""); | ||
| File.WriteAllText(runtimeConfigJson, updatedContent); |
There was a problem hiding this comment.
q: this test is modifying the assets right?
There was a problem hiding this comment.
Yes that's why I created a new test asset. I originally thought that we were doing a local copy of the asset for each test but we don't so I wasn't sure how to best do this.
Given the question, I should probably put a comment in the code to explain what/why.
Thanks for pointing it out!
src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs
Show resolved
Hide resolved
| // flush its output and error streams).See https://github.com/microsoft/vstest/issues/3375 | ||
| var runtimeConfigJson = Path.Combine(Path.GetDirectoryName(assemblyPath), testAssetProjectName + ".runtimeconfig.json"); | ||
| var fileContent = File.ReadAllText(runtimeConfigJson); | ||
| var updatedContent = fileContent.Replace("\"version\": \"2.1.0\"", "\"version\": \"42\""); |
There was a problem hiding this comment.
maybe use some version that is lower than when is current, or that we know was not published, I think 2.8.0 was my original proposal? Or 0.0.0?
There was a problem hiding this comment.
I went with 0.0.0, it seems easier to notice this is invalid version.
| { | ||
| [TestMethod] | ||
| [NetCoreTargetFrameworkDataSource] | ||
| public void MissingFrameworkTextIsCorrectlyDisplayed(RunnerInfo runnerInfo) |
There was a problem hiding this comment.
| public void MissingFrameworkTextIsCorrectlyDisplayed(RunnerInfo runnerInfo) | |
| public void WhenTestHostProcessExitsBecauseTheTargetedRuntimeIsNoFoundThenTheMessageIsCapturedFromTheErrorOutput(RunnerInfo runnerInfo) |
Might be also worth adding some more info about the problem we were solving.
Description
Handle correctly waiting for process exit on Unix systems.
Related issue
Fixes #3375