-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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.
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
Conversation
Tagging subscribers to this area: @eiriktsarpalis Issue Details@eiriktsarpalis testing I think that we just test the implementation to ensure that #36768 is not coming back My motivation is that the implementation just checks if a boolean flag is set and calls WinAPI: Fixes #40703
|
/azp run runtime-libraries-coreclr outerloop-windows |
Azure Pipelines successfully started running 1 pipeline(s). |
Assert.True(process.Responding); // sets haveResponding to true | ||
Assert.True(GetHaveResponding(process)); | ||
|
||
process.Refresh(); // sets haveResponding to false |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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:
- the other two properties have existing tests:
public void MainWindowHandle_GetWithGui_ShouldRefresh_Windows() public void MainWindowTitle_GetWithGui_ShouldRefresh_Windows() - the other two properties start returning values after the main windows gets loaded, to test them using the private fields I would have to basically copy-paste the code from the other existing tests that waits until the window gets loaded
Co-authored-by: Eirik Tsarpalis <eirik.tsarpalis@gmail.com>
/azp run runtime-libraries-coreclr outerloop-windows |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-libraries-coreclr outerloop-windows |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-libraries-coreclr outerloop-windows |
Azure Pipelines successfully started running 1 pipeline(s). |
…nWindowTitle_GetWithGui_ShouldRefresh_Windows tests are not ignored for CI, but only excluded for Nano
/azp run runtime-libraries-coreclr outerloop-windows |
Azure Pipelines successfully started running 1 pipeline(s). |
…le private fields
@eiriktsarpalis testing
Process.Responding
using a real unresponsive process would be very hard (or even close to impossible) to do properlyI think that we just test the implementation to ensure that #36768 is not coming back
My motivation is that the implementation just checks if a boolean flag is set and calls WinAPI:
https://github.com/JeroenOortwijn/runtime/blob/c4d3ed52ed8c3763ab69dbc53376eca77e02abd4/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Win32.cs#L275-L302
Fixes #40703