Use the differ to print diffs when args are mismatched between expectations and calls #751

fables-tales · 2014-08-03T10:45:29Z

Related #434. This uses the differ that is now in RSpec support to
perform argument diffing when arguments don't match.

Things worth noting/still to do:

Special case behaviour needs to be added for the no arguments matcher
and the any arguments matcher, at the moment something like this:

       @@ -1,2 +1,2 @@
       -[#<RSpec::Mocks::ArgumentMatchers::NoArgsMatcher:0x000001024ec728>]
       +[{:bees=>:foo}]

gets printed, and that's obviously not great.

Need to work out what to do in the "similar args" case where there are
potentially multiple calls. I think the best thing to do would be to
would be to only diff if there's one call. (@myronmarston maybe has thoughts?)

There's some refactoring that could be done here but I want to finish
implementing the "similar args" implementation before I do that so that
I'm sure that I'm refactoring the right thing.

Also: needs more specs.

JonRowe · 2014-08-04T09:25:36Z

lib/rspec/mocks/error_generator.rb

+      end
+
+      def diff_message(expected_args, actual_args)
+        RSpec::Support::Differ.new(:color => true).diff(actual_args, expected_args)


Colour shouldn't be on by default, and having it based off the configuration would simply the spec below too :p

Agreed...rspec-expectations exposes a color config option if rspec-core isn't loaded, and otherwise simply delegates to rspec-core's config if it is:

https://github.com/rspec/rspec-expectations/blob/bfb7961851451e2775afc88a89ce721d6bf2cdea/lib/rspec/expectations/configuration.rb#L60-L76

JonRowe · 2014-08-04T09:26:45Z

Looks fairly simple, nice :)

myronmarston · 2014-08-04T17:46:59Z

Special case behaviour needs to be added for the no arguments matcher and the any arguments matcher, at the moment something like this:
@@ -1,2 +1,2 @@
-[#RSpec::Mocks::ArgumentMatchers::NoArgsMatcher:0x000001024ec728]
+[{:bees=>:foo}]
gets printed, and that's obviously not great.

Can't this use the description of the arg matcher?

Need to work out what to do in the "similar args" case where there are potentially multiple calls. I think the best thing to do would be to would be to only diff if there's one call. (@myronmarston maybe has thoughts?)

Hmmm...I'd have to see example output from the different options before forming an opinion. Overall, I'd say I'd err on the side of only adding diffing to places where it definitely makes sense; we can always add additional diffing later in the future.

fables-tales · 2014-08-13T08:53:43Z

I'm gonna be trying to finish this off over the coming weekend \o/

myronmarston · 2014-08-13T14:59:07Z

I think you also need to add diff-lcs as a runtime dependency of rspec-mocks.

JonRowe · 2014-08-13T23:03:54Z

Yep, if we're using the differ from support.

fables-tales · 2014-08-16T12:11:18Z

I added some specs with output. @myronmarston @JonRowe got thoughts?

JonRowe · 2014-08-16T16:46:40Z

Looks good to me, except it doesn't seem to work on 1.8.7?

fables-tales · 2014-08-16T16:48:18Z

1.8.7 is ordering the hashes differently. I'll take a look at it when I get the chance.

JonRowe · 2014-08-16T16:56:00Z

It's moaning about objects too isn't it? Also on 1.8.7 hashes aren't ordered, we deal with that somewhere by generating the string from the hash so the ordering is the same.

myronmarston · 2014-08-21T11:33:17Z

lib/rspec/mocks/error_generator.rb

+
+      def diff_message(expected_args, actual_args)
+        formatted_expected_args = expected_args.map do |x|
+          if x.respond_to?(:description)


Checking respond_to?(:description) is problematic because it could be something like a BlogPost AR instance that has a description attribute which is a blank string or nil...we can't count on it being a matcher in that case.

I think this should also check if it's a matcher using Support.is_a_matcher?:

https://github.com/rspec/rspec-support/blob/0fddfffd73917d2c6d6675a88d702e952cd8f27e/lib/rspec/support/matcher_definition.rb#L27-L29

fables-tales · 2014-08-24T11:14:34Z

I can't get this to fail locally on 1.8.7

fables-tales · 2014-08-24T11:21:07Z

Yolo pushed a fix.

fables-tales · 2014-08-24T11:24:20Z

Hey it passed!

JonRowe · 2014-08-24T16:16:59Z

1.8.7 doesn't like your last commit again :/

fables-tales · 2014-08-24T16:17:54Z

I had a look locally but I didn't get anywhere concrete. Going to try again tomorrow.

fables-tales · 2014-08-26T23:54:08Z

@myronmarston good point. I'll do that :)

fables-tales · 2014-08-26T23:54:45Z

I'm having some trouble debugging this 1.8.7 failure but I'm going to give it another stab on my upcoming holiday (first one in 9 months I am so excite)

myronmarston · 2014-08-28T15:41:28Z

spec/rspec/mocks/double_spec.rb

@@ -257,7 +257,7 @@ def @double.method_with_default_argument(arg={}); end
          expect {
            @double.method_with_default_argument(nil)
            verify @double
-          }.to raise_error(RSpec::Mocks::MockExpectationError, "Double \"test double\" received :method_with_default_argument with unexpected arguments\n  expected: ({})\n       got: (nil)")
+          }.to raise_error(RSpec::Mocks::MockExpectationError, "Double \"test double\" received :method_with_default_argument with unexpected arguments\n  expected: ({})\n       got: (nil) \e[0m\n\e[0m\e[34m@@ -1,2 +1,2 @@\n\e[0m\e[31m-[{}]\n\e[0m\e[32m+[nil]\n\e[0m")


