Bring back stub_chain (receive_message_chain)

[mhenrixon]

I just upgraded to RSpec 3 and am totally in love with being forced to clean up all my deprecated mocks and expectations.

However there is one I am truly missing. While only used in a handful of places in every project I feel like it should truly be part of the framework. My reasoning for it is the following. While it may violate the law of demeter it is a very convenient way of making sure that some class receives the expected arguments. Take the following example:

# I usually end up with something like below in my code
def fetch_something
  current_something.coll_association.find_by some: 123, other: 'sasd'
end

# I want to test that the finder gets called with the right arguments
it "finds cool_association using the right arguments" do
 current_something.stub_chain('cool_association.find_by').
                             with(some: 123, other: 'sasd') { cool_association }
end

Rather than stubbing the fetch_something method and return what I need I can ensure that it is only returned when the association receives the right arguments. I could refactor it into the below

# I could refactor the above into the following
def fetch_something
  fetch_something2 some: 123, other: 'sasd'
end

def fetch_something2(args = {})
  current_something.cool_association.find_by args
end

it "finds cool_association using the right arguments" do
  expect(subject).to receive(:fetch_something2).
                     with(some: 123, other: 'sasd') { cool_association } 
end

The above however won't guard me against accidentally deleting the line that actually does the fetching from the database. It also adds unnecessary complexity. I have to add another method just to verify the arguments are right and I still don't get the guard against accidental code deletion.

To get both I would have to refactor that into:

def fetch_something
  CoolAssociation.find_by some: 123, other: 'sasd', something_id: current_something.id)
end

it "finds cool_association using the right arguments" do
  expect(CoolAssociation).to receive(:find_by).
                             with(some: 123, other: 'sasd', something_id: current_something.id) { cool_association } 
end

Now I get both validation and guard and while the law of demeter isn't violated I feel violated having to write this code. That is my case for having something like receive_message_chain as a native member of rspec-mocks.

edit I realize I am mixing up the concepts a little with stub and expectations but there was no expectation on the stub_chain while I would if added use this with expectations to ensure that something calls the method chain using the right arguments. I would actually not stub it using allow.

[JonRowe]

You can use your original line of code and test with

expect(current_something).to receive(:cool_association).and_return(cool_association)
expect(cool_association).to receive(:find_by).with(some: 123, other: 'sasd') { scoped_cool_association }

Yes it's an extra line of test, but you can assert on more things, and in my opinion it helps exposes this as a code smell. If I remember correctly we took a deliberate decision not to replicate stub_chain in the new syntax for this very reason, it's only a short hand and removing it helps expose these sorts of smells.

Note you could also refactor your code on your class:

class CurrentSomething

  def cool_assosciation_with(query = {})
    self.cool_association.with(query)
  end
end

and then in your tests

expect(current_something).to receive(:cool_association_with).with(some: 123, other: 'sasd') { cool_association }

[mhenrixon]

@JonRowe I believe the job of the mocks framework should be to help me easily test my code in a readable fashion and not identify code smells. In my humble opinion there are far better tools for that job.

Don't get me wrong, I appreciate the suggestion of the instance method but then I'll end up with a gazillion of those methods that I need to also test and all of a sudden RSpec is forcing me to write code that I shouldn't have to.

[JonRowe]

So use my first suggestion and add the extra line of test, its also worth noting the should syntax will continue to work in RSpec 3, so you're welcome to continue using stub_chain as is.

[myronmarston]

I'm on the fence about this. On the one hand, I consider stub_chain a code smell. But that doesn't mean there aren't valid uses for it. It's not RSpec's job to decide for you what uses of this construct are and are not a problem. We left it off of the new syntax largely because I wanted to see how much demand there was for it. You're the first to request bringing it forward.

its also worth noting the should syntax will continue to work in RSpec 3, so you're welcome to continue using stub_chain as is.

If our suggestion to users is "keep using the old should syntax if you want this feature", then I think we should bring it forward, because I don't want to recommend users keep using that syntax.

[mhenrixon]

@JonRowe what about when I want to make sure that the following takes place with the right parameters?

def create_transaction(options = {})
   # do some processing with options.merge!({some other stuff})
   account.transactions.create! options
end

In a situation like that I don't care about what is done to collect the arguments I just care that the activerelation's create! method is hit with the right arguments. Sure you could argue that I do the extra setup to verify this but why should I have to? You could also argue that I create the transaction like in the last example I gave in my original post but I consider that to be more of a code smell than going through the active relation. That code is much harder to understand. In the code above anyone seeing that can easily understand that the transaction created belongs to whatever account. If you hide that in a merge somewhere the complexity increases a whole lot so I am perfectly fine violating the law of LoD in situations like these.

Using stub_chain is not an option, that means I have to turn should syntax on and will end up having to get rid of other peoples should expectations all over again. That is something I really don't want.

[JonRowe]

allow(account).to receive(:transactions)
expect(account.transactions).to receive(:create!).with(options)

[mhenrixon]

@JonRowe that is such an inconvenient way of writing that expectation. Had it not been so incredibly difficult to understand how the mock expectations work under the hood I would have written the code myself which I consider a truly good argument for why it should be available in the framework. Forcing people to write that extra allow statement when it wasn't needed before is not good enough in my humble opinion.

[fables-tales]

Fwiw I think we should support:

allow(object).to receive_chained_messages(:m1, :m2, :m3)
expect(object).to receive_chained_messages(:m1, :m2, :m3)

and with and and_return on those as well.

[myronmarston]

The last thing I want is users choosing not to upgrade to the newer syntax specifically because of the lack of a stub_chain replacement. So I'm with Sam at this point. However, I think it should be called receive_message_chain rather than receive_chained_messages.

[mhenrixon]

I'm with @myronmarston on the name @samphippen it sounds better to me, easier flow and easier to remember but I am native Swedish so I'm not sure I am allowed to even have an opinion on that.

[JonRowe]

@samphippen is working on this in #467

[myronmarston]

Closing here since we'll be merging #467 very soon to address this :).