Skip to content

Conversation

rafaelfranca
Copy link
Contributor

@@ -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
Copy link
Member

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.

Copy link
Contributor

@aardvark179 aardvark179 left a 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?

@rafaelfranca
Copy link
Contributor Author

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
Copy link
Member

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

Copy link
Member

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 :)

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Member

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
Copy link
Member

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

Copy link
Member

@eregon eregon left a 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!

@eregon eregon added this to the 20.0.0 milestone Nov 7, 2019
@eregon eregon added the in-ci The PR is being tested in CI. Do not push new commits. label Nov 7, 2019
graalvmbot pushed a commit that referenced this pull request Nov 8, 2019
@graalvmbot graalvmbot merged commit 925d3e0 into oracle:master Nov 8, 2019
@chrisseaton chrisseaton deleted the rm-raw-status-in-to-i branch December 3, 2019 20:48
@chrisseaton chrisseaton added the shopify Pull requests from Shopify label Feb 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-ci The PR is being tested in CI. Do not push new commits. oca-signed shopify Pull requests from Shopify
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants