Diff the arguments passed to expectations on failure, similar to how matchers do

[phiggins]

Recently I've had the idea that it would be useful if the arguments expected by the mocks set up with expect().to receive and friends were diffed similarly to how the arguments passed to matchers are.

Here's a short example of what I mean:

describe "bad error message" do
  it "does not diff arguments with mocked methods" do
    foo = double

    expect(foo).to receive(:a).with(foo: :bar, baz: :qux)

    foo.a(foo: :bar, baz: :qux, tacos: :please)
  end
end

describe "good error message" do
  it "diffs arguments with matchers" do
    expected = { foo: :bar, baz: :qux }

    actual = { foo: :bar, baz: :qux, tacos: :please }

    expect(actual).to eq(expected)
  end
end

When run, I see output like this:

FF

Failures:

  1) bad error message does not diff arguments with mocked methods
     Failure/Error: foo.a(foo: :bar, baz: :qux, tacos: :please)
       Double received :a with unexpected arguments
         expected: ({:foo=>:bar, :baz=>:qux})
              got: ({:foo=>:bar, :baz=>:qux, :tacos=>:please})
     # ./lib/rspec/mocks/error_generator.rb:162:in `__raise'
     # ./lib/rspec/mocks/error_generator.rb:26:in `raise_unexpected_message_args_error'
     # ./lib/rspec/mocks/proxy.rb:190:in `raise_unexpected_message_args_error'
     # ./lib/rspec/mocks/proxy.rb:172:in `message_received'
     # ./lib/rspec/mocks/method_double.rb:61:in `block (2 levels) in define_proxy_method'
     # ./test_diffs.rb:7:in `block (2 levels) in <top (required)>'

  2) good error message diffs arguments with matchers
     Failure/Error: expect(actual).to eq(expected)

       expected: {:foo=>:bar, :baz=>:qux}
            got: {:foo=>:bar, :baz=>:qux, :tacos=>:please}

       (compared using ==)

       Diff:
       @@ -1,3 +1,4 @@
        :baz => :qux,
       -:foo => :bar
       +:foo => :bar,
       +:tacos => :please
     # ./test_diffs.rb:17:in `block (2 levels) in <top (required)>'

Finished in 0.00145 seconds
2 examples, 2 failures

Failed examples:

rspec ./test_diffs.rb:2 # bad error message does not diff arguments with mocked methods
rspec ./test_diffs.rb:12 # good error message diffs arguments with matchers

It is much easier to spot the difference in the error message of the second test than it is in the first. This would come in handy when doing things like testing that the arguments passed to a collaborator are filtered or modified in a certain way. I've wanted this functionality for testing things like knowing that an object receives a certain subset of parameters passed as query arguments to a rails controller, for example.

I took a look at what it would take to either reimplement or share the code for the diffing stuff in rspec-expectations, and it seems very specific to implementing the error messages of the matchers. I'd be willing to take a crack at a patch for this if it's deemed a worthy addition and there was some guidance for how to proceed.

[fables-tales]

I can see how this would work, but we'd have to pull the differ from -expectations into -mocks and that strikes me as some dangerous duplication. @JonRowe do you think pulling the differ into -support to support this is a good idea?

[JonRowe]

At one point we talked about pulling the differ out into it's own gem, and allowing people to pick the differ they wanted, in which case we wouldn't want to support it -support but if we don't go down that route then I don't see why it shouldn't go in support.

[phiggins]

I've got branches of rspec-expectations and rspec-support where I've attempted to extract the diffing code into support:

https://github.com/phiggins/rspec-expectations/tree/extract-differ-to-support
https://github.com/phiggins/rspec-support/tree/extract-expectations-differ-to-support

It's a bit rough and doesn't have all the complexities worked out, but I thought I'd request some feedback before I put too much more time into it.

[myronmarston]

@phiggins: this general direction looks great. I had this slated for post 3.0 (e.g. 3.1 or whatever) but that's mostly a function of the limited time of the core team. Getting a contribution from a user like yourself for this is great!

[JonRowe]

Looks like a good start! Let us know when you want a proper review :)

[myronmarston]

One thing to be aware of is changes to the DiffPresenter coming in rspec/rspec-expectations#407. We'll probably be merging that today.

[phiggins]

Thanks for the heads up @myronmarston!

[phiggins]

I have a few general questions that you guys could help me with.

Some of the specs in RSpec projects use nested modules around the describe calls while others include the nesting in the describe call. Is there a preference project wide, or does it depend on the context? I generally prefer the latter and would use that if there's no preference, but I'm happy to do some cleanups if there's strong preferences either way.

