-
Notifications
You must be signed in to change notification settings - Fork 137
Improve test process' kill signalling #925
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
Improve test process' kill signalling #925
Conversation
The reason for storing the symbol is to use in console tests such as debug/test/support/console_test_case.rb Line 210 in c3bfcc2
But it's ok to check each killing method(kill_remote_debuggee and kill_safely). Do you think that it'll be better? |
Yeah definitely. Since we always check the state right after calling kill methods, using return value will be more readable and make refactoring easier. |
60f787b
to
971d8c0
Compare
Yes, that's what I said here: We can write fixed error messages by checking the state in all killing methods. |
All right, Let's go with that idea. |
I'll review the changes tomorrow. Thank you for the explanation. |
Sorry I misunderstood 😅
Thanks! |
How about renaming |
1. `remote_info.failed_process` stores symbol but we only use the value's presence to check if the process is force-killed. 2. The force-killed status is directly controlled by `kill_safely` through `kill_remote_debuggee`, which is directly called right before we check the status with `remote_info.failed_process`. Combining the two, we can just let `kill_safely` and `kill_remote_debuggee` to return the force-killed status and not storing it in `remote_info`, which already contains a bunch of information. This also eliminates the need to pass `test_info` to `kill_safely`, which makes related code easier to understand and maintain.
971d8c0
to
64732f6
Compare
Yeah |
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.
Great! Thank you 👍
remote_info.failed_process
stores symbol but we only use the value's presence to check if the process is force-killed.kill_safely
throughkill_remote_debuggee
, which is directly called right before we check the status withremote_info.failed_process
.Combining the two, we can just let
kill_safely
andkill_remote_debuggee
return the force-killed status and not storing it inremote_info
, which already contains a bunch of information.This also eliminates the need to pass
test_info
tokill_safely
, which makes related code easier to understand and maintain.