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.

Sign up for GitHub

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

Jump to bottom

Regression in overcommit's tests in JRuby 9000 #3686

Closed
deivid-rodriguez opened this issue Feb 19, 2016 · 8 comments
Closed

Regression in overcommit's tests in JRuby 9000 #3686

deivid-rodriguez opened this issue Feb 19, 2016 · 8 comments
Labels
JRuby 1.7.x JRuby 9000 regression
Milestone
JRuby 9.1.0.0

Comments

@deivid-rodriguez
Copy link
Contributor

https://github.com/brigade/overcommit

In JRuby 1.7.24

git clone git@github.com:brigade/overcommit.git
cd overcommit
gem install bundler
bundle
bundle exec rspec ./spec/overcommit/hook/pre_push/protected_branches_spec.rb[1:1:1:1,1:1:2:1]

results in

Coverage may be inaccurate; set "cli.debug=true" ("-Xcli.debug=true") in your .jrubyrc or do JRUBY_OPTS="-d"
Run options: include {:ids=>{"./spec/overcommit/hook/pre_push/protected_branches_spec.rb"=>["1:1:1:1", "1:1:2:1"]}}
..

Finished in 0.082 seconds (files took 0.447 seconds to load)
2 examples, 0 failures

Coverage report generated for RSpec to /home/deivid/Code/overcommit/coverage. 981 / 2306 LOC (42.54%) covered.

whereas in JRuby 9.0.5.0 results in

Coverage may be inaccurate; set "cli.debug=true" ("-Xcli.debug=true") in your .jrubyrc or do JRUBY_OPTS="-d"
Run options: include {:ids=>{"./spec/overcommit/hook/pre_push/protected_branches_spec.rb"=>["1:1:1:1", "1:1:2:1"]}}
.F

Failures:

  1) Overcommit::Hook::PrePush::ProtectedBranches when pushing to unprotected branch when push is destructive should pass the check
     Failure/Error: protected?(pushed_ref.remote_ref) && pushed_ref.destructive?

     NameError:
       undefined method `remote_ref' for class `#<Class:0x406b058b>'
     # ./lib/overcommit/hook/pre_push/protected_branches.rb:18:in `block in illegal_pushes'
     # ./lib/overcommit/hook/pre_push/protected_branches.rb:17:in `illegal_pushes'
     # ./lib/overcommit/hook/pre_push/protected_branches.rb:5:in `run'
     # ./spec/support/matchers/hook.rb:10:in `matches?'
     # ./spec/support/matchers/hook.rb:84:in `block in matches?'
     # ./spec/overcommit/hook/pre_push/protected_branches_spec.rb:36:in `block in (root)'
     # ./spec/spec_helper.rb:52:in `block in (root)'
     # ./lib/overcommit/utils.rb:259:in `with_environment'
     # ./spec/spec_helper.rb:51:in `block in (root)'

Finished in 0.161 seconds (files took 0.563 seconds to load)
2 examples, 1 failure

Failed examples:

rspec ./spec/overcommit/hook/pre_push/protected_branches_spec.rb:36 # Overcommit::Hook::PrePush::ProtectedBranches when pushing to unprotected branch when push is destructive should pass the check

Coverage report generated for RSpec to /home/deivid/Code/overcommit/coverage. 983 / 2306 LOC (42.63%) covered.

Regards!
@deivid-rodriguez deivid-rodriguez mentioned this issue Feb 19, 2016
Closed
@kares kares added this to the JRuby 9.1.0.0 milestone Feb 19, 2016
@headius
Copy link
Member

headius commented Apr 28, 2016

Confirmed still broken. Investigating.

@headius
Copy link
Member

headius commented Apr 28, 2016

This appears to be happening against an rspec-mocks thingy...method_double.rb, whatever that's for. Now this code gets seriously tangled...

@headius
Copy link
Member

headius commented Apr 28, 2016

