Add expectation diff logs #1203

jdgreenberger · 2016-12-07T21:22:31Z

Purpose

Addresses issue #1179 by using npm libraries "diff" and "colors" to display colored diffs when arguments to sinonSpy.calledWith do not match the expectation.

Background

Took a stab at implementing diffs for the base case of calledWith. In order to limit changes in this PR, there's a temporary hack to use the original formatting when arguments include sinon matchers. Would appreciate feedback on a better way to implement this, and any other architectural improvements.

Solution

Creates a new formatting type %D on spyApi.formatters in spy.js to take the diff of expected and called arguments and created a useful colored message.

Example:

expected doSomething to be called with arguments 
[{
  first: "a",
-  mismatchKey: true,
+  mismatchKeyX: true,
  second: { nest: true },
-  third: [{ fourth: { nest: true } }]
+  third: [{ fourth: { nest: false } }]
}, "fifth"]

How to verify -

Check the added tests in assert-test.js

Check out this branch
npm install
mocha test/assert-test.js

coveralls · 2016-12-07T23:06:30Z

Coverage increased (+0.01%) to 96.332% when pulling 26b715e on jdgreenberger:add-expectation-diff-logs into f6e7916 on sinonjs:master.

coveralls · 2016-12-07T23:07:57Z

Coverage increased (+0.01%) to 96.332% when pulling 26b715e on jdgreenberger:add-expectation-diff-logs into f6e7916 on sinonjs:master.

jdgreenberger · 2016-12-16T23:55:25Z

@mroderick excited to get this in! Let me know what needs to be changed 😃

fatso83

This absolutely seems like a nice addition. I just need to understand all the changes and why :-)

fatso83 · 2016-12-17T10:17:33Z

lib/sinon/assert.js

