From ff3484dec7b6c2cb040ef8265afa195c0e8a009a Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Wed, 16 Dec 2020 15:20:59 +0100 Subject: [PATCH 1/9] don't ignore MainWindowTitle_GetWithGui_ShouldRefresh_Windows for the CI --- .../System.Diagnostics.Process/tests/ProcessTests.cs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs index f296ebc6e42ad7..7b956b6bc481c7 100644 --- a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs +++ b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs @@ -1592,15 +1592,17 @@ public void MainWindowHandle_GetWithGui_ShouldRefresh_Windows() } [Fact] - [OuterLoop] - [Trait(XunitConstants.Category, XunitConstants.IgnoreForCI)] // Pops UI + [OuterLoop("Launches notepad")] [PlatformSpecific(TestPlatforms.Windows)] public void MainWindowTitle_GetWithGui_ShouldRefresh_Windows() { const string ExePath = "notepad.exe"; Assert.True(IsProgramInstalled(ExePath)); - using (Process process = Process.Start(ExePath)) + using (Process process = Process.Start(new ProcessStartInfo(ExePath) + { + WindowStyle = ProcessWindowStyle.Minimized + })) { try { From cc22bbc041e06101cf27ed696d23c1caa40919db Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Wed, 16 Dec 2020 16:10:25 +0100 Subject: [PATCH 2/9] implement a simple test for Process.Responding --- .../tests/ProcessTests.cs | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs index 7b956b6bc481c7..d0cea044bf7e1e 100644 --- a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs +++ b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs @@ -1629,6 +1629,42 @@ public void MainWindowTitle_GetWithGui_ShouldRefresh_Windows() } } + [PlatformSpecific(TestPlatforms.Windows)] // it tests Windows implementation + [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] + public void RespondingIsRefreshedAfterEveryCallToRefresh() + { + // testing Process.Responding using a real unresponsive process would be very hard to do properly + // instead of this, we just test the implementation to ensure that #36768 is not coming back + + using (Process process = CreateProcess()) + { + process.Start(); + + Assert.False(GetHaveResponding(process)); + + Assert.True(process.Responding); // sets haveResponding to true + Assert.True(GetHaveResponding(process)); + + process.Refresh(); // sets haveResponding to false + Assert.False(GetHaveResponding(process)); + + Assert.True(process.Responding); // sets haveResponding to true + Assert.True(GetHaveResponding(process)); + + + if (!process.HasExited) + { + process.Kill(); + } + + Assert.True(process.WaitForExit(WaitInMS)); + } + + static bool GetHaveResponding(Process process)=> (bool)typeof(Process) + .GetField("_haveResponding", Reflection.BindingFlags.NonPublic | Reflection.BindingFlags.Instance) + .GetValue(process); + } + [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] public void MainWindowTitle_NoWindow_ReturnsEmpty() { From eaae6c1263381d33b4d00bde8cd6b9b50b0ff80e Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Wed, 16 Dec 2020 16:19:03 +0100 Subject: [PATCH 3/9] move kill to the finally block --- .../tests/ProcessTests.cs | 30 +++++++++++-------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs index d0cea044bf7e1e..e3d0986be11ead 100644 --- a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs +++ b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs @@ -1640,24 +1640,28 @@ public void RespondingIsRefreshedAfterEveryCallToRefresh() { process.Start(); - Assert.False(GetHaveResponding(process)); - - Assert.True(process.Responding); // sets haveResponding to true - Assert.True(GetHaveResponding(process)); - - process.Refresh(); // sets haveResponding to false - Assert.False(GetHaveResponding(process)); + try + { + Assert.False(GetHaveResponding(process)); - Assert.True(process.Responding); // sets haveResponding to true - Assert.True(GetHaveResponding(process)); + Assert.True(process.Responding); // sets haveResponding to true + Assert.True(GetHaveResponding(process)); + process.Refresh(); // sets haveResponding to false + Assert.False(GetHaveResponding(process)); - if (!process.HasExited) - { - process.Kill(); + Assert.True(process.Responding); // sets haveResponding to true + Assert.True(GetHaveResponding(process)); } + finally + { + if (!process.HasExited) + { + process.Kill(); + } - Assert.True(process.WaitForExit(WaitInMS)); + Assert.True(process.WaitForExit(WaitInMS)); + } } static bool GetHaveResponding(Process process)=> (bool)typeof(Process) From 929db30cadbcacecbddc5de46662f52ce04a786c Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 17 Dec 2020 16:40:11 +0100 Subject: [PATCH 4/9] Update src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs Co-authored-by: Eirik Tsarpalis --- src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs index e3d0986be11ead..89e66a6a8555ab 100644 --- a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs +++ b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs @@ -1664,7 +1664,7 @@ public void RespondingIsRefreshedAfterEveryCallToRefresh() } } - static bool GetHaveResponding(Process process)=> (bool)typeof(Process) + static bool GetHaveResponding(Process process) => (bool)typeof(Process) .GetField("_haveResponding", Reflection.BindingFlags.NonPublic | Reflection.BindingFlags.Instance) .GetValue(process); } From 35c9d39eb638eb6b4704ed9162c912bb76f95260 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 18 Dec 2020 09:25:56 +0100 Subject: [PATCH 5/9] ensure that MainWindowHandle_GetWithGui_ShouldRefresh_Windows and MainWindowTitle_GetWithGui_ShouldRefresh_Windows tests are not ignored for CI, but only excluded for Nano --- .../System.Diagnostics.Process/tests/ProcessTests.cs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs index 89e66a6a8555ab..259a6657db0f9e 100644 --- a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs +++ b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs @@ -1556,10 +1556,9 @@ public void MainWindowHandle_GetNotStarted_ThrowsInvalidOperationException() var process = new Process(); Assert.Throws(() => process.MainWindowHandle); } - - [Fact] - [OuterLoop] - [Trait(XunitConstants.Category, XunitConstants.IgnoreForCI)] // Pops UI + + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotWindowsNanoServer))] // it needs Notepad + [OuterLoop("Pops UI")] [PlatformSpecific(TestPlatforms.Windows)] public void MainWindowHandle_GetWithGui_ShouldRefresh_Windows() { @@ -1591,8 +1590,8 @@ public void MainWindowHandle_GetWithGui_ShouldRefresh_Windows() } } - [Fact] - [OuterLoop("Launches notepad")] + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotWindowsNanoServer))] // it needs Notepad + [OuterLoop("Pops UI")] [PlatformSpecific(TestPlatforms.Windows)] public void MainWindowTitle_GetWithGui_ShouldRefresh_Windows() { From b9e7a1556ddf6c6cd10332d57e998cc72ffa6a5f Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 18 Dec 2020 09:26:24 +0100 Subject: [PATCH 6/9] give a clear error when Notepad is missing --- .../System.Diagnostics.Process/tests/ProcessTests.cs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs index 259a6657db0f9e..e25d460672a80f 100644 --- a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs +++ b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs @@ -1563,7 +1563,7 @@ public void MainWindowHandle_GetNotStarted_ThrowsInvalidOperationException() public void MainWindowHandle_GetWithGui_ShouldRefresh_Windows() { const string ExePath = "notepad.exe"; - Assert.True(IsProgramInstalled(ExePath)); + Assert.True(IsProgramInstalled(ExePath), "Notepad is not installed"); using (Process process = Process.Start(ExePath)) { @@ -1596,12 +1596,9 @@ public void MainWindowHandle_GetWithGui_ShouldRefresh_Windows() public void MainWindowTitle_GetWithGui_ShouldRefresh_Windows() { const string ExePath = "notepad.exe"; - Assert.True(IsProgramInstalled(ExePath)); + Assert.True(IsProgramInstalled(ExePath), "Notepad is not installed"); - using (Process process = Process.Start(new ProcessStartInfo(ExePath) - { - WindowStyle = ProcessWindowStyle.Minimized - })) + using (Process process = Process.Start(new ProcessStartInfo(ExePath))) { try { From e4dc30a6609274e0ce7b3b6ed37e103a276303a0 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 18 Dec 2020 09:26:59 +0100 Subject: [PATCH 7/9] make sure RespondingIsRefreshedAfterEveryCallToRefresh passes on Nano --- .../tests/ProcessTests.cs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs index e25d460672a80f..d989fcffeeb626 100644 --- a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs +++ b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs @@ -1638,16 +1638,16 @@ public void RespondingIsRefreshedAfterEveryCallToRefresh() try { - Assert.False(GetHaveResponding(process)); - - Assert.True(process.Responding); // sets haveResponding to true - Assert.True(GetHaveResponding(process)); + for (int i = 0; i < 3; i++) + { + Assert.False(GetHaveResponding(process)); - process.Refresh(); // sets haveResponding to false - Assert.False(GetHaveResponding(process)); + Assert.True(process.Responding // sets haveResponding to true + || PlatformDetection.IsWindowsNanoServer); // underlying WinAPI returns false on Nano + Assert.True(GetHaveResponding(process)); - Assert.True(process.Responding); // sets haveResponding to true - Assert.True(GetHaveResponding(process)); + process.Refresh(); // sets haveResponding to false + } } finally { From c43c550360c381fd9f5d55ddfe75ff905bcc0048 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 21 Dec 2020 12:47:38 +0100 Subject: [PATCH 8/9] apply code review suggestion and use reflection to test all refreshable private fields --- .../tests/ProcessTests.cs | 84 ++++++++++++++----- 1 file changed, 61 insertions(+), 23 deletions(-) diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs index d989fcffeeb626..da1f28275e15de 100644 --- a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs +++ b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs @@ -8,6 +8,7 @@ using System.IO.Pipes; using System.Linq; using System.Net; +using System.Reflection; using System.Security; using System.Text; using System.Threading; @@ -1625,44 +1626,81 @@ public void MainWindowTitle_GetWithGui_ShouldRefresh_Windows() } } - [PlatformSpecific(TestPlatforms.Windows)] // it tests Windows implementation - [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] - public void RespondingIsRefreshedAfterEveryCallToRefresh() + [Fact] + public void RefreshResetsAllRefreshableFields() { // testing Process.Responding using a real unresponsive process would be very hard to do properly // instead of this, we just test the implementation to ensure that #36768 is not coming back - using (Process process = CreateProcess()) - { - process.Start(); + Assert.NotEmpty(GetRefreshableBooleanFields()); - try + var process = new Process(); + + VerifyPrivateFieldsValues(process, shouldHaveDefaultValues: true); + + SetPrivateFieldsToNonDefaultValues(process); + + VerifyPrivateFieldsValues(process, shouldHaveDefaultValues: false); + + process.Refresh(); + + VerifyPrivateFieldsValues(process, shouldHaveDefaultValues: true); + + static void VerifyPrivateFieldsValues(Process process, bool shouldHaveDefaultValues) + { + foreach (var booleanField in GetRefreshableBooleanFields()) { - for (int i = 0; i < 3; i++) + if (shouldHaveDefaultValues) + { + Assert.False((bool)booleanField.GetValue(process), $"{booleanField.Name} had unexpected value"); + } + else { - Assert.False(GetHaveResponding(process)); + Assert.True((bool)booleanField.GetValue(process), $"{booleanField.Name} had unexpected value"); + } + + } - Assert.True(process.Responding // sets haveResponding to true - || PlatformDetection.IsWindowsNanoServer); // underlying WinAPI returns false on Nano - Assert.True(GetHaveResponding(process)); + Assert.Equal(shouldHaveDefaultValues, null == GetPrivateFieldValue(process, "_processInfo")); + Assert.Equal(shouldHaveDefaultValues, null == GetPrivateFieldValue(process, "_threads")); + Assert.Equal(shouldHaveDefaultValues, null == GetPrivateFieldValue(process, "_modules")); - process.Refresh(); // sets haveResponding to false - } + if (OperatingSystem.IsWindows()) + { + Assert.Equal(shouldHaveDefaultValues, null == GetPrivateFieldValue(process, "_mainWindowTitle")); } - finally + } + + static void SetPrivateFieldsToNonDefaultValues(Process process) + { + foreach (var booleanField in GetRefreshableBooleanFields()) { - if (!process.HasExited) - { - process.Kill(); - } + booleanField.SetValue(process, true); + } - Assert.True(process.WaitForExit(WaitInMS)); + SetPrivateFieldValue(process, "_processInfo", typeof(Process).Assembly.GetType("System.Diagnostics.ProcessInfo").GetConstructor(BindingFlags.NonPublic | BindingFlags.Instance, Array.Empty()).Invoke(null)); + SetPrivateFieldValue(process, "_threads", new ProcessThreadCollection(Array.Empty())); + SetPrivateFieldValue(process, "_modules", new ProcessModuleCollection(Array.Empty())); + + if (OperatingSystem.IsWindows()) + { + SetPrivateFieldValue(process, "_mainWindowTitle", "notNull"); } } - static bool GetHaveResponding(Process process) => (bool)typeof(Process) - .GetField("_haveResponding", Reflection.BindingFlags.NonPublic | Reflection.BindingFlags.Instance) - .GetValue(process); + static FieldInfo[] GetRefreshableBooleanFields() => typeof(Process) + .GetFields(BindingFlags.NonPublic | BindingFlags.Instance) + .Where(field => field.Name.StartsWith("_have") && field.FieldType == typeof(bool) + && field.Name != "_haveProcessId" && field.Name != "_haveProcessHandle") // these fields should not be reset by Refresh) + .ToArray(); + + static object GetPrivateFieldValue(Process process, string fieldName) => typeof(Process) + .GetField(fieldName, BindingFlags.NonPublic | BindingFlags.Instance) + .GetValue(process); + + static void SetPrivateFieldValue(Process process, string fieldName, object value) => typeof(Process) + .GetField(fieldName, BindingFlags.NonPublic | BindingFlags.Instance) + .SetValue(process, value); } [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] From 74c97a049e4856cac2423ef9543216c61c445c3c Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 21 Dec 2020 14:03:44 +0100 Subject: [PATCH 9/9] be explicit about field names --- .../tests/ProcessTests.cs | 43 ++++++++----------- 1 file changed, 18 insertions(+), 25 deletions(-) diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs index da1f28275e15de..c38fde15d3013a 100644 --- a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs +++ b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs @@ -1631,9 +1631,6 @@ public void RefreshResetsAllRefreshableFields() { // testing Process.Responding using a real unresponsive process would be very hard to do properly // instead of this, we just test the implementation to ensure that #36768 is not coming back - - Assert.NotEmpty(GetRefreshableBooleanFields()); - var process = new Process(); VerifyPrivateFieldsValues(process, shouldHaveDefaultValues: true); @@ -1648,18 +1645,12 @@ public void RefreshResetsAllRefreshableFields() static void VerifyPrivateFieldsValues(Process process, bool shouldHaveDefaultValues) { - foreach (var booleanField in GetRefreshableBooleanFields()) - { - if (shouldHaveDefaultValues) - { - Assert.False((bool)booleanField.GetValue(process), $"{booleanField.Name} had unexpected value"); - } - else - { - Assert.True((bool)booleanField.GetValue(process), $"{booleanField.Name} had unexpected value"); - } - - } + Assert.Equal(shouldHaveDefaultValues, !(bool)GetPrivateFieldValue(process, "_exited")); + Assert.Equal(shouldHaveDefaultValues, !(bool)GetPrivateFieldValue(process, "_haveWorkingSetLimits")); + Assert.Equal(shouldHaveDefaultValues, !(bool)GetPrivateFieldValue(process, "_haveProcessorAffinity")); + Assert.Equal(shouldHaveDefaultValues, !(bool)GetPrivateFieldValue(process, "_havePriorityClass")); + Assert.Equal(shouldHaveDefaultValues, !(bool)GetPrivateFieldValue(process, "_haveExitTime")); + Assert.Equal(shouldHaveDefaultValues, !(bool)GetPrivateFieldValue(process, "_havePriorityBoostEnabled")); Assert.Equal(shouldHaveDefaultValues, null == GetPrivateFieldValue(process, "_processInfo")); Assert.Equal(shouldHaveDefaultValues, null == GetPrivateFieldValue(process, "_threads")); @@ -1668,15 +1659,20 @@ static void VerifyPrivateFieldsValues(Process process, bool shouldHaveDefaultVal if (OperatingSystem.IsWindows()) { Assert.Equal(shouldHaveDefaultValues, null == GetPrivateFieldValue(process, "_mainWindowTitle")); + Assert.Equal(shouldHaveDefaultValues, !(bool)GetPrivateFieldValue(process, "_signaled")); + Assert.Equal(shouldHaveDefaultValues, !(bool)GetPrivateFieldValue(process, "_haveMainWindow")); + Assert.Equal(shouldHaveDefaultValues, !(bool)GetPrivateFieldValue(process, "_haveResponding")); } } static void SetPrivateFieldsToNonDefaultValues(Process process) { - foreach (var booleanField in GetRefreshableBooleanFields()) - { - booleanField.SetValue(process, true); - } + SetPrivateFieldValue(process, "_exited", true); + SetPrivateFieldValue(process, "_haveWorkingSetLimits", true); + SetPrivateFieldValue(process, "_haveProcessorAffinity", true); + SetPrivateFieldValue(process, "_havePriorityClass", true); + SetPrivateFieldValue(process, "_haveExitTime", true); + SetPrivateFieldValue(process, "_havePriorityBoostEnabled", true); SetPrivateFieldValue(process, "_processInfo", typeof(Process).Assembly.GetType("System.Diagnostics.ProcessInfo").GetConstructor(BindingFlags.NonPublic | BindingFlags.Instance, Array.Empty()).Invoke(null)); SetPrivateFieldValue(process, "_threads", new ProcessThreadCollection(Array.Empty())); @@ -1684,16 +1680,13 @@ static void SetPrivateFieldsToNonDefaultValues(Process process) if (OperatingSystem.IsWindows()) { + SetPrivateFieldValue(process, "_signaled", true); + SetPrivateFieldValue(process, "_haveMainWindow", true); SetPrivateFieldValue(process, "_mainWindowTitle", "notNull"); + SetPrivateFieldValue(process, "_haveResponding", true); } } - static FieldInfo[] GetRefreshableBooleanFields() => typeof(Process) - .GetFields(BindingFlags.NonPublic | BindingFlags.Instance) - .Where(field => field.Name.StartsWith("_have") && field.FieldType == typeof(bool) - && field.Name != "_haveProcessId" && field.Name != "_haveProcessHandle") // these fields should not be reset by Refresh) - .ToArray(); - static object GetPrivateFieldValue(Process process, string fieldName) => typeof(Process) .GetField(fieldName, BindingFlags.NonPublic | BindingFlags.Instance) .GetValue(process);