DiffPresenter has two explicit dependencies on rspec-expectations: RSpec::Matchers.configuration.color? and RSpec::Matchers::Composable.surface_descriptions_in(object).

I thought I'd make this more reusable by changing the way DiffPresenter is created to something like this:

# in rspec-expectations
DiffPresenter.new(
  :color? => RSpec::Matchers.configuration.color?,
  :stringifier => lambda {|obj| RSpec::Matchers::Composable.surface_descriptions_in(obj) }
)

# in rspec-support
module RSpec
  module Support
    class DiffPresenter
      def initialize(opts)
        @color = opts.fetch(:color?, false)
        @stringifier = opts.fetch(:stringifier) { lambda {|obj| obj } }
      end
    end
  end
end

Does this seem like a good way to go?

I'm not crazy about the name DiffPresenter. It seems like this could be better organized by making Differ a private class of DiffPresenter called HunkGenerator or something, and renaming DiffPresenter to Differ, so it'd be called as RSpec::Support::Differ.blah. WDYT?

[JonRowe]

Personally I favour the latter but the former is more common. I'm happy for either usage tbh. As regards to naming I'm in favour of your proposed names.

[myronmarston]

Some of the specs in RSpec projects use nested modules around the describe calls while others include the nesting in the describe call. Is there a preference project wide, or does it depend on the context? I generally prefer the latter and would use that if there's no preference, but I'm happy to do some cleanups if there's strong preferences either way.

I prefer the former, as it allows you to reference constants without needing to fully qualify them. I think the majority of rspec's specs are written in this style. It's not a big deal either way, though.

Re: config color?: that exists in rspec-expectations only for the differ. It defaults to the rspec-core config option if used with that gem, and otherwise can be set by the user off the rspec-expectations config object. Given that it exists only for the differ and the differ is moving, maybe it should move as well? Although, rspec-support is intended only for internal use, and has no public APIs, and I don't want to start adding public APIs there...so I'm not sure what the best way is. More thought is needed...

The surface_descriptions_in method could potentially move to rspec-support as well, since rspec-mocks argument matchers could benefit from that logic if they are being diffed. There's some complexity around when to use description, though; in rspec-mocks it uses description for any arg if it responds to it and it's not blank (code). In rspec-expectations, it's stricker; it only uses description if it's a matcher that responds to description (code). One of the things that adds to the complexity is rspec/rspec-expectations#260: description is general enough that there can be domain objects (such as Article or Book or Course) that have a description attribute, but the contents of that attribute may not identify the object well. Again, I'm not sure what the best solution here is.

[phiggins]

Re: config color?: that exists in rspec-expectations only for the differ. It defaults to the rspec-core config option if used with that gem, and otherwise can be set by the user off the rspec-expectations config object. Given that it exists only for the differ and the differ is moving, maybe it should move as well? Although, rspec-support is intended only for internal use, and has no public APIs, and I don't want to start adding public APIs there...so I'm not sure what the best way is. More thought is needed...

To maintain the idea that rspec-expectations and rspec-mocks are self-contained modules that can be used outside of rspec, I think it makes sense for both of them to defer to RSpec's configuration but provide their own in case that doesn't exist. It does create a bit of duplication, but I'm not sure how, for example, rspec-mocks used outside of rspec would expose the color setting if it was only implemented in rspec-support.

[phiggins]

The surface_descriptions_in method could potentially move to rspec-support as well, since rspec-mocks argument matchers could benefit from that logic if they are being diffed.

The approach I was trying to make by passing in a stringifier proc was to avoid any implementation trickiness that depended on peculiarities of the instantiator. It makes sense to me for rspec-support to just punt that decision to the place where it is used and keep that dependency clean rather than try to handle all sorts of edge cases. Does my proposal above seem like a good enough solution to go forward?

[JonRowe]

I'm ok with a minor bit of duplication on the configuration objects, then having each gem use that to configure the differ. There might be some scope for support having a "config" decider though...

[phiggins]

I'm ok with a minor bit of duplication on the configuration objects, then having each gem use that to configure the differ. There might be some scope for support having a "config" decider though...

Outside the scope of this PR, but it might be cool to move the configuration object into support as well, then have expectations and mocks each try to reuse RSpec's configuration instance, creating their own if that doesn't exist.

[myronmarston]

Does my proposal above seem like a good enough solution to go forward?

Yep.

[JonRowe]

Off topic again but if we do anything with config I think we could move the core "how configuration is made" logic to support, and then define the keys supported by each gem in their own config classes, as although it's a minor bit of duplication it better shows the intent of the config in each gem.

[fables-tales]

Closing because of #751