equal matcher broken for Delegator since 309c4f08b59651737ddc8c3c6e0167220b604b12

[jeremyevans]

Commit 309c4f0 changed the should/should_not integration into Kernel in such a way that it breaks the equal matcher on DelegateClass instances.

Consider the following code:

require 'delegate'

describe 'should equal' do
  it "should not falsely raise an error when the two things are equal" do
    c = Class.new(DelegateClass(Array))
    o = c.new([1, 2])
    o.should equal(o)
  end
end

On RSpec 2.9 and previous versions, this example passes. On RSpec 2.10 and later versions, this example fails.

The reason behind this is the implementation of the delegate library in the stdlib, which does this:

class Delegator < BasicObject
  kernel = ::Kernel.dup
  kernel.class_eval do
    [:to_s,:inspect,:=~,:!~,:===,:<=>,:eql?,:hash].each do |m|
      undef_method m
    end
  end
  include kernel

Basically, at the time the delegate library is loaded, it checks the Kernel module for methods and defines most of those methods on the Delegate class. So with the previous RSpec code, the Kernel module was modified by default when RSpec was loaded, so when you loaded the delegate library after that, the should/should_not methods were included in the "kernel" module that is included in Delegator.

Commit 309c4f0 changes this so that Kernel is not modified by default when RSpec is loaded, it is not modified till later (unsure exactly when, looks like first example group definition from some brief testing). Unfortunately, this means that the should/should_not methods are not defined on Delegator, so when they are called, method_missing calls the method on the delegated object instead of the Delegator itself. Since equal checks for identity, and the delegated object is not the same as the Delegator instance, this breaks the equal matcher.

The only work around I can see now is enabling the should/should_not Kernel integration before requiring delegate:

require 'rspec/expectations/syntax'
RSpec::Expectations::Syntax.enable_should
require 'delegate'
# ...

This is ugly but appears to work. Unfortunately, I don't know how portable it is to older versions of RSpec.

This isn't a hypothetical error, this causes some of Sequel's specs to break. As Sequel's specs are designed to work on all versions of RSpec (going back to at least RSpec 1.3), I would prefer a direct fix or a workaround that wasn't RSpec version dependent. The only simple way I can think of for doing that now would be to call RSpec::Expectations::Syntax.enable_should at the end of the rspec/expectations/syntax.rb file and requiring the rspec/expectations/syntax.rb file when RSpec is initially loaded. However, that may cause other issues that I cannot foresee.

[jeremyevans]

Looks like my original workaround only works on master, on 2.10.0 you need to require a different file, and you don't have to call a method. This is the best workaround I can think of currently to handle both 2.10.0 and the master branch:

if defined?(RSpec)
  begin
    # Fix Master branch
    require 'rspec/expectations/syntax'
    if defined?(RSpec::Expectations::Syntax) && RSpec::Expectations::Syntax.respond_to?(:enable_should)
       RSpec::Expectations::Syntax.enable_should 
    end
  rescue LoadError
    begin
      # Fix 2.10.0
      require 'rspec/expectations/extensions/kernel'
    rescue LoadError
      # Older versions don't require a fix
      nil
    end
  end
end

[myronmarston]

@jeremyevans -- thanks for the detailed bug report, and taking the time to test Sequel against unreleased rspec versions!

It's funny that you mention having the problem with delegate; I had some problems with delegate that were the impetus for enabling some syntax changes to rspec-expectations, and these changes fell out of that. The main changes to rspec-expectations are discussed in length in #119.

We've realized that the should/should_not syntax is prone to these issues (as is minitest/spec's must_blah syntax) since it relies upon should and should_not being available and working properly on every object in the system. However, RSpec doesn't own every object in the system, so if should does something unexpected (such as proxying it to some other object as delegate does) then specs can fail in weird, unexpected ways.

The solution we came up with is a new syntax:

c = Class.new(DelegateClass(Array))
o = c.new([1, 2])
expect(o).to equal(o)

By default, both the old should syntax and the new expect syntax are available. A configuration option is provided for configuring the available syntax, so that projects that want to use just one syntax and enforce it can.

Commit 309c4f0 changes this so that Kernel is not modified by default when RSpec is loaded, it is not modified till later (unsure exactly when, looks like first example group definition from some brief testing). Unfortunately, this means that the should/should_not methods are not defined on Delegator

Kernel is still modified when rspec-expectations are loaded, but it may happen at a slightly different point now (the file may get required after another file as already required delegator, for example). Currently it happens in rspec/matchers/configuration.rb.

As far as workarounds go, I believe this should work:

if defined?(RSpec::Expectations::Syntax)
  require 'delegate'
  RSpec::Expectations::Syntax.enable_should(Delegator)
end

RSpec::Expectations::Syntax.enable_should accepts a "syntax_host" argument that defaults to Kernel. You can define it use it to add the syntax to other objects that do not work properly with the syntax being added to Kernel. That said, RSpec::Expectations::Syntax.enable_should isn't an official public API at this point; we discussed it and decided against it since we think that the new expect syntax is the best solution for this problem.

Anyhow, I want to fix this in rspec-expectations so that users don't have to use this kind of hack to have it work with delegate properly (given that it did previously for you). I'll look into this this week and see if I can come up with something.

As Sequel's specs are designed to work on all versions of RSpec (going back to at least RSpec 1.3), I would prefer a direct fix or a workaround that wasn't RSpec version dependent.

Sequel's specs work on all rspec versions >= 1.3? That's pretty crazy cool! I've never heard of a project doing that before (plenty of projects work with lots of versions of runtime dependencies, but it's rare for a project to do that with the testing library it uses). What benefit do you get out of maintaining that level of compatibility?

[jeremyevans]

Myron,

Thanks for the quick response. I like the new #expect syntax and how it works around the issue, but I don't want to force everyone testing Sequel to upgrade to the master branch of rspec-expectations.

I tried the RSpec::Expectations::Syntax.enable_should(Delegator) approach first thinking it would be a cleaner fix, but it doesn't appear to work, the spec still fails. It even fails if you do RSpec::Expectations::Syntax.enable_should(c) inside the spec. I'm not sure exactly why that is, but o.should.instance_variable_get(:@actual).class # => 'Array', so it's clear that should is being delegated to the underlying object. I wonder if this is related to it checking if should is already defined?

I think Sequel's specs work on RSpec 1.2.x+ (not sure which point version), but I don't test on RSpec 1.2 any longer. I do test regularly on RSpec 1.3. As to the benefit of working on both RSpec 1 and 2, it's probably small, but it's been fairly easy to keep it working on both, so there hasn't been a reason to stop working on older versions. This issue is the first RSpec-related problem I remember having in years.

Thanks for looking into the issue, hopefully there's a simpler fix than the ugly workaround I used.

[myronmarston]

OK, so I think I've figured it out.

The changes in rspec-expectations that you pointed out looked like the source of the regression, but it turns out it's due to a change in rspec-core:

rspec/rspec-core@b93433d

