Mocha test failures when using JRuby

[chrisroos]

I came across these test failures when trying to merge PR #225.

It would appear that the changes in commit 43d5667 mean that we now have some test failures when using JRuby (I tested 9.0.0.0.pre2, 9.0.0.0 and 9.1.5.0).

$ ruby -v
jruby 9.1.5.0 (2.3.1) 2016-09-07 036ce39 Java HotSpot(TM) 64-Bit Server VM 24.79-b02 on 1.7.0_79-b15 +jit [darwin-x86_64]

$ MOCHA_OPTIONS=debug JRUBY_OPTS=-Xbacktrace.mask=true bundle exec rake test

  1) Failure:
StubClassMethodDefinedOnSuperclassTest#test_should_stub_method_on_superclass_and_leave_it_unchanged_after_test [/Users/chrisroos/Code/freerange/mocha/test/acceptance/stub_class_method_defined_on_superclass_test.rb:86]:
Snapshot has changed: {:added=>[], :removed=>[#<Class:#<Class:0x76d817ff>>#my_class_method (public)]}


  2) Failure:
StubInstanceMethodDefinedOnSingletonClassTest#test_should_stub_private_method_and_leave_it_unchanged_after_test [/Users/chrisroos/Code/freerange/mocha/test/acceptance/stub_instance_method_defined_on_singleton_class_test.rb:60]:
Snapshot has changed: {:added=>[], :removed=>[#<Class:#<#<Class:0x43e611b1>:0x35939e4a>>#my_singleton_method (private)]}


  3) Failure:
StubInstanceMethodDefinedOnSingletonClassTest#test_should_stub_protected_method_and_leave_it_unchanged_after_test [/Users/chrisroos/Code/freerange/mocha/test/acceptance/stub_instance_method_defined_on_singleton_class_test.rb:42]:
Snapshot has changed: {:added=>[], :removed=>[#<Class:#<#<Class:0x6ebd9dc>:0x662bfe10>>#my_singleton_method (protected)]}


  4) Failure:
StubInstanceMethodDefinedOnSingletonClassTest#test_should_stub_public_method_and_leave_it_unchanged_after_test [/Users/chrisroos/Code/freerange/mocha/test/acceptance/stub_instance_method_defined_on_singleton_class_test.rb:24]:
Snapshot has changed: {:added=>[], :removed=>[#<Class:#<#<Class:0x7ca25509>:0x57d01cc>>#my_singleton_method (public)]}


  5) Failure:
StubClassMethodDefinedOnClassTest#test_should_stub_private_method_and_leave_it_unchanged_after_test [/Users/chrisroos/Code/freerange/mocha/test/acceptance/stub_class_method_defined_on_class_test.rb:68]:
Snapshot has changed: {:added=>[], :removed=>[#<Class:#<Class:0x7e315f40>>#my_class_method (private)]}


  6) Failure:
StubClassMethodDefinedOnClassTest#test_should_stub_protected_method_and_leave_it_unchanged_after_test [/Users/chrisroos/Code/freerange/mocha/test/acceptance/stub_class_method_defined_on_class_test.rb:47]:
Snapshot has changed: {:added=>[], :removed=>[#<Class:#<Class:0xd6867ea>>#my_class_method (protected)]}


  7) Failure:
StubClassMethodDefinedOnClassTest#test_should_stub_public_method_and_leave_it_unchanged_after_test [/Users/chrisroos/Code/freerange/mocha/test/acceptance/stub_class_method_defined_on_class_test.rb:26]:
Snapshot has changed: {:added=>[], :removed=>[#<Class:#<Class:0x661a0b9e>>#my_class_method (public)]}


  8) Failure:
StubMethodDefinedOnModuleAndAliasedTest#test_stubbing_class_method_defined_by_aliasing_module_instance_method [/Users/chrisroos/Code/freerange/mocha/test/acceptance/stub_method_defined_on_module_and_aliased_test.rb:30]:
Snapshot has changed: {:added=>[], :removed=>[#<Class:#<Class:0x124f8942>>#aliased_module_instance_method (public)]}


  9) Failure:
StubAnyInstanceMethodDefinedOnSuperclassTest#test_should_stub_method_and_leave_it_unchanged_after_test [/Users/chrisroos/Code/freerange/mocha/test/acceptance/stub_any_instance_method_defined_on_superclass_test.rb:25]:
Snapshot has changed: {:added=>[], :removed=>[#<Class:0x68171e49>#my_superclass_method (public)]}

338 runs, 246 assertions, 9 failures, 0 errors, 0 skips

[floehopper]

@chrisroos: It's worth remembering that those snapshot assertions depend on the introspection gem which might not work correctly in JRuby.

[headius]

A quick glance through the introspection gem shows only one issue: ObjectSpace.each_object here: https://github.com/floehopper/introspection/blob/master/lib/introspection/change_detector.rb#L29

Arbitrary walking of all objects in the system is not supported under JRuby without a flag, since there's no simple/efficient way to walk all objects on a JVM heap (concurrent threads, GC, objects moving around, etc).

However, if this were changed to ObjectSpace.each_object(Class) it will always work on JRuby because we can always walk all classes in the system.

I'll try to run the mocha tests and see if JRuby master is any better. Many fixes since 9.1.5.0.

[headius]

I get the same 9 failures on JRuby master with or without ObjectSpace enabled. Investigating.

[headius]

Ok so these all seem to be the same problem, but I'm confused why it doesn't work (or rather, I'm confused why it works in MRI.

Basically, when mocking a method for a class/object that already has that method, I see the call to ensure_method_not_already_defined which does an undef on the metaclass. That explains the "after" snapshot showing that the method has been removed. But why isn't it still removed in MRI? How does it get restored in the MRI case?

I'm focusing on one test right now:

  def test_should_stub_public_method_and_leave_it_unchanged_after_test
    klass = Class.new do
      class << self
        def my_class_method
          :original_return_value
        end
        public :my_class_method
        def self.public(*args); end
      end
    end
    assert_snapshot_unchanged(klass) do
      test_result = run_as_test do
        klass.stubs(:my_class_method).returns(:new_return_value)
        assert_method_visibility klass, :my_class_method, :public
        assert_equal :new_return_value, klass.my_class_method
      end
      assert_passed(test_result)
    end
    assert_equal :original_return_value, klass.my_class_method
  end

klass is a newly-defined, anonymous class that has my_class_method as a class method. klass.stubs undefs the original my_class_method so the mock calls go through method_missing. I could not find code that reverses the undef, and as I'd expect on JRuby the method is still undef'ed in the second snapshot.

I just don't understand where that undef damage gets undone.

[headius]

Ok I think I've untangled some of it.

So I was following the wrong path. Confirm for me that I have this right...

When mocking an object, the method is not directly removed. Instead, a new module is prepended onto that object's class. Any methods that already existed on the original that now need to be mocked are undef'ed so they'll go through method_missing. Eventually, the prepended module's undef is removed with remove_method.

So it may be that our remove_method is not properly handling undef'ed methods.

[headius]

Ok, revision...under Ruby 2+, the prepended module gets a new definition for the method in question that just redispatches to method_missing. That method is later removed, so the dispatch returns to the original method. So it's getting overwritten by the def or improperly removed by the later remove_method.

[chrisroos]

Hey @headius. Thanks for investigating!

Ok, revision...under Ruby 2+, the prepended module gets a new definition for the method in question that just redispatches to method_missing. That method is later removed, so the dispatch returns to the original method.

Yes - @floehopper and I think that your understanding sounds correct.

So it's getting overwritten by the def or improperly removed by the later remove_method.

Can you clarify what "it" is in this sentence? The original method?

Assuming you are talking about the original method then based on the assertion messages the most likely scenario is that it's being incorrectly removed.

[headius]

Assuming you are talking about the original method then based on the assertion messages the most likely scenario is that it's being incorrectly removed.

Or at least removed for purposes of snapshotting. I thought I'd be able to reproduce the problem with this code, but it works fine on JRuby, contrary to what I expected based on mocha:

M = Module.new
cls = Class.new do
  class << self
    def foo
      :old
    end
    prepend M
  end
end

module M
  def foo
    :new
  end
end

p cls.foo

module M
  remove_method :foo
end

p cls.foo
p cls.public_methods - Class.public_methods
__END__
Running on JRuby (matches MRI):

$ jruby blah.rb
:new
:old
[:foo]

So either I'm not doing this the same way as mocha, or the snapshot is getting messed up somehow.

[headius]

Indeed, it seems to be something related to the snapshot. Here I print out the non-Class public methods for the given test, and the method the snapshot claims was removed is actually there:

$ jruby -r ./bundle/bundler/setup.rb -I test:lib test/acceptance/stub_class_method_defined_on_class_test.rb -n test_should_stub_public_method_and_leave_it_unchanged_after_test
Run options: -n test_should_stub_public_method_and_leave_it_unchanged_after_test --seed 44255

# Running:

[:my_class_method]
F

Finished in 0.189000s, 5.2910 runs/s, 5.2910 assertions/s.

  1) Failure:
StubClassMethodDefinedOnClassTest#test_should_stub_public_method_and_leave_it_unchanged_after_test [test/acceptance/stub_class_method_defined_on_class_test.rb:26]:
Snapshot has changed: {:added=>[], :removed=>[#<Class:#<Class:0x56928307>>#my_class_method (public)]}

1 runs, 1 assertions, 1 failures, 0 errors, 0 skips

Perhaps it is expecting the Method objects to be idempotent in some way?

[headius]

Ah-ha, I think I've figured it out. It appears our UnboundMethod is not prepend-aware.

[] ~/projects/mocha $ jruby -e "class A; class << self; def foo; end; end; end; p A.method(:foo).owner; module B; end; class << A; prepend B; end; p A.method(:foo).owner"
#<Class:A>
#<Class:0x46daef40>

[] ~/projects/mocha $ ruby23 -e "class A; class << self; def foo; end; end; end; p A.method(:foo).owner; module B; end; class << A; prepend B; end; p A.method(:foo).owner"
#<Class:A>
#<Class:A>

After the prepend, the owner of a given UnboundMethod is getting set to the prepended class shim rather than the relocated method table. This causes the snapshot logic to skip the method, since it does not appear to be from the given receiver:

  class Snapshot
    attr_reader :methods

    def initialize(object)
      @methods = (object.receivers rescue []).map do |receiver|
        [:public, :protected, :private].map do |visibility|
          query_method = "#{visibility}_instance_methods"
          x = receiver.send(query_method, false).map do |method|
            unbound_method = receiver.instance_method(method)
            if unbound_method.owner.equal?(receiver) # NOPE
              Method.new(unbound_method, visibility)
            end
          end.compact
        end
      end.flatten
    end

[headius]

With the fix in jruby/jruby#4250, all mocha tests appear to pass. I am still evaluating that fix, though.

[floehopper]

@headius: Thanks for getting to the bottom of the problem. Let us know if you think there's anything wrong with the mocha or introspection code and/or if there's anything we can do to help you.

[headius]

@floehopper I see no other issues with introspection. We were in error here.

I am a bit confused why the ObjectSpace.each_object use never produced a warning on JRuby. Perhaps that path is not followed for JRuby? In any case, it could be made to work on JRuby all the time, and still be equivalent logic, if you changed it to each_object(Class).

[headius]

jruby/jruby#4250 has been merged and will be in JRuby 9.1.6.0 (probably released in the next week).

[floehopper]

@headius: Many thanks. I've actually just removed change_detector.rb file from introspection (see floehopper/introspection#2) and so the calls to ObjectSpace.each_object no longer exist.

[floehopper]

We're planning to keep this issue open until we can verify that the Mocha tests pass in JRuby v9.1.6.0, ideally by adding JRuby to the Travis CI build (see #282).

[headius]

You could try adding jruby-head right now, so we can confirm master is green on mocha before release.

[floehopper]

@headius:

I tried adding jruby-head to the build matrix in this branch, but I'm seeing failures on Travis CI.

However, I then tried building JRuby at the HEAD commit and the Mocha build passes OK.

I assume that for some reason the jruby-head Travis CI build does not incorporate the fix.

[headius]

@floehopper Oh bother. Ok, I'll look into why it isn't updating. It's supposed to update every time we have a green build.

[floehopper]

I just restarted the build for the branch with jruby-head in the Travis matrix. The build seems to pass for the unit & acceptance tests with JRuby, but fail for the integration tests with JRuby & Minitest with JRuby & Test::Unit. In both cases the failure seems to be in this assertion about the execution point:

Failure: test_real_object_expectation_does_not_leak_into_subsequent_test(TestUnitTest)
/home/travis/build/freerange/mocha/test/integration/shared_tests.rb:171:in `test_real_object_expectation_does_not_leak_into_subsequent_test'
     168:     )
     169:     assert_failed(test_result)
     170:     exception = test_result.errors.first.exception
  => 171:     assert_equal execution_point, ExecutionPoint.new(exception.backtrace)
     172:     assert_match %r{undefined method `foo'}, exception.message
     173:   end
     174: end
org/jruby/RubyKernel.java:1115:in `catch'
org/jruby/RubyKernel.java:1115:in `catch'
<file: /home/travis/build/freerange/mocha/test/integration/shared_tests.rb; line: 166> expected but was
<file: org/jruby/RubyBasicObject.java; line: 1654>

I've also just installed JRuby v9.1.6.0 and I see the same failures locally. I suspect they might be fixable by filtering out JRuby lines from the backtrace, so I'm going to investigate that now.

[floehopper]

I've fixed the assertion failures and updated the build matrix to use the latest stable version of JRuby in this new branch and the build passed. I plan to use this to open a PR and then to get it merged.

[floehopper]

Now that I've demonstrated that the Mocha build passes with the latest stable version of JRuby in #288, I'm going to close this issue.