ErrorGenerator#arg_list interferes with arg#description methods

[olivierlacan]

I'm having a hard time diagnosing this issue I'm having (running 2.14.6 on Rails 3.1.12).

When I set the following expectation:

expect(SubscriptionMailer).to receive(:cancel_subscription).with(subscription.owner, subscription.next_bill_on)

I receive the following error:

RSpec::Mocks::MockExpectationError: <SubscriptionMailer (class)> received :cancel_subscription with unexpected arguments
  expected: (, Fri, 27 Jun 2014 15:52:06 UTC +00:00)
       got: (#<User id: 1, ...>, "June 27th, 2014")
from /Users/olivierlacan/.rbenv/versions/1.9.3-p545/lib/ruby/gems/1.9.1/gems/rspec-mocks-2.14.6/lib/rspec/mocks/error_generator.rb:144:in `__raise'

If I save the expectation, the expected arguments are indeed:

=> [#<User id: 1, ...>,
 Fri, 27 Jun 2014 15:52:06 UTC +00:00]

Any clue as to what could impact the expectation like that?

[myronmarston]

What do you mean by "save the expectation"?

[olivierlacan]

@myronmarston Into a variable.

[olivierlacan]

Found the issue I think, although the expect output missing the first argument prevented me from noticing it at first:

Fri, 27 Jun 2014 15:52:06 UTC +00:00 vs. `"June 27th, 2014"

@myronmarston I'll let you close but it does seem like weird error feedback.

[myronmarston]

Can you show an example of what you mean?

[myronmarston]

@myronmarston I'll let you close but it does seem like weird error feedback.

It's still unclear to me what the issue is you are reporting. I'm not trying to be obtuse...can you paste a reproducible example?

[olivierlacan]

Only way I could reproduce this was with an ActiveRecord object.

the_object = double("the object")
subscription = double("the subscription")
owner = User.create # had to use an AR model here, didn't work with doubles

allow(subscription).to receive(:owner).and_return(owner)
allow(subscription).to receive(:next_charge_on).and_return(1.month.from_now.in_time_zone)

expect(the_object).to receive(:die).with(subscription.owner, subscription.next_charge_on)

subscription_with_weird_date = double("a subscription", next_charge_on: 1.month.from_now.in_time_zone.to_date.to_formatted_s(:long_ordinal))

the_object.die(owner, subscription_with_weird_date.next_charge_on)

Here, subscription_with_weird_date represents a piece of production code which changes the behavior of next_charge_on so that the spec is now out of date.

This is the output:

RSpec::Mocks::MockExpectationError: Double "the object" received :die with unexpected arguments
  expected: (, Fri, 27 Jun 2014 18:46:25 UTC +00:00)
       got: (#<User id: 5, ...>, "June 27th, 2014")
from /Users/olivierlacan/.rbenv/versions/1.9.3-p545/lib/ruby/gems/1.9.1/gems/rspec-mocks-2.14.6/lib/rspec/mocks/error_generator.rb:144:in `__raise'

[olivierlacan]

Welp, figured it out further. Our User class defines a description instance method and while rspec-mocks' ErrorGenerator class tests the expected arguments to see if they respond to description. My User argument does, but the return value in this context is "".

Maybe rspec-mocks shouldn't expect such a common method name? Why not mock_description?

See:

rspec-mocks/lib/rspec/mocks/error_generator.rb

Line 204 in dbd5533

args.collect {|arg| arg_has_valid_description(arg) ? arg.description : arg.inspect }.join(", ")

[myronmarston]

Maybe rspec-mocks shouldn't expect such a common method name? Why not mock_description?

Because rspec-mocks supports any custom matcher (using the matcher protocol defined by rspec-expectations) for with, and the method on the matcher protocol in description.

You're right that using description is problematic, though. If I had realized this issue earlier, I probably would have made the change to rspec_description (rather than description) as part of the matcher protocol changes that went into 3.0. At this point, we're at 3.0.0.rc1 and planning to release 3.0 final this weekend, and even though this is an outstanding issue, I don't think that we should change it -- it would cause more pain to make 3rd parties update their matchers again, rather than dealing with this relatively rare edge case.

I've got some thoughts on how to fix it, though: we can do a check using a method like this:

https://github.com/rspec/rspec-expectations/blob/1dce0c78b1a19f0d328d9574d7b3e4201e269b3f/lib/rspec/matchers.rb#L908-L915

...and only use description if the object appears to be a matcher.

[olivierlacan]

@myronmarston That sounds like a good non-API breaking way to do that. I agree with you, it's pretty edgy case even if I can see it bitting other people unexpectedly.

Want me to submit a PR? Or you want to tackle it?

[myronmarston]

Want me to submit a PR? Or you want to tackle it?

You are welcome to :). It's a bit more complicated then it appears at first, though:

• The is_a_matcher? method is in rspec-expectations but this is rspec-mocks and it can't rely on that. We should probably move it to rspec-support so it can be used from both gems.
• The argument matchers in rspec-mocks' are not full matchers in the sense that rspec-expectations looks for -- so we'll have to do something about them so that is_a_matcher? returns true for them. One idea is to do Reconcile duplicatated (but slightly different) matchers between rspec-mocks and rspec-expectations #513, and move the arg matchers into rspec-support and then rspec-mocks and rspec-expectations can both use them, and they'd be a full rspec-expectations matcher in order to be usable there.