-
Notifications
You must be signed in to change notification settings - Fork 194
Process::Status#to_i: return raw waitpid(2) status #1800
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
Conversation
@@ -729,19 +731,20 @@ class Status | |||
attr_reader :termsig | |||
attr_reader :stopsig | |||
|
|||
def initialize(pid=nil, status=nil, termsig=nil, stopsig=nil) | |||
def initialize(pid=nil, status=nil, termsig=nil, stopsig=nil, raw_status=nil) | |||
@pid = pid | |||
@status = status |
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.
I think we should rename this to @exitstatus
to avoid confusion and mirror the C macro names.
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.
The CI system is showing a test failure in test/mri/tests/ruby/test_process.rb
. Could you try running that MRI test locally and investigate?
I run all the test in my machine and saw no failures with my last commit. |
end | ||
|
||
def coredump? | ||
false | ||
end | ||
|
||
def exited? | ||
@status != nil | ||
@raw_status != nil |
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.
This one should be @exitstatus
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.
I'll fix those two :)
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.
Thanks. We will need test coverage on those as well.
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.
Good point, actually there are already specs but one of the exited?
specs was tagged. That one fails with @raw_status
.
@raw_status
seems to work in success?
but I think it's just luck of the native encoding for exited.
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.
Together with #1790 we pass all Process::Status specs.
@@ -779,14 +782,14 @@ def stopped? | |||
|
|||
def success? | |||
if exited? | |||
@status == 0 | |||
@raw_status == 0 |
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.
This one should be @exitstatus
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.
Thanks for the fix!
PullRequest: truffleruby/1121
Shopify#1