Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

ArgumentMatchers make it impossible to have lets with same name #102

Closed
amarshall opened this Issue · 31 comments

5 participants

@amarshall

As a result of the methods defined in RSpec::Mocks::ArgumentMatchers, it is impossible to have a let() with certain names. The most potentially devastating of which is boolean. As anytime boolean is referenced within the test, the method in RSpec::Mocks::ArgumentMatchers is called, rather than referencing the let that was created. This can be seen by running the example tests below. This has all sorts of horrific ramifications, including false-passing tests and utter confusion, as it is non-obvious until you inspect the object itself that it is not the value you assigned it to.

I'm not sure what the "right" way to fix this is, or if it can be properly fixed, but at the least RSpec should raise an exception when "illegal" names are passed to let, so the unexpected behavior is at least explicitly denied by RSpec.

class Example
  def self.test(boolean)
    boolean
  end
end

shared_examples "example" do
  context "and it is true" do
    let(:value) { true }
    it { should be_true }
    it { subject.class.should == TrueClass }
  end

  context "and it is false" do
    let(:value) { false }
    it { should be_false }
    it { subject.class.should == FalseClass }
  end

  context "and it is not defined" do
    it "raises a NameError" do
      expect { subject }.to raise_error(NameError)
    end
  end
end

describe Example do
  describe "#test" do
    # Passes for "bool" & "some_valid_name", fails for all others
    %w[bool some_valid_name boolean any_args anything no_args duck_type hash_including].each do |name|
      context "when variable is named #{name}" do
        subject { Example.test(send(name.to_sym)) }
        let(name.to_sym) { value }
        include_examples "example"
      end
    end
  end
end
@justinko

The failure message is plenty good enough to tell you what's going on:


15) Example#test when variable is named no_args and it is false
Failure/Error: it { subject.class.should == FalseClass }
expected: FalseClass
got: RSpec::Mocks::ArgumentMatchers::NoArgsMatcher (using ==)
Diff:
@@ -1,2 +1,2 @@
-FalseClass
+RSpec::Mocks::ArgumentMatchers::NoArgsMatcher


Whether it is Ruby, Rails, or RSpec, if you suspect a method name is too vague or general, it's always a good idea to check the applicable API's before hand. Or, you can always be more verbose in your naming. boolean is too terse for application code, IMO.

Bottom line, executing a "check" for every let is not worth the effort nor complexity. It is a rabbit hole.

@justinko justinko closed this
@amarshall

I disagree. I don't think I've ever seen anyone explicitly check that the class of a boolean is indeed FalseClass. The only reason that test exists in my example is to illustrate what is happening.

A check isn't necessary, I was just suggesting it as a possible solution. The real issue, as I see it, is that the let's scope is overridden by an API method, despite it being closer to the actual test, which is completely unexpected.

@dchelimsky dchelimsky reopened this
@dchelimsky
Owner

@amarshall agreed that let should override other definitions that are out of sight in the spec.

In @justinko's defense, your original post focused more on a solution than a problem, and that solution would be a rabbit-hole/non-starter. Your subsequent comment clarifies the problem very well.

@amarshall

@dchelimsky I apologize for the lack of clarity in my original description of the problem, and am glad my comment cleared it up and got it recognized as a real issue and hope to see it fixed sometime soon :).

@dchelimsky
Owner

It'll get fixed sooner if you submit a patch ;)

@amarshall

I wish I knew enough about RSpec's internals to do that right now, but I may take this as an opportunity to dive into them a little later this week. Don't get your hopes up though ;).

@justinko

I'll fix it this weekend since I completely overlooked the problem.

@justinko justinko was assigned
@dchelimsky
Owner

It's not like a punishment system :)

But that would be great.

@justinko

@dchelimsky I just spent about 5 hours on this. Here is my conclusion:

Because there are two stub methods, they cannot exist within the same scope. To allow the two methods to coexist, one (obj.stub(:foo)) is defined in Object, and the other (stub('foo')) is inserted (along with the arg matchers) into the metaclass of each ExampleGroup instance. The latter methods cannot be over-ridden (via let, def, etc.) because they exist in the metaclass.

Here are my two solutions:

1.) Deprecate object.stub(:foo) for object.stubs(:foo).
2.) Deprecate stub and mock for double. stub would remain in Object for messaging (object.stub(:foo)).