Ok, seemed to have narrowed it down to this code in rspec-mocks:

      # works around the fact that repeated calls for method parameters will
      # falsely return empty arrays on JRuby in certain circumstances, this
      # is necessary here because we can't dup/clone UnboundMethods.
      #
      # This is necessary due to a bug in JRuby prior to 1.7.5 fixed in:
      # https://github.com/jruby/jruby/commit/99a0613fe29935150d76a9a1ee4cf2b4f63f4a27
      if RUBY_PLATFORM == 'java' && JRUBY_VERSION.split('.')[-1].to_i < 5
        def find_method(mod)
          mod.dup.instance_method(@method_name)
        end
      else
        def find_method(mod)
          mod.instance_method(@method_name)
        end
      end

This workaround appears to be doing something strange when it comes to your PushedRef struct. With this code intact, I track the classes coming into this method, and after the first call to remote_ref, the next spec runs with the same PushedRef struct, but this time the remote_ref method is missing and the mock blows up deep inside rspec code.

If I remove the workaround and just go with the regular code, your specs pass fine.

@headius
Copy link
Member

headius commented Apr 28, 2016

Hah...well based on the conditional here, this hack was intended to run only under JRuby 1.7.4 and lower, but it did so by checking that the last digit of JRUBY_VERSION is less than 5. That has not been the case for all of JRuby 9k, so this started firing again :-)

So there's two things to do:

  • Get rspec folks to remove this conditional.
  • Figure out why this workaround breaks in other ways

@headius
Copy link
Member

headius commented Apr 28, 2016

Ok, I think this might be the bug:

[--dev] ~/projects/jruby/overcommit $ jruby -e "x = Struct.new(:foo, :bar, :baz); 2.times { p x.instance_methods.sort - Object.instance_methods - Enumerable.instance_methods; x.dup }"
[:[], :[]=, :bar, :bar=, :baz, :baz=, :dig, :each, :each_pair, :foo, :foo=, :length, :members, :size, :values, :values_at]
[:[], :[]=, :dig, :each, :each_pair, :length, :members, :size, :values, :values_at]

[--dev] ~/projects/jruby/overcommit $ ruby23 -e "x = Struct.new(:foo, :bar, :baz); 2.times { p x.instance_methods.sort - Object.instance_methods - Enumerable.instance_methods; x.dup }"
[:[], :[]=, :bar, :bar=, :baz, :baz=, :dig, :each, :each_pair, :foo, :foo=, :length, :members, :size, :values, :values_at]
[:[], :[]=, :bar, :bar=, :baz, :baz=, :dig, :each, :each_pair, :foo, :foo=, :length, :members, :size, :values, :values_at]

@headius
Copy link
Member

headius commented Apr 28, 2016

Note that this bug exists in both 1.7 and in 9k...we only see this impact on rspec in 9k because the last digit of JRUBY_VERSION is 0.

headius added a commit that referenced this issue Apr 28, 2016
@headius
Spec that struct-class duping produces identical method tables.
a4323b3 
This could be expanded into a full set of dup/clone specs, or
the Class#dup/clone specs could be expanded to also test other
types of "weird" classes like structs and prepends.

For #3686
@headius headius closed this as completed Apr 28, 2016
@deivid-rodriguez deivid-rodriguez mentioned this issue Apr 29, 2016
Merged
@deivid-rodriguez
Copy link
Contributor Author

Thanks @headius! ❤️ 💛 💜

I added a fix in rspec-mocks!

@headius
Copy link
Member

headius commented May 2, 2016

@deivid-rodriguez Thank you!

eregon pushed a commit to ruby/spec that referenced this issue May 10, 2016
@headius @eregon
Spec that struct-class duping produces identical method tables.
2d068df 
This could be expanded into a full set of dup/clone specs, or
the Class#dup/clone specs could be expanded to also test other
types of "weird" classes like structs and prepends.

For jruby/jruby#3686
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
JRuby 1.7.x JRuby 9000 regression
Projects
None yet
Milestone
JRuby 9.1.0.0
Development

No branches or pull requests
3 participants
@headius @kares @deivid-rodriguez