Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test for System.Diagnostics.Process.Responding #46142

Merged
merged 9 commits into from
Dec 21, 2020
Merged

Add test for System.Diagnostics.Process.Responding #46142

Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ff3484d
don't ignore MainWindowTitle_GetWithGui_ShouldRefresh_Windows for the CI
adamsitnik Dec 16, 2020
cc22bbc
implement a simple test for Process.Responding
adamsitnik Dec 16, 2020
eaae6c1
move kill to the finally block
adamsitnik Dec 16, 2020
929db30
Update src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs
adamsitnik Dec 17, 2020
35c9d39
ensure that MainWindowHandle_GetWithGui_ShouldRefresh_Windows and Mai…
adamsitnik Dec 18, 2020
b9e7a15
give a clear error when Notepad is missing
adamsitnik Dec 18, 2020
e4dc30a
make sure RespondingIsRefreshedAfterEveryCallToRefresh passes on Nano
adamsitnik Dec 18, 2020
c43c550
apply code review suggestion and use reflection to test all refreshab…
adamsitnik Dec 21, 2020
74c97a0
be explicit about field names
adamsitnik Dec 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 45 additions & 3 deletions src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand All @@ -1627,6 +1629,46 @@ public void MainWindowTitle_GetWithGui_ShouldRefresh_Windows()
}
}

[PlatformSpecific(TestPlatforms.Windows)] // it tests Windows implementation
[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
public void RespondingIsRefreshedAfterEveryCallToRefresh()
adamsitnik marked this conversation as resolved.
Show resolved Hide resolved
{
// 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();

try
{
Assert.False(GetHaveResponding(process));

Assert.True(process.Responding); // sets haveResponding to true
Assert.True(GetHaveResponding(process));

process.Refresh(); // sets haveResponding to false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given we're going down that route, wouldn't it make more sense to just make sure Refresh() invalidates all the right caches?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean testing other fields as well? It's a good idea, I'll do that

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, and perhaps renaming the test to something like Refresh_Invalidates_All_Caches (or something like that).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eiriktsarpalis I've decided to give up on this idea for two reasons:

Assert.False(GetHaveResponding(process));

Assert.True(process.Responding); // sets haveResponding to true
Assert.True(GetHaveResponding(process));
}
finally
{
if (!process.HasExited)
{
process.Kill();
}

Assert.True(process.WaitForExit(WaitInMS));
}
}

static bool GetHaveResponding(Process process)=> (bool)typeof(Process)
adamsitnik marked this conversation as resolved.
Show resolved Hide resolved
.GetField("_haveResponding", Reflection.BindingFlags.NonPublic | Reflection.BindingFlags.Instance)
.GetValue(process);
}

[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
public void MainWindowTitle_NoWindow_ReturnsEmpty()
{
Expand Down