[feature request] [discussion] Addings support for walaby.js

[hellboy81]

sinon-chai should set showDiff flag for AssertionError to make available to show the diff in walaby.js for.

See also walaby.js issue

[domenic]

I am pretty sure this is already taken care of by Sinon-Chai, which includes actual and expected properties. If walaby.js is making the bad decision to only show diffs for errors which have an additional showDiff property, then that's it's problem, and needs to be fixed in that library.

See also chaijs/chai#469. Tagging in @keithamus in case I got this wrong.

[keithamus]

Actually I'm pretty sure this is sinon-chai's issue.

Firstly, there is a showDiff which is meant to suppress diffs, when false, like we can see with Mocha. Chai has showDiff set to true, unless you explictly override it, and some engines will check to see if showDiff === false and suppress the diff. Generally speaking engines should only check if the flag is explicitly false.

So showDiff is a red-herring here. The actual problem, is that sinon-chai doesn't actually give chai the actual/expected properties, as far as I can read. If we take a look at sinonProperty it passes 3 arguments to this.assert - the assertion boolean, the affirmative message, and the negative message. Chai's this.assert takes 5 arguments, to quote:

In all, the assert method accepts five arguments:

a boolean (result of a truth test)
a string error message to be used if the first argument is false
a string error message to be used if the assertion is negated and the first argument is true
(optional) the expected value
(optional) the actual value, which will default to _obj

Sinon-chai isn't passing in expected (4th arg), which would be required to show up diffs at the very least. It should probably pass in actual (5th arg) too.

A quick test in the REPL proves as much:

> var error; try { chai.expect(sinon.spy()).to.be.called } catch(e) { error = e };
> error.expected
undefined
> error.actual
> error.actual.toString()
'spy'

[domenic]

Ah excellent, good catch and I'm glad I tagged you in :).

PR welcome if someone can figure this out, or I can try to make time for it. Although I have no idea what the actual/expected would be for an example like .to.be.called. I guess the args-comparison ones are the most straightforward.

[keithamus]

sinonProperty would probably not benefit from showing a diff - comparing true/false for .called or 1/2/3/n for .called[Once,Twice,Thrice] is kind of pointless. I'd suggest only passing in actual/expected to sinonMethod. Part of the reason why 4th/5th args to this.assert are optional is because you don't always have a good comparison to make 😉

[hellboy81]

I guess the args-comparison ones are the most straightforward

• For each arg in args:
  • Compare expected with actual
    • If arg is object
    • Each key-value pair should be compared
    • Also nested objects should be supported
  • Comparison result should be available both as text colored diff and also for walaby.js

[keithamus]

We shouldn't need to go that far. We can present the given args and actual args (as whole objects), and the test-runners diffing engine will handle the rest.

[hellboy81]

OK, this should be implemented by walaby.js itself?

[keithamus]

If wallaby.js supports chai diffs properly, then they need to do nothing else.

Sinon-chai needs to pass in expected and actual objects into some of its assertions. Sinon-chai should have no diffing logic, or in fact any loops or any computation of that kind. It just needs to pass additional arguments to chai - via this.assert.

[ArtemGovorov]

@keithamus

So showDiff is a red-herring here.

Is it? Correct me me if I'm wrong, but if Sinon-chai will only be fixed to pass expected and actual, without passing showDiff with true value, then Chai will just set it to false making mocha and wallaby.js to suppress showing the diff.

some engines will check to see if showDiff === false and suppress the diff. Generally speaking engines should only check if the flag is explicitly false.

Mocha checks if an assertion error nas showDiff !== false before calculating the diff, in wallaby.js I check if showDiff === true before doing so.

So if an assertion error has showDiff set to undefined, mocha will calculate and display the diff, while wallaby.js will not.

It seems more semantically valid to me that when some assertion framework throws an AssertionError and doesn't set showDiff, then its value should be treated as false and the diff should not be displayed. After all, undefined is falsy, so if the intention of the property is to signal when to suppress, then it'd probably be better to have suppressDiff property instead.

Could you please elaborate a bit on why you reckon "engines should only check if the flag is explicitly false"? Chai actually enforces the logic that I think is correct by setting showDiff to false unless the assertion error explicitly passes true for it.

[keithamus]

My understanding is that showDiff is meant for suppressing diffs. Maybe it doesn't have the best name, but we are where we are. I could be wrong here - because I'm not even sure where showDiff originated from, alls I know is Mocha explicitly checks it isn't false, because not every assertion comes with it set.

If you're only showing diffs when showDiff === true then you're missing out on diffs from other assertion libs, like Node's assert which does not have showDiff set:

> var error; try { assert.ok(false) } catch(e) { error = e; }
{ [AssertionError: false == true]
  name: 'AssertionError',
  actual: false,
  expected: true,
  operator: '==',
  message: 'false == true',
  generatedMessage: true }
> error.showDiff === true
false
> 'showDiff' in error
false

But actually, you're right. It was a mistake to call showDiff a red herring, because, as you rightly point out, assertions need to pass true to show a diff.

So ultimately, the change is that sinon-chai needs to change L95 to include the 4th, 5th and 6th arguments, being expected, actual and true respectively.

[ArtemGovorov]

If you're only showing diffs when showDiff === true then you're missing out on diffs from other assertion libs, like Node's assert which does not have showDiff set.

You are right. Some assertions libraries, such as node's assert, don't set it, some do set it.

One of the other reasons why I have decided to check showDiff === true was that I'm concerned about is how to avoid calculating diffs when it doesn't really make sense to display the diffs in the context of test runner.

For example in the case of node's assertion functions, there's really only one of them where calculating diffs makes sense in my opinion, it's deepEqual. For the rest, generated message seems more than enough. It's possibly not really flexible, but I'd rather check the operator field in addition to the showDiff === true check.

[hellboy81]

I am still waiting for feature

How can I visually compare

 AssertionError: expected create to have been called with arguments matching {
....
}

methodName({
   .....
})

it not very easy to compare per hand this 2 JSON outputs