No showDiff set on assert interface

[timruffles]

Just wondering why? Happy to PR this in. It's a bit of a shame this behaviour magically stopped working.

e.g

    test.assert(
        exp == flag(test, 'object')
      , 'expected #{this} to equal #{exp}'
      , 'expected #{this} to not equal #{act}'
      , exp
      , act
// <- showDiff true not passed
    );

[keithamus]

Hey @timruffles thanks for the issue.

I assume you are saying that at one point you didn't need to pass in true as the showDiff argument? As far as I can see the behaviour has been the same for over a year - unless something changed somewhere else which effected this?

However (if Im right in what you're saying), I do agree that passing exp and act should be enough to make showDiff be true. So feel free to raise a PR, but since it's a breaking change could you please raise it under the 4.x.x branch, and we'll get it merged in for the next major release.

Thanks 😄

[keithamus]

To summarise what should change, rather than L101 currently being:

if (true !== showDiff) showDiff = false;

It should be changed to

if (false !== showDiff) showDiff = true;
if (undefined === expected && undefined === _actual) showDiff = false;

And you'll need to add tests reflecting this change - in the chai/test/assert.js file

[lucasfcosta]

Hi @keithamus, would you mind taking a look at what I've thought could be done to test this?

  it.only('passing exp and act makes showDiff be true', function() {
    try {
      assert.equal('one', 'two');
    } catch(e) {
      assert.isTrue(e.showDiff);
    }
  });

Unfortunately this works on node but it doesn't on karma.
On node expected is 'two' and actual is 'one' and everything works fine, but on karma expected is true and actual is undefined.

I have no idea why this happens, apparently this should work correctly on both platforms.

[lucasfcosta]

Well, I'm about to surrender, @keithamus, I have no idea why my tests work on Node but don't on Karma.

The only change I've made was the one you suggested and my test is as I described on the comment above.

On node everything runs fine, but on karma the test called getMessage template tag substitution fails (I have no idea why, because showDiff has nothing to do with it) and my other test to ensure my change is working fails too.

Is there a difference between node and browsers that could make this happen?
I think my syntax works the same on both...

[qbolec]

Hi, I've tried to reproduce what you describe, and I see quite a different picture.

What I see is this:

C:\Users\qbolec\Documents\GitHub\chai>make test-node
==> [Test :: Node.js]

expected: two
actual: one
{ message: 'expected \'one\' to equal \'two\'',
  showDiff: true,
  actual: 'one',
  expected: 'two' }
expected: true
actual: undefined

  .

  1 passing (7ms)

That is: the node is visiting assert twice .. which is not surprising since we have two assertions in:

 try {
      assert.equal('one', 'two'); // <--- first assertion: one vs. two
    } catch(e) {
      assert.isTrue(e.showDiff); // <-- second assertion: true vs undefined
    }

When using karma:

C:\Users\qbolec\Documents\GitHub\chai>make test-phantom
==> [Browser :: build]
==> [Test :: Karma (PhantomJS)]
INFO [karma]: Karma v0.12.37 server started at http://localhost:9876/
INFO [launcher]: Starting browser PhantomJS
INFO [PhantomJS 1.9.8 (Windows 8 0.0.0)]: Connected on socket BHNaQRHiHfEQmFKCt1
9Q with id 93100303
LOG: 'expected: two'
LOG: 'actual: one'
LOG: AssertionError{message: 'expected 'one' to equal 'two'', showDiff: true, ac
tual: 'one', expected: 'two', stack: 'AssertionError: expected 'one' to equal 't
wo'
    at http://localhost:9876/base/chai.js?56ff911da279b5418caf8bb4cafeb8f4fed851
32:213
    at http://localhost:9876/base/chai.js?56ff911da279b5418caf8bb4cafeb8f4fed851
32:2287
    at http://localhost:9876/base/test/assert.js?285ef96253d23e29d96246953cc18d9
1ce491363:1051
    at callFn (http://localhost:9876/base/node_modules/mocha/mocha.js?e2869adc1f
343c52a99a2c23eaa29bb493769d13:4202)
    at http://localhost:9876/base/node_modules/mocha/mocha.js?e2869adc1f343c52a9
9a2c23eaa29bb493769d13:4195
    at http://localhost:9876/base/node_modules/mocha/mocha.js?e2869adc1f343c52a9
9a2c23eaa29bb493769d13:4661
    at http://localhost:9876/base/node_modules/mocha/mocha.js?e2869adc1f343c52a9
9a2c23eaa29bb493769d13:4790
    at next (http://localhost:9876/base/node_modules/mocha/mocha.js?e2869adc1f34
3c52a99a2c23eaa29bb493769d13:4581)
    at http://localhost:9876/base/node_modules/mocha/mocha.js?e2869adc1f343c52a9
9a2c23eaa29bb493769d13:4591
    at next (http://localhost:9876/base/node_modules/mocha/mocha.js?e2869adc1f34
3c52a99a2c23eaa29bb493769d13:4523)
    at http://localhost:9876/base/node_modules/mocha/mocha.js?e2869adc1f343c52a9
9a2c23eaa29bb493769d13:4559
    at timeslice (http://localhost:9876/base/node_modules/mocha/mocha.js?e2869ad
c1f343c52a99a2c23eaa29bb493769d13:12326)', line: 213, sourceId: 89954768, source
URL: 'http://localhost:9876/base/chai.js?56ff911da279b5418caf8bb4cafeb8f4fed8513
2', stackArray: [Object{sourceURL: ..., line: ...}, Object{sourceURL: ..., line:
 ...}, Object{sourceURL: ..., line: ...}, Object{function: ..., sourceURL: ...,
line: ...}, Object{sourceURL: ..., line: ...}, Object{sourceURL: ..., line: ...}
, Object{sourceURL: ..., line: ...}, Object{function: ..., sourceURL: ..., line:
 ...}, Object{sourceURL: ..., line: ...}, Object{function: ..., sourceURL: ...,
line: ...}, Object{sourceURL: ..., line: ...}, Object{function: ..., sourceURL:
..., line: ...}]}
LOG: 'expected: true'
LOG: 'actual: undefined'
PhantomJS 1.9.8 (Windows 8 0.0.0): Executed 1 of 1 SUCCESS (0 secs / 0.002 secs)
PhantomJS 1.9.8 (Windows 8 0.0.0): Executed 1 of 1 SUCCESS (0.012 secs / 0.002 s
ecs)

So, again, we have two separate assertions reported.

I've noticed a problem with Makefile though: make test-phantom states that it requires chai.js which is fine, but the rule for chai.js mentions only node_modules lib/* as prerequisites, which is simply wrong, as it does not take into account files deeper in the structure like lib/chai/assertions.js which you have edited. So in order to see any changes in the output of karma I had to manually rm chai.js before running make test-phantom.

[lucasfcosta]

Thank you, @qbolec!
That was well noticed. I've had to remove chai.js and run make test again and then it worked!

What I've described before was probably happening because Karma was using the old chai.js file (the one at the repositories' root) for the test (as described above by @qbolec).

EDIT: I've just seen this is written into the file that talks about contributions, sorry for not paying enough attention to it 😢

[qbolec]

IMHO, the fact that rm chai.js is mentioned in CONTRIBUTING.md does not make it more reasonable :) The whole point of using make is to make life easier by keeping track of required steps and dependencies. That's why I would not expect to have to do anything more than typing make test-phantom to run tests for phantom. I also believe that this is a convention of npm, to make sure that npm test does all that is required to actually test something. Requiring people to dig into documentation just to learn that you have to run rm chai.js in order to run tests is strange.
I think this is simply a bug in the Makefile line:

chai.js: node_modules lib/*`

which should instead read:

chai.js: node_modules lib/* lib/*/* lib/*/*/*

I'm not sure how to tell make to search at arbitrary depth - the accepted solution to http://stackoverflow.com/questions/4036191/sources-from-subdirectories-in-makefile seems to not work on my version of make 4.1, that is $(wildcard lib/*/.js) matches only two files lib/chai/config.js lib/chai/assertion.js so I guess that double asterix does not really do what one assumes. Perhaps this is an issue with Windows.

[keithamus]

@qbolec you're totally right on that. Our build scripts are not doing a good enough job. Feel free to PR a change of the Makefile and CONTRIBUTING.md (that is, if you want to). Double asterisk isn't very well supported (e.g., not by default on Mac OSX), so I'd like for us to avoid using it if possible.

[lucasfcosta]

Hello everyone,
I've just sent a PR with the Makefile I was using when making these changes and for the rest of the issue: I think we can close it since everything's been done.

Thanks for the help!

[keithamus]

👍