Both of these solutions would allow us to not muck around with metaclasses, and let the user redefine any RSpec method they want.

Personally, I would prefer option 2. You have any better solutions? /cc @myronmarston

@dchelimsky
Owner

@justinko neither option is acceptable to me. I'd rather suffer the mess of treating stub differently from other methods than deprecate either one.

@justinko

@dchelimsky well we can at least move the arg matchers out of the metaclass:

Remove this and somehow inject it into the host class.

@dchelimsky
Owner

I haven't looked into this yet, but it strikes me that the root cause is that arg matchers get added after the methods defined by let, thus overriding them, in which case the solution would be to make sure that the methods declared by let are actually defined later. That make any sense to you?

@justinko

The arg matchers are injected right before an example is eval'd. So yes, they are added later, but it doesn't matter because they're being added to the metaclass --- Ruby looks for the method in the metaclass first.

Simply including the arg matchers in ExampleGroup would solve this.

@dchelimsky
Owner

DO IT!

@justinko

Given that rspec-mocks needs to run standalone, and that we cannot call RSpec::Core::ExampleGroup directly in rspec-mocks, this is the best that I could come up with: http://pastie.org/3377917

WDYT?

@dchelimsky
Owner

@justinko how about including it in ExampleGroup from the adapter?

@justinko

@dchelimsky I thought of that, but we still need to get those methods in rspec-mocks when used stand-alone.

@dchelimsky
Owner

@justinko on ExampleGroup, not Object

@justinko

By adapter, I'm assuming you mean RSpec::Mocks.setup? This would mean checking for ExampleGroup:

if defined?(RSpec::Core::ExampleGroup)
  RSpec::Core::ExampleGroup.send :include, RSpec::Mocks::ArgumentMatchers
end

Obviously, this still wouldn't solve the problem of making arg matchers available in rspec-mocks stand-alone.

If you do mean the adapter from rspec-core, yes, that would fix it in rspec-core, but not when rspec-mocks is used stand-alone.

@dchelimsky
Owner

@justinko RSpec::Mocks::Methods are methods like stub and should_receive that belong on objects. What gets added to ExampleGroup is RSpec::Mocks::ExampleMethods (see https://github.com/rspec/rspec-mocks/blob/master/lib/rspec/mocks.rb#L12-14). How about we add include RSpec::Mocks::ArgumentMatchers to that block?

@justinko

RSpec::Mocks::ArgumentMatchers is currently included in RSpec::Mocks::ExampleMethods, so that won't change anything.

The objective is to get arg matchers out of the "host" metaclass, and onto the same scope as let (which is in the example group instance, not example group instance metaclass).

@dchelimsky
Owner

Makes sense. Let's do the same for RSpec::Mocks::ExampleMethods then: a one-time addition to the class. Something like:

def setup(host)
  # ...
  host.class.send :include, RSpec::Mocks::ExampleMethods unless host.class < RSpec::Mocks::ExampleMethods
  # ...
end
@justinko

If you take RSpec::Mocks::ExampleMethods out of the metaclass, it will then be in the same scope as RSpec::Mocks::Methods, which means you can't have both stubs. I am pretty certain that is why you injected the methods into the metaclass: to separate the scopes of the stubs.

I think you can probably now see why I spent 5 hours on this, and also my "radical" suggestions such as renaming one of them to stubs :)

@dchelimsky
Owner

If you take RSpec::Mocks::ExampleMethods out of the metaclass, it will then be in the same scope as RSpec::Mocks::Methods

I see what you're saying, but we should be able to solve this by including things in the right order, no? e.g.


1.9.3p0 :001 > module FirstModule; def foo; "first"; end; end
 => nil 
1.9.3p0 :002 > module SecondModule; def foo; "second"; end; end
 => nil 
1.9.3p0 :003 > class FirstClass; end
 => nil 
1.9.3p0 :004 > class SecondClass; end
 => nil 
1.9.3p0 :005 > Object.send :include, FirstModule
 => Object 
1.9.3p0 :006 > SecondClass.send :include, SecondModule
 => SecondClass 
1.9.3p0 :007 > FirstClass.new.foo
 => "first" 
1.9.3p0 :008 > SecondClass.new.foo
 => "second" 
@justinko

@dchelimsky I actually tried that, and it didn't work with the spec given in this issue. However, I just tried it with a much more simple spec and it does work! Now I'm trying to get the example spec to pass...

@justinko

@dchelimsky okay, so your solution does work, but not with example groups that don't contain an example:

# fails
describe do
  let(:boolean) { true }
  describe do
    specify { boolean.class.should eq(TrueClass) }
  end
end

# passes
describe do
  let(:boolean) { true }
  specify { boolean.class.should eq(TrueClass) }
end

This is because the "host" that gets passed to RSpec::Mocks.setup is an instance of RSpec::Core::ExampleGroup::Nested_1::Nested_1. So as you can see, RSpec::Core::ExampleGroup::Nested_1 does not get the inclusion.

I'm not quite sure how to solve this. Maybe we could traverse up each example group subclass (via split('::') and const_get) and include the methods? This would happen on every example run, but we would obviously check to see if the methods have been included or not (via your < approach).

@dchelimsky
Owner

By one-time addition, I meant to ExampleGroup (even though my example didn't show that). We should be able to extend ExampleGroup (or TestCase in the case of t/u or minitest) with the methods we want. Then the subs inherit them.

@justinko

I haven't figured it out yet, but Object#stub always wins, even when doing this:

RSpec::Core::ExampleGroup.send :include, RSpec::Mocks::ExampleMethods
Object.class_eval { include RSpec::Mocks::Methods }
RSpec::Core::ExampleGroup.send :include, RSpec::Mocks::ExampleMethods
@snuggs

Has there been any progress on this? I am attempting to use rspec-mocks standalone with the following:

module MiniTest
  class Spec
    require 'rspec/mocks/standalone'
  end
end

Is this correct? Because I can't seem to get this working in before :each and it specs

@dchelimsky? Long time no speak homie!

@myronmarston myronmarston referenced this issue from a commit in rspec/rspec-core
@myronmarston myronmarston Include RSpec::Mocks::ExampleMethods in the adapter.
Before now, that module has been included in the
singleton class of each example's evaluation
context (e.g. an example group instance) via
`RSpec::Mocks.setup`.  That approach has some issues,
though:

* It blows away MRI's method cache unnecessarily.
  Far better to include the module only once.
* It prevents users from overriding RSpec::Mocks::ArgumentMatchers
  methods with a `let` declaration (see rspec/rspec-mocks#102).

We still need to remove it from rspec-mocks' `setup`
method but this is a step towards that.
7846f6d
@myronmarston myronmarston referenced this issue in rspec/rspec-core
Merged

Fix mock adapters #1188

@myronmarston myronmarston referenced this issue from a commit
@michihuber michihuber Don't override let definitions with argument matchers
see #102

ArgumentMatchers (together with ExampleMethods) were
included into the ExampleGroup instance's metaclass.
This caused argument matcher methods such as boolean()
or hash_containing() to override let definitions with
the same name.

This commit fixes this bug by including
ArgumentMatchers into the example group's class and
not into the metaclass.

Conflicts:
	Changelog.md
	lib/rspec/mocks.rb
	spec/rspec/mocks/mock_spec.rb

(This commit originated in rspec/rspec-mocks#238. At this point,
the rest of the changes in that aren't valid or needed, and the
broken behavior these specs documented has been fixed upstream
in rspec-core by rspec/rspec-core#1188)
655294c
@myronmarston myronmarston referenced this issue from a commit
@myronmarston myronmarston Remove unnecessary module inclusion.
As of rspec/rspec-core#1188, rspec-core now takes
care of including the ExampleMethods module.
This should be faster (including once, rather than
once-per-example) and fixes #102.
e6bf83c
@myronmarston

Solved by #471.

@amarshall

Glad to see this finally fixed! :+1:

@sanemat sanemat referenced this issue from a commit in sanemat/rspec-mocks
@michihuber michihuber Don't override let definitions with argument matchers
see rspec#102

ArgumentMatchers (together with ExampleMethods) were
included into the ExampleGroup instance's metaclass.
This caused argument matcher methods such as boolean()
or hash_containing() to override let definitions with
the same name.

This commit fixes this bug by including
ArgumentMatchers into the example group's class and
not into the metaclass.
38377ef
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.