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

Regression in overcommit's tests in JRuby 9000 #3686

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

Comments

Projects
None yet
3 participants
@deivid-rodriguez

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!

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Apr 28, 2016

Member

Confirmed still broken. Investigating.

Member

headius commented Apr 28, 2016

Confirmed still broken. Investigating.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Apr 28, 2016

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@headius

headius Apr 28, 2016

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@headius

headius Apr 28, 2016

Member

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

This comment has been minimized.

Show comment
Hide comment
@headius

headius Apr 28, 2016

Member

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]
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

This comment has been minimized.

Show comment
Hide comment
@headius

headius Apr 28, 2016

Member

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.

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 headius added the JRuby 1.7.x label Apr 28, 2016

headius added a commit that referenced this issue Apr 28, 2016

Spec that struct-class duping produces identical method tables.
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 Apr 28, 2016

@deivid-rodriguez

This comment has been minimized.

Show comment
Hide comment
@deivid-rodriguez

deivid-rodriguez Apr 29, 2016

Thanks @headius! ❤️ 💛 💜

I added a fix in rspec-mocks!

Thanks @headius! ❤️ 💛 💜

I added a fix in rspec-mocks!

@headius

This comment has been minimized.

Show comment
Hide comment
Member

headius commented May 2, 2016

@deivid-rodriguez Thank you!

eregon added a commit to ruby/spec that referenced this issue May 10, 2016

Spec that struct-class duping produces identical method tables.
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