@@ -78,6 +78,11 @@ function mirrorPropAsAssertion(name, method, message) {
        var args = slice.call(arguments, 1);
        var failed = false;

+        // Hack to be replaced


This seems like a TODO statement. No one ever does anything to a TODO statement. Could you rather expand on your comment? I am not totally sure what this does.

Lines 82-84 check for one of the sinon match objects, and if found, revert to using the original message formatting without colored diffs. I did this to avoid making a lot of changes in a single PR, and because the choice to print a diff for a sinon match comparison is a little bit more ambigious than comparing two plain objects (What do I print for expect({x: true}).to.be.calledWith(match.falsy)?). I was hoping one of the contributors could suggest a less hacky way to reserve the diff formatting just for plain object comparison.

If this is explicitly to catch matchers, can you use the built-in sinonMatch.isMatcher(...) on line 82?

Can you update the comment to say why you're doing this (like some of the posted comment above) instead of this one?

Yes! Exactly what I was looking for, thank you 👍

startswithaj · 2016-12-20T13:25:10Z

@jdgreenberger great work on this. LGTM.

fearphage · 2016-12-20T13:36:16Z

lib/sinon/assert.js

@@ -78,6 +78,11 @@ function mirrorPropAsAssertion(name, method, message) {
        var args = slice.call(arguments, 1);
        var failed = false;

+        // Hack to be replaced


If this is explicitly to catch matchers, can you use the built-in sinonMatch.isMatcher(...) on line 82?

Can you update the comment to say why you're doing this (like some of the posted comment above) instead of this one?

fearphage · 2016-12-20T13:37:03Z

lib/sinon/assert.js

@@ -199,7 +204,7 @@ mirrorPropAsAssertion(
 );
 mirrorPropAsAssertion("calledWithNew", "expected %n to be called with new");
 mirrorPropAsAssertion("alwaysCalledWithNew", "expected %n to always be called with new");
-mirrorPropAsAssertion("calledWith", "expected %n to be called with arguments %*%C");
+mirrorPropAsAssertion("calledWith", "expected %n to be called with arguments %D");


Why only calledWith? Why not calledWithExactly and others?

coveralls · 2016-12-20T19:45:03Z

Coverage increased (+0.06%) to 96.374% when pulling f67ab71 on jdgreenberger:add-expectation-diff-logs into f6e7916 on sinonjs:master.

coveralls · 2016-12-20T19:45:03Z

Coverage increased (+0.06%) to 96.374% when pulling f67ab71 on jdgreenberger:add-expectation-diff-logs into f6e7916 on sinonjs:master.

coveralls · 2016-12-20T19:49:08Z

Coverage increased (+0.06%) to 96.374% when pulling 4af2809 on jdgreenberger:add-expectation-diff-logs into f6e7916 on sinonjs:master.

jdgreenberger · 2016-12-20T19:49:13Z

@fearphage knowing about .isMatcher, it wasn't much harder to use colored diffs for the matchers. Updated PR to apply formatting to other relevant calledWith methods
@fatso83

fatso83 · 2016-12-21T08:39:40Z

This now looks to be in great IMHO! Could I ask you just to clean up the commit history before merging? Just do an interactive rebase, or whatever you prefer, to get rid of the cleanup log commits.

PS. I am not asking you to squash all the commits. They tell a good story.

jdgreenberger · 2016-12-21T16:23:03Z

@fatso83 updated!

coveralls · 2016-12-21T16:24:44Z

Coverage increased (+0.02%) to 96.374% when pulling 6f346b3 on jdgreenberger:add-expectation-diff-logs into 1924a89 on sinonjs:master.

fatso83 · 2016-12-21T16:26:19Z

Perfect! Thanks for the work on this.

jdgreenberger · 2016-12-21T16:32:45Z

No problem! Thanks for the review 😄

* See [Sinon #1203](sinonjs/sinon#1203)

* See sinonjs/sinon#1203

Closes #104. Follows sinonjs/sinon#1203 mostly, although we still include the %* arguments list, as doing otherwise is a bit tricky for the negated case.

jdgreenberger closed this Dec 7, 2016

jdgreenberger reopened this Dec 7, 2016

fatso83 requested changes Dec 17, 2016

View reviewed changes

fearphage requested changes Dec 20, 2016

View reviewed changes

jdgreenberger and others added 4 commits December 21, 2016 08:19

Add new spy output formatter for handling diffs

c23c530

Print diffs for multiple spy calls

4160e14

Color diffs for sinon matchers

92f76fa

Convert remaining calledWith methods to use diff color formatting

6f346b3

jdgreenberger force-pushed the add-expectation-diff-logs branch from 4af2809 to 6f346b3 Compare December 21, 2016 16:21

fatso83 merged commit a0c77c0 into sinonjs:master Dec 21, 2016

fatso83 mentioned this pull request Jan 13, 2017

__defineGetter__ is not supported in IE9 & IE10 #1244

Closed

coderaiser mentioned this pull request Apr 6, 2017

Called with diff doesn't work #1376

Closed

BuiltByWalsh mentioned this pull request Jul 17, 2017

[feature-request] add chai colored diff to "calledWith" expectations chaijs/sinon-chai#93

Closed

BuiltByWalsh pushed a commit to BuiltByWalsh/sinon-chai that referenced this pull request Jul 17, 2017

Add improved diffing for calledWith (chaijs#93)

58b05e6

* See [Sinon #1203](sinonjs/sinon#1203)

BuiltByWalsh pushed a commit to BuiltByWalsh/sinon-chai that referenced this pull request Jul 17, 2017

Add improved diffing for calledWith (chaijs#93)

b923d50

* See sinonjs/sinon#1203

BuiltByWalsh mentioned this pull request Jul 17, 2017

Add improved diffing for calledWith chaijs/sinon-chai#104

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add expectation diff logs #1203

Add expectation diff logs #1203

jdgreenberger commented Dec 7, 2016 •

edited

Loading

coveralls commented Dec 7, 2016 •

edited

Loading

coveralls commented Dec 7, 2016 •

edited

Loading

jdgreenberger commented Dec 16, 2016

fatso83 left a comment •

edited

Loading

fatso83 Dec 17, 2016

jdgreenberger Dec 18, 2016 •

edited

Loading

fearphage Dec 20, 2016

jdgreenberger Dec 20, 2016

startswithaj commented Dec 20, 2016

fearphage Dec 20, 2016

fearphage Dec 20, 2016

coveralls commented Dec 20, 2016 •

edited

Loading

coveralls commented Dec 20, 2016

coveralls commented Dec 20, 2016 •

edited

Loading

jdgreenberger commented Dec 20, 2016 •

edited

Loading

fatso83 commented Dec 21, 2016

jdgreenberger commented Dec 21, 2016

coveralls commented Dec 21, 2016 •

edited

Loading

fatso83 commented Dec 21, 2016

jdgreenberger commented Dec 21, 2016

Add expectation diff logs #1203

Add expectation diff logs #1203

Conversation

jdgreenberger commented Dec 7, 2016 • edited Loading

Purpose

Background

Solution

How to verify -

coveralls commented Dec 7, 2016 • edited Loading

coveralls commented Dec 7, 2016 • edited Loading

jdgreenberger commented Dec 16, 2016

fatso83 left a comment • edited Loading

Choose a reason for hiding this comment

fatso83 Dec 17, 2016

Choose a reason for hiding this comment

jdgreenberger Dec 18, 2016 • edited Loading

Choose a reason for hiding this comment

fearphage Dec 20, 2016

Choose a reason for hiding this comment

jdgreenberger Dec 20, 2016

Choose a reason for hiding this comment

startswithaj commented Dec 20, 2016

fearphage Dec 20, 2016

Choose a reason for hiding this comment

fearphage Dec 20, 2016

Choose a reason for hiding this comment

coveralls commented Dec 20, 2016 • edited Loading

coveralls commented Dec 20, 2016

coveralls commented Dec 20, 2016 • edited Loading

jdgreenberger commented Dec 20, 2016 • edited Loading

fatso83 commented Dec 21, 2016

jdgreenberger commented Dec 21, 2016

coveralls commented Dec 21, 2016 • edited Loading

fatso83 commented Dec 21, 2016

jdgreenberger commented Dec 21, 2016

jdgreenberger commented Dec 7, 2016 •

edited

Loading

coveralls commented Dec 7, 2016 •

edited

Loading

coveralls commented Dec 7, 2016 •

edited

Loading

fatso83 left a comment •

edited

Loading

jdgreenberger Dec 18, 2016 •

edited

Loading

coveralls commented Dec 20, 2016 •

edited

Loading

coveralls commented Dec 20, 2016 •

edited

Loading

jdgreenberger commented Dec 20, 2016 •

edited

Loading

coveralls commented Dec 21, 2016 •

edited

Loading