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.

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

spec/ruby: Bring in some changes from upstream #5140

Closed
wants to merge 1 commit into from

Conversation

perlun
Copy link
Contributor

@perlun perlun commented Apr 13, 2018

This is a followup to #5134 which brings in the changes I did in ruby/spec#596 into our tree also. They highlight a current issue in JRuby that should be fixed at one point or another.

(There were other, mostly BigDecimal-related changes coming from there also, but I picked only the relevant changes from the spec tree for now. I don't know what the official process is for updating these; I looked for a Rake task but couldn't find any.)


Here are the results from these tests, illustrating the problem & current, incorrect behavior in JRuby. These specs pass on MRI.

$ ~/git/3rd-party/mspec/bin/mspec core/process/status
$ ruby /Users/plundberg/git/3rd-party/mspec/bin/mspec-run core/process/status
jruby 9.2.0.0-SNAPSHOT (2.5.0) 2018-04-12 34bec6e Java HotSpot(TM) 64-Bit Server VM 10+46 on 10+46 +jit [darwin-x86_64]

1)
Process::Status#exitstatus for a child that raised SignalException returns nil FAILED
Expected 143
 to equal nil

/Volumes/git/3rd-party/jruby/spec/ruby/core/process/status/exitstatus_spec.rb:21:in `block in (root)'
org/jruby/RubyBasicObject.java:2596:in `instance_eval'
org/jruby/RubyEnumerable.java:1737:in `all?'
org/jruby/RubyArray.java:1778:in `each'
org/jruby/RubyArray.java:1778:in `each'
/Volumes/git/3rd-party/jruby/spec/ruby/core/process/status/exitstatus_spec.rb:3:in `<main>'
org/jruby/RubyKernel.java:990:in `load'
org/jruby/RubyBasicObject.java:2596:in `instance_eval'
org/jruby/RubyArray.java:1778:in `each'

2)
Process::Status#termsig for a child that raised SignalException returns the signal FAILED
Expected nil
 to equal 15

/Volumes/git/3rd-party/jruby/spec/ruby/core/process/status/termsig_spec.rb:23:in `block in (root)'
org/jruby/RubyBasicObject.java:2596:in `instance_eval'
org/jruby/RubyEnumerable.java:1737:in `all?'
org/jruby/RubyArray.java:1778:in `each'
org/jruby/RubyArray.java:1778:in `each'
/Volumes/git/3rd-party/jruby/spec/ruby/core/process/status/termsig_spec.rb:3:in `<main>'
org/jruby/RubyKernel.java:990:in `load'
org/jruby/RubyBasicObject.java:2596:in `instance_eval'
org/jruby/RubyArray.java:1778:in `each'
[- | ==================100%================== | 00:00:00]      2F      0E

Finished in 43.987804 seconds

16 files, 13 examples, 13 expectations, 2 failures, 0 errors, 0 taggede, 3 examples, 3 expectations, 1 failure, 0 errors, 0 tagged

@kares
Copy link
Member

kares commented Apr 13, 2018

yep as you figured specs eventually end up here as they get merged, just check history :
fb1001f + a9b4d28
... now CI is unfortunately, once again, in a very 🔴 shape so maybe its best to tag new failures at this point (so that its less confusing for the person doing the cleanup to worry about new failures as well) ?

@headius
Copy link
Member

headius commented Apr 16, 2018

Sorry about some confusion on master about failing tests. I made a change for 9.1 that caused a number of suites to start terminating early with a successful return code, so it looked like things were suddenly green.

We will follow normal process for updating specs, so we get the appropriate logs. @perlun If you like you could rebase off current master and force push, so we'd get an accounting of new failures.

@headius headius added this to the JRuby 9.2.0.0 milestone Apr 16, 2018
This brings in the changes I did in ruby/spec#596 into our tree also. They highlight a current issue in JRuby that should be fixed at one point or another.

(There were other, mostly BigDecimal-related changes also, but I picked _only_ the relevant changes from the spec tree. I don't know what the official process is for updating these; I looked for a Rake task but couldn't find any.)
@perlun perlun force-pushed the spec/update-with-sigterm-specs branch from 64b49ba to 6bdfd05 Compare April 17, 2018 05:02
@perlun
Copy link
Contributor Author

perlun commented Apr 17, 2018

@headius Thank you for that. Rebased & force-pushed, we should be start seeing the failures in a short while.

@perlun
Copy link
Contributor Author

perlun commented Apr 18, 2018