Before that commit, rspec-core loaded the rspec-expecations kernel extension, causing should to be added to Kernel when rspec-core is loaded. That commit changed rspec-core so that the kernel extension is no longer loaded at that point. rspec-expectations as a whole is loaded quite late in the process--when the first example group is defined, unless it is explicitly configured (using RSpec.configure { |c| c.expect_with :rspec }). (There's a whole history to why rspec-expectations is loaded when it is involving an obscure ruby 1.9 bug I found--my blog contains more details).

So...before RSpec 2.10, when you had a spec that loaded and used the delegate library, the rspec-expectations kernel extension was already loaded, and should would be available on delegate objects. In 2.10, your spec loads delegate before the kernel extension has been loaded, so should is never added.

The way to fix this is to manually load rspec/expecations before delegate. This works on RSpec 2.10 and rspec-master:

require 'rspec/expectations'
require 'delegate'

describe 'should equal' do
  it "should not falsely raise an error when the two things are equal" do
    c = Class.new(DelegateClass(Array))
    o = c.new([1, 2])
    o.should equal(o)
  end
end

I'm not sure what we should do (if anything) to fix this.

Any ideas?

/cc @dchelimsky @alindeman @justinko

[jeremyevans]

Thanks, that appears to work and greatly simplifies the workaround I was using.

[myronmarston]

Great, I'm glad that fixed it for you.

I've been trying to think of a way to fix it, and none of the options I've thought of are very appealing:

• We can have rspec-core require rspec-expectations early on so that the kernel extension is loaded before users have a chance to require delegate. This would defeat the purpose of allowing users to configure rspec to expect_with :stdlib for those users who use rspec-core but not rspec-expectations...so I don't think we should go that route.
• We can put special handling for delegate in rspec-expectations--i.e. require it in rspec-expectations (even if users aren't using it) and add should and should_not to it. I don't think we want to start down the path of special-case handling of objects that do meta magic and mixin only parts of Kernel.

Given that we have a proper solution now (use the new expect syntax) and there's no good solution I can think of, I'm going to close this. If anyone thinks of a better solution, feel free to comment and re-open.

[jeremyevans]

The workaround works almost everywhere. However, it appears on jruby 1.7.0 preview 1 (and jruby-head), delegate is required by rspec, causing it to fail.

Code:

p defined?(Delegator)
p $LOADED_FEATURES.grep(/dele/i)
require 'rspec/expectations'
require 'delegate'

describe 'should equal' do
  it "should not falsely raise an error when the two things are equal" do
    c = Class.new(DelegateClass(Array))
    o = c.new([1, 2])
    o.should equal(o)
  end
end

Output:

$ jruby -S rspec ../equal_spec.rb
"constant"
["/usr/local/jruby/lib/ruby/1.9/delegate.rb"]
F

Failures:

  1) should equal should not falsely raise an error when the two things are equal
     Failure/Error: o.should equal(o)

       expected #<#<Class:0x12b8d07>:3228> => [1, 2]
            got #<Array:3230> => [1, 2]

       Compared using equal?, which compares object identity,
       but expected and actual are not the same object. Use
       'actual.should == expected' if you don't care about
       object identity in this example.
     # /home/billg/equal_spec.rb:10:in `(root)'

Finished in 0.027 seconds
1 example, 1 failure

Failed examples:

rspec /home/billg/equal_spec.rb:7 # should equal should not falsely raise an error when the two things are equal.

Some simple checking shows that rspec requires delegator on jruby 1.7.0:

$ jruby -ve 'p defined?(Delegator)'
jruby 1.7.0.preview1 (ruby-1.9.3-p203) (2012-05-22 fffffff) (OpenJDK Server VM 1.7.0_03) [OpenBSD-i386-java]
nil
$ jruby -e 'require "rspec"; p defined?(Delegator)'
"constant"

Is there anything that can be done about this inside RSpec?

[myronmarston]

Hmm...I'm not seeing the same thing with jruby 1.6.7.2 (the jruby I installed recently with RVM). Maybe there's a jruby regression in there somewhere?

Is there anything that can be done about this inside RSpec?

I mentioned the only ways to deal with this I can think of above, but I find both to be distasteful, unfortunately. Got any better ideas?

[myronmarston]

I just install jruby-head with RVM and I'm seeing it now, too...I'll dig in and see if I can figure out what's going on.

[myronmarston]

OK, I think I've figured out what's going on.

• RSpec loads drb since it supports a drb mode.
• As of a month ago, JRuby's drb implementation loads its weakref library.
• Weakref loads delegate.

I just pushed an attempted fix to rspec-core, to the delay-drb-loading branch.

Can you give that a try? It fixed the problem for me. I need to do a bit more testing before pushing it into rspec-core master since drb is tricky to test with unit tests.

[jeremyevans]

The delay-drb-loading branch appears to fix both the simple test case and Sequel's extension specs on jruby 1.7.0. Thanks!

[myronmarston]

This has been merged into rspec-core.