migrate Mocha's tests to Unexpected assertion library

[boneskull]

These changes convert (nearly) all usages of expect.js and assert to unexpected.

Motivation:

1. Consistency
2. Better diffs
3. Custom assertions (see the new test/assertions.js) are easy to implement
4. Easier to avoid potentially throwing multiple errors in a single test w/ multiple assertions (see to satisfy)
5. Out-of-the-box support for Promise
6. Active development

• I removed the userland assert b/c as I understand the Node.js team is no longer discouraging the use of the built-in module.
• chai is added because we have a test that asserts compatibility, which was previously not-so-valuable, because we weren't actually testing against Chai itself. Whether or not we should remove this particular test is debatable.
• We now have custom assertions for handling the result of runMochaJSON in the integration tests
• test/unit/runnable.spec.js uses no assertion library. See my comments there and in the Runnable test is bonkers #3342.
• There's now a test/README.md that explains some things.

[boneskull]

should probably fix those windows-only tests.

[coveralls]

Coverage increased (+0.009%) to 90.018% when pulling 85c8f0d on experimental/unexpected into 1606f35 on master.

[Bamieh]

AFAIK chai/expect are the most familiar stack among developers, maybe this will raise the barrier of contributions?

This is the first time I see unexpected, but it seems interesting.

[boneskull]

I’m not too concerned about it creating friction. Even if it did, I feel the benefits of using it outweigh any negatives about the general unfamiliarity with it.

What we should do, however, is document the custom assertions and make it clear where the documentation for the base assertions live. If a contributor is familiar with Chai, it shouldn’t be too jarring, anyway.

That said, to someone unfamiliar with either, I’d argue Unexpected is fundamentally easier to understand than Chai, since it doesn’t do strange things with object properties:

// chai
// this is really weird if you’ve never seen it before
expect(foo).to.be.ok; 

// unexpected
expect(foo, ‘to be ok’);

[Munter]

I'm obviously all for this change. But I'm completely biased, having contributed to the unexpected ecosystem myself.

[outsideris]

This is the first time for 'unexpected' like @Bamieh.
I'm not sure that this is too abstraction or not, but unexpected is interesting.

[outsideris]

Only one test failed is the matter here not 2.
So, it should be expect(res, 'to have failed test count', 1).

[boneskull]

good catch. note that this implies “to have failed” but to have passed test count does not

[boneskull]

(imply to have passed). sorry on phone

[outsideris]

How about expect(result, '[not] to satisfy', {stats: {failures: count}});?

the result of previous is:

expected
{
  stats: {
    suites: 1, tests: 1, passes: 0, pending: 0, failures: 2, start: '2018-04-22T17:06:46.509Z', end: '2018-04-22T17:06:46.513Z',
    duration: 4
  },
  tests: [
    {
      title: 'should only display this error', fullTitle: 'suite1 should only display this error', duration: 1, currentRetry: 0,
      err: ...
    }
  ],
  pending: [],
  failures: [
    {
      title: 'should only display this error', fullTitle: 'suite1 should only display this error', duration: 1, currentRetry: 0,
      err: ...
    },
    { title: '"after all" hook', fullTitle: 'suite1 "after all" hook', duration: 0, currentRetry: 0, err: ... }
  ],
  passes: [], code: 2
}
to have failed test count 1

And the result of satisfy is:

expected
{
  stats: {
    suites: 1, tests: 1, passes: 0, pending: 0, failures: 2, start: '2018-04-22T17:07:42.938Z', end: '2018-04-22T17:07:42.941Z',
    duration: 3
  },
  tests: [
    {
      title: 'should only display this error', fullTitle: 'suite1 should only display this error', duration: 0, currentRetry: 0,
      err: ...
    }
  ],
  pending: [],
  failures: [
    {
      title: 'should only display this error', fullTitle: 'suite1 should only display this error', duration: 0, currentRetry: 0,
      err: ...
    },
    { title: '"after all" hook', fullTitle: 'suite1 "after all" hook', duration: 0, currentRetry: 0, err: ... }
  ],
  passes: [], code: 2
}
to have failed test count 1

{
  stats: {
    suites: 1,
    tests: 1,
    passes: 0,
    pending: 0,
    failures: 2, // should equal 1
    start: '2018-04-22T17:07:42.938Z',
    end: '2018-04-22T17:07:42.941Z',
    duration: 3
  },
  tests: [
    {
      title: 'should only display this error', fullTitle: 'suite1 should only display this error', duration: 0, currentRetry: 0,
      err: ...
    }
  ],
  pending: [],
  failures: [
    {
      title: 'should only display this error', fullTitle: 'suite1 should only display this error', duration: 0, currentRetry: 0,
      err: ...
    },
    { title: '"after all" hook', fullTitle: 'suite1 "after all" hook', duration: 0, currentRetry: 0, err: ... }
  ],
  passes: [],
  code: 2
}

I feel that the result is pretty verbose, but it is straightforward.

[boneskull]

yeah, hmmm... so what I didn’t do was create customized diffs for each assertion. you can do this. but didn’t see it as necessary for a first pass.

what you’re saying comes in to play when working with the diffs—I’m not sure which of the two forms will more easily provide a better diff once it’s customized.

[outsideris]

Agreed. I'm interested in the diff, but I'm also not sure which is better.

[plroebuck]

Had never heard or even seen this of this library; Google searches for expect all reference "chai".
Really dislike the use of 'to be' comparing two different variables; I immediately think of comparing their addresses, not values. That said, glancing the code changes, I notice several places where the new assertion should probably be different.

For example, TAP reporter spec had this:
expect(stdout[0], 'to equal', expectedMessage);
should probably be:
expect(stdout[0], 'to be', expectedMessage);

Similar elsewhere. Sorry, but couldn't comment inline.

[boneskull]

to be is a strict equality which is going to be akin to a reference.

[boneskull]

and yes, we should probably audit those assertions as this was mostly a search/replace job.

[boneskull]

but if it ended up being to equal it was originally .to.eql. in this case it was already incorrect

[boneskull]

@plroebuck curious what the value of the Google search is? expect is a variable. I could call it fireball instead.

[plroebuck]

to be is a strict equality which is going to be akin to a reference.

Unexpected's documentation says to be uses SameValue equality, which is different from ===.

[boneskull]

@plroebuck you're right. though the documentation page @ ECMA is suspended, apparently?

Either way, their implementation makes more sense, since things like NaN === NaN evaluate to false. thanks javascript

[boneskull]

squashed and rebased and reformatted; @outsideris' comment addressed in 03b3c4a

[boneskull]

weird, how did options.spec.js get reverted...uarggghh

UPDATE: this was actually a new test which was added that I hadn't yet converted

[Bamieh]

I've just had a full thorough look at the PR. This is awesome work. My only concern was about the community's familiarity with unexpected. But like you mentioned, the syntax is almost identical and easy to comprehend.

[plroebuck]

I know this is already merged, but...

mocha/test/node-unit/file-utils.spec.js

Lines 44 to 59 in 85c8f0d

	it('should accept a glob "path" value', function() {
	var res = utils
	.lookupFiles(tmpFile('mocha-utils*'), ['js'], false)
	.map(path.normalize.bind(path));
	var expectedLength = 0;
	var ex = expect(res, 'to contain', tmpFile('mocha-utils.js'));
	expectedLength++;
	if (symlinkSupported) {
	ex = ex.and('to contain', tmpFile('mocha-utils-link.js'));
	expectedLength++;
	}
	ex.and('to have length', expectedLength);
	});

I fail to see any value to using expectedLength here, since it's really being used to calculate the actual length. #L58 will always be correct if the other two tests pass.

Should remove all lines containing expectedLength. As a better fix, collect the expected files in an array and spread them in single call to expect(). Something like:

    it('should accept a glob "path" value', function() {
      var res = utils
        .lookupFiles(tmpFile('mocha-utils*'), ['js'], false)
        .map(path.normalize.bind(path));

      var expectedFiles = [
        tmpFile('mocha-utils.js')
      ];
      if (symlinkSupported) {
        expectedFiles.push(tmpFile('mocha-utils-link.js'));
      }
      expect(res, 'to contain', ...expectedFiles);
    });