The diff isn't really the point in this spec, so I'd prefer it not leak into the expectation here as it's brittle. Instead, you can pass a_string_starting_with(...) as the string arg and only include the part of the failure message that was there before.

myronmarston · 2014-08-28T15:42:36Z

I kicked the build and left a couple comments.

myronmarston · 2014-09-03T21:41:25Z

@samphippen where are you at on this? My 3.1 blog post is almost ready...

fables-tales · 2014-09-04T07:57:21Z

@myronmarston I'm gonna get this done today whilst travelling to berlin.

fables-tales · 2014-09-04T08:08:17Z

Gonna have to fix a bunch of rubocop violations now that I've merged master into this.

fables-tales · 2014-09-04T11:19:53Z

I can't reproduce this jruby failure locally, can anyone take a look at it?

myronmarston · 2014-09-04T18:01:54Z

lib/rspec/mocks/error_generator.rb

@@ -226,7 +245,7 @@ def arg_list(*args)
      end

      def arg_has_valid_description(arg)
-        return false unless arg.respond_to?(:description)
+        return false unless RSpec::Support.is_a_matcher?(arg)

        !arg.description.nil? && !arg.description.empty?
      end


The !arg.description.nil? && !arg.description.empty? logic is no longer necessary -- it was an earlier attempt to deal with non-matcher args that responded to description, but now that we check if it's a matcher we don't need that logic anymore. I'd just remove the arg_has_valid_description method altogether and replace it with a call to RSpec::Support.is_a_matcher?.

Actually, I just remembered that description is an optional part of the rspec matcher interface, so a valid matcher may not respond to description. Thus, I think we actually do need to keep this method (although, it should be renamed to have a ? on the end...), and its logic should be:

RSpec::Support.is_a_matcher?(arg) && arg.respond_to?(:description)

Your diff_message method above should then use this method as well.

fables-tales · 2014-09-23T17:20:28Z

I've been slowly addressing the feedback. I need to do one more pass and get it mergable but then it should be fine. I'll aim to have it all done this weekend. As a note: I think from now on I'd always prefer in-pull request feedback because github does not collapse commit feedback even if it's been addressed.

JonRowe · 2014-09-23T21:37:33Z

@samphippen it does, but only if the previous 5 (from memory) lines have changed

fables-tales · 2014-10-05T22:16:11Z

I'm sorry I haven't gotten to this recently. Some things have gone a little awry at my company and it's keeping me very occupied. If someone else wants to punt this over the finish line, go for it. I'll get to this as soon as I can.

fables-tales · 2014-10-11T13:07:40Z

@myronmarston it looks like github has collapsed all of your feedback. Can you check this over once more (please leave comments on the PR itself and not the commits). I'm also going to look into this JRuby failure.

This fixes #685. It means that we only print descriptions in errors for matcher objects and not for user objects.

myronmarston · 2014-10-12T03:28:32Z

Changelog.md

+
+* Print diffs when arguments in mock expectations are mismatched.
+  (Sam Phippen, #751)
+


You're changelog entry is in the 3.1.2 section. It should go in the 3.2.0 section.

Also I think you fixed #685, right? That probably warrants a call-out in the changelog as well.

fables-tales · 2014-10-18T12:15:51Z

Gonna push to get this done on sunday.

fables-tales · 2014-10-19T10:44:08Z

@myronmarston how's this looking?

JonRowe · 2014-10-19T22:58:46Z

lib/rspec/mocks/error_generator.rb

-                "unexpected arguments\n  expected: #{expected_args}\n" \
-                "       got: #{actual_args}"
+        diff = diff_message(expectation.expected_args, args)
+        message = "#{intro} received #{expectation.message.inspect} #{unexpected_arguments_message(expected_args, actual_args)}"


Can you split these lines up again? We're looking to reduce our line length, not increase it :P

Agree. Not a merge blocker though.

JonRowe · 2014-10-19T23:04:24Z

Looks good to me although I'd like to split the lines up to help reduce our overall length.

myronmarston · 2014-10-20T15:59:19Z

Thanks, @samphippen! As @JonRowe said it would be good to re-split the lines to keep new or recently edited code under 80 chars as that's where we'd like to get eventually. Not a merge blocker, though, so I'm going to merge and maybe you can follow up by splitting those lines in a separate tiny PR.

Use the differ to print diffs when args are mismatched between expectations and calls

phiggins · 2014-11-28T18:57:50Z

Thanks for this @samphippen, @myronmarston, @JonRowe!

JonRowe reviewed Aug 4, 2014
View reviewed changes

myronmarston reviewed Aug 21, 2014
View reviewed changes

myronmarston mentioned this pull request Aug 28, 2014

Work around bug in 1.8.7's Method#to_proc. rspec/rspec-expectations#641

Merged

myronmarston reviewed Aug 28, 2014
View reviewed changes

fables-tales force-pushed the args-diffing branch from c51b2f7 to 7e1a938 Compare September 4, 2014 08:05

myronmarston reviewed Sep 4, 2014
View reviewed changes

Use is_a_matcher? in arg_has_valid_description

c55d11b

This fixes #685. It means that we only print descriptions in errors for matcher objects and not for user objects.

fables-tales force-pushed the args-diffing branch from ed4f247 to 2d7b8a7 Compare October 11, 2014 17:02

fables-tales pushed a commit that referenced this pull request Oct 11, 2014

Add a changelog entry for #751

5008f23

myronmarston reviewed Oct 12, 2014
View reviewed changes

Sam Phippen added 2 commits October 19, 2014 10:59

Implement diffing between args for mock expectations.

139473d

Add a changelog entry for #751

29ee51d

fables-tales force-pushed the args-diffing branch from 2d7b8a7 to 29ee51d Compare October 19, 2014 10:00

JonRowe reviewed Oct 19, 2014
View reviewed changes

myronmarston added a commit that referenced this pull request Oct 20, 2014

Merge pull request #751 from rspec/args-diffing

3da6cdc

Use the differ to print diffs when args are mismatched between expectations and calls

myronmarston merged commit 3da6cdc into master Oct 20, 2014

myronmarston deleted the args-diffing branch October 20, 2014 15:59

fables-tales mentioned this pull request Oct 21, 2014

Trim line length in error generator #802

Merged

fables-tales mentioned this pull request Nov 16, 2014

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

Closed

nevinera mentioned this pull request May 9, 2024

Show a diff for comparisons of arrays of one-line strings rspec/rspec-support#597

Draft


		* Print diffs when arguments in mock expectations are mismatched.
		(Sam Phippen, #751)

Use the differ to print diffs when args are mismatched between expectations and calls #751

Use the differ to print diffs when args are mismatched between expectations and calls #751

Conversation

fables-tales commented Aug 3, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonRowe commented Aug 4, 2014

myronmarston commented Aug 4, 2014

fables-tales commented Aug 13, 2014

myronmarston commented Aug 13, 2014

JonRowe commented Aug 13, 2014

fables-tales commented Aug 16, 2014

JonRowe commented Aug 16, 2014

fables-tales commented Aug 16, 2014

JonRowe commented Aug 16, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fables-tales commented Aug 24, 2014

fables-tales commented Aug 24, 2014

fables-tales commented Aug 24, 2014

JonRowe commented Aug 24, 2014

fables-tales commented Aug 24, 2014

fables-tales commented Aug 26, 2014

fables-tales commented Aug 26, 2014

Choose a reason for hiding this comment

myronmarston commented Aug 28, 2014

myronmarston commented Sep 3, 2014

fables-tales commented Sep 4, 2014

fables-tales commented Sep 4, 2014

fables-tales commented Sep 4, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fables-tales commented Sep 23, 2014

JonRowe commented Sep 23, 2014

fables-tales commented Oct 5, 2014

fables-tales commented Oct 11, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fables-tales commented Oct 18, 2014

fables-tales commented Oct 19, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonRowe commented Oct 19, 2014

myronmarston commented Oct 20, 2014

phiggins commented Nov 28, 2014