Interestingly enough, I can't see the expected test failures now (but it fails on a random bunch of other tests. 😉) I'll try to update my jruby-head in the near future to re-test this locally with the latest master code.

@eregon
Copy link
Member

eregon commented Apr 18, 2018

To summarize, for specs fixing bugs in JRuby, the recommended process is adding the spec directly in JRuby's spec/ruby, which is synchronized ~once a month with ruby/spec and other copies of the specs.
This has the advantage that the fix in JRuby can be immediately tested with the new added spec :)

If there is any question regarding new specs, feel free to ping me.

@enebo @headius Maybe I should add a spec/README similar to what's in MRI?
https://github.com/ruby/ruby/tree/trunk/spec

@perlun
Copy link
Contributor Author

perlun commented Apr 18, 2018

To summarize, for specs fixing bugs in JRuby

Alright. In this case, some of it was fixed in JRuby but some of it is still unfixed. I was under the impression that the CI runs of this PR should highlight the latter, but I couldn't find it in the output (there are unfortunately a few other CI errors at the moment.)

@perlun
Copy link
Contributor Author

perlun commented Apr 19, 2018

I tried rebuilding my local JRuby and can verify that the specs still fail with the latest master version (well, 1cc0674 which is probably not latest anymore but still)

$ ../mspec/bin/mspec spec/ruby/core/process/status/
$ /Volumes/git/3rd-party/jruby/bin/jruby /Volumes/git/3rd-party/mspec/bin/mspec-run spec/ruby/core/process/status/
jruby 9.2.0.0-SNAPSHOT (2.5.0) 2018-04-19 6bdfd05 Java HotSpot(TM) 64-Bit Server VM 10+46 on 10+46 +jit [darwin-x86_64]

1)
Process::Status#exitstatus for a child that raised SignalException returns nil FAILED
Expected 143
 to equal nil

/Volumes/git/3rd-party/jruby/spec/ruby/core/process/status/exitstatus_spec.rb:21:in `block in (root)'
org/jruby/RubyBasicObject.java:2596:in `instance_eval'
org/jruby/RubyEnumerable.java:1741:in `all?'
org/jruby/RubyArray.java:1791:in `each'
org/jruby/RubyArray.java:1791:in `each'
/Volumes/git/3rd-party/jruby/spec/ruby/core/process/status/exitstatus_spec.rb:3:in `<main>'
org/jruby/RubyKernel.java:990:in `load'
org/jruby/RubyBasicObject.java:2596:in `instance_eval'
org/jruby/RubyArray.java:1791:in `each'

2)
Process::Status#termsig for a child that raised SignalException returns the signal FAILED
Expected nil
 to equal 15

/Volumes/git/3rd-party/jruby/spec/ruby/core/process/status/termsig_spec.rb:23:in `block in (root)'
org/jruby/RubyBasicObject.java:2596:in `instance_eval'
org/jruby/RubyEnumerable.java:1741:in `all?'
org/jruby/RubyArray.java:1791:in `each'
org/jruby/RubyArray.java:1791:in `each'
/Volumes/git/3rd-party/jruby/spec/ruby/core/process/status/termsig_spec.rb:3:in `<main>'
org/jruby/RubyKernel.java:990:in `load'
org/jruby/RubyBasicObject.java:2596:in `instance_eval'
org/jruby/RubyArray.java:1791:in `each'
[- | ==================100%================== | 00:00:00]      2F      0E

Finished in 29.725798 seconds

16 files, 13 examples, 13 expectations, 2 failures, 0 errors, 0 tagged

@perlun
Copy link
Contributor Author

perlun commented Apr 23, 2018

@kares @headius Are you fine with merging this for now? The change is already in ruby/spec as mentioned, so it will come our way sooner or later anyway.

@perlun
Copy link
Contributor Author

perlun commented Apr 25, 2018

Ping - are there any major problems with merging this?

@eregon
Copy link
Member

eregon commented Apr 25, 2018

Looks fine for me, since this is the exact same diff as in ruby/spec it should be easy to ignore it when doing the ruby/spec update (in a few days, I do it at the end of the month).

I'll let JRuby devs merge this if they want though.

@eregon
Copy link
Member

eregon commented Apr 28, 2018

I updated all specs in #5154.

I recommend to add specs directly in JRuby when working on a bug, so then the specs can be immediately run in JRuby's CI. The specs are anyway integrated in ruby/spec ~once a month, so other implementations can also benefit from them.

@eregon eregon closed this Apr 28, 2018
@kares kares modified the milestones: JRuby 9.2.0.0, Non-Release Apr 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants