Cannot stub_chain with the 'select' method #587

Closed
phantomwhale opened this Issue Aug 2, 2012 · 8 comments

Comments

Projects
None yet
5 participants
@phantomwhale

I found an odd "before" block in our code that seemed to define a stub unnecessarily, so I tried to factor it into one stub_chain call

I have created a gist to demonstrate this : https://gist.github.com/3234264

What happens when I put it all into one stub_chain is I see a NoMethodError: private method `select' called for #Object:0x7f5b31904988

I then changed the method I was stubbing to be called something else (telect) and it didn't throw the error.

Is the "select" method somehow a reserved method that rspec is unable to stub_chain ?

@alindeman

This comment has been minimized.

Show comment Hide comment
@alindeman

alindeman Aug 3, 2012

Contributor

In fact, this is a problem on any stub.

class Thing
end

describe Thing do
  it "attempts to stub the select method" do
    t = Thing.new
    t.stub(:select => "stubbed method")

    puts t.select
  end
end

produces

  1) Thing attempts to stub the select method
     Failure/Error: puts t.select
     NoMethodError:
       private method `select' called for #<Thing:0x007fbc397ac298>
     # ./foo_spec.rb:10:in `block (2 levels) in <top (required)>'

My gut is that Kernel#select is interfering somehow, but I haven't nailed it completely down yet.

Contributor

alindeman commented Aug 3, 2012

In fact, this is a problem on any stub.

class Thing
end

describe Thing do
  it "attempts to stub the select method" do
    t = Thing.new
    t.stub(:select => "stubbed method")

    puts t.select
  end
end

produces

  1) Thing attempts to stub the select method
     Failure/Error: puts t.select
     NoMethodError:
       private method `select' called for #<Thing:0x007fbc397ac298>
     # ./foo_spec.rb:10:in `block (2 levels) in <top (required)>'

My gut is that Kernel#select is interfering somehow, but I haven't nailed it completely down yet.

@alindeman

This comment has been minimized.

Show comment Hide comment
@alindeman

alindeman Aug 3, 2012

Contributor

Ahhh, I get it now.

RSpec restores the visibility of methods it stubs. Since Kernel#select is a private method, the stub is defined as private.

You can see that with:

class Thing
end

describe Thing do
  it "attempts to stub the select method" do
    t = Thing.new
    t.stub(:select => "stubbed method")

    puts t.__send__(:select) # prints "stubbed method"
  end
end

This does feel awkward in this case. Thoughts @dchelimsky, @myronmarston ?

Contributor

alindeman commented Aug 3, 2012

Ahhh, I get it now.

RSpec restores the visibility of methods it stubs. Since Kernel#select is a private method, the stub is defined as private.

You can see that with:

class Thing
end

describe Thing do
  it "attempts to stub the select method" do
    t = Thing.new
    t.stub(:select => "stubbed method")

    puts t.__send__(:select) # prints "stubbed method"
  end
end

This does feel awkward in this case. Thoughts @dchelimsky, @myronmarston ?

@andreychernih

This comment has been minimized.

Show comment Hide comment
@andreychernih

andreychernih Aug 15, 2012

Yeah, that's pretty painful. Can't stub :select too.

Yeah, that's pretty painful. Can't stub :select too.

@alindeman

This comment has been minimized.

Show comment Hide comment
@alindeman

alindeman Aug 15, 2012

Contributor

@dchelimsky, @myronmarston: any thoughts?

Does it make sense to have a list of special cases where we don't set the visibility (which methods .. or everything on Kernel?). Or remove the restoration of visibility altogether (does this make sense and/or is it a breaking change?)

Contributor

alindeman commented Aug 15, 2012

@dchelimsky, @myronmarston: any thoughts?

Does it make sense to have a list of special cases where we don't set the visibility (which methods .. or everything on Kernel?). Or remove the restoration of visibility altogether (does this make sense and/or is it a breaking change?)

@dchelimsky

This comment has been minimized.

Show comment Hide comment
@dchelimsky

dchelimsky Aug 15, 2012

Owner

Removing restoration of visibility would be a breaking change.

As for whitelisting some special cases, I'm open to it of someone can come up with a reasonably reliable way to handle it. I'm opposed to it if it leads us down a rabbit hole of special cases, which is what I suspect would be the case.

Owner

dchelimsky commented Aug 15, 2012

Removing restoration of visibility would be a breaking change.

As for whitelisting some special cases, I'm open to it of someone can come up with a reasonably reliable way to handle it. I'm opposed to it if it leads us down a rabbit hole of special cases, which is what I suspect would be the case.

@myronmarston

This comment has been minimized.

Show comment Hide comment
@myronmarston

myronmarston Aug 15, 2012

Owner

Hmm...I would think that stubbing select would work OK in these 2 cases:

  • When stubbing it on a test double. I believe rspec-mocks makes all stubbed methods public on pure mock objects.
  • When stubbing it on an object that has a public select method.

If it's being stubbed on an object that isn't a pure mock, then the private visibility would be kept. This is what's happening in @alindeman's example above. I think that it's the right behavior, actually.

The question is, in @phantomwhale's gist, where select is in the middle of a stub_chain, what is the object it is stubbed on? I'm a bit fuzzy on what rspec-mocks uses as the intermediate objects. If it's not a pure mock object, it probably should be, which would fix this issue, right? Otherwise, maybe one of my "assumed working" cases above actually doesn't work that way and that issue should be fixed.

Owner

myronmarston commented Aug 15, 2012

Hmm...I would think that stubbing select would work OK in these 2 cases:

  • When stubbing it on a test double. I believe rspec-mocks makes all stubbed methods public on pure mock objects.
  • When stubbing it on an object that has a public select method.

If it's being stubbed on an object that isn't a pure mock, then the private visibility would be kept. This is what's happening in @alindeman's example above. I think that it's the right behavior, actually.

The question is, in @phantomwhale's gist, where select is in the middle of a stub_chain, what is the object it is stubbed on? I'm a bit fuzzy on what rspec-mocks uses as the intermediate objects. If it's not a pure mock object, it probably should be, which would fix this issue, right? Otherwise, maybe one of my "assumed working" cases above actually doesn't work that way and that issue should be fixed.

@dchelimsky

This comment has been minimized.

Show comment Hide comment
@dchelimsky

dchelimsky Aug 16, 2012

Owner

I fixed this in rspec/rspec-mocks@05741e9 but didn't realize this was reported in rspec-rails when I wrote the commit message to try to close issue #587.

@myronmarston the issue was that the objects in the middle of the chain were Object.new. Now they are Mock.new.

Owner

dchelimsky commented Aug 16, 2012

I fixed this in rspec/rspec-mocks@05741e9 but didn't realize this was reported in rspec-rails when I wrote the commit message to try to close issue #587.

@myronmarston the issue was that the objects in the middle of the chain were Object.new. Now they are Mock.new.

@dchelimsky dchelimsky closed this Aug 16, 2012

@alindeman

This comment has been minimized.

Show comment Hide comment
@alindeman

alindeman Aug 16, 2012

Contributor

Nice one.

Contributor

alindeman commented Aug 16, 2012

Nice one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment