Set up mocha, chai and karma for sheriff-o-matic unit testing.

Set up mocha, chai and karma for sheriff-o-matic unit testing.

This is the first step in switching to the mocha testing framework.

We'll use the Chai assertion library, which provides should-style,
expect-style, and assert-style assertions. We should avoid 'should',
since it modifies Object.prototype. 'expect' is more readable and
flexible than 'assert', although the (optional) chaining keywords can
make it overly verbose. For a full comparison, see:
http://chaijs.com/guide/styles/

I've used 'expect' in the two example tests, but left out the unnecessary
chaining keywords. This makes the most sense to me but feel free to
disagree.

Finally, Karma is a test runner that allows us to remotely run unit tests
in Chrome from the command line. We may need to use some trickery to have
this work for both Chrome and Chromium, but that is for another patch.

R=esprehn@chromium.org,ojan@chromium.org
NOTRY=true

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179075

[michaelpg, 2014-07-22 22:52:35]

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/ka...
File Tools/GardeningServer/karma.conf.js (right):

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/ka...
Tools/GardeningServer/karma.conf.js:55: port: 9876,
this is the default. would it be safer to pick a more "random" port?
http://xkcd.com/221/

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/pa...
File Tools/GardeningServer/package.json (right):

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/pa...
Tools/GardeningServer/package.json:3: "version": "0.1.0",
npm yells at me if I don't include a version. so... what's our version gonna be?

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/pa...
Tools/GardeningServer/package.json:4: "repository": {
same here, this is just so npm shuts up.

[ojan, 2014-07-23 01:36:58]

This l g t m. Much better than qunit. I'd like esprehn to take a look for the
final approval though.

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/RE...
File Tools/GardeningServer/README-mocha (right):

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/RE...
Tools/GardeningServer/README-mocha:16: 
Maybe add a section to the readme explaining what chai/mocha/karma/sugar are and
do? Just a one-sentence description for each is enough.

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/ka...
File Tools/GardeningServer/karma.conf.js (right):

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/ka...
Tools/GardeningServer/karma.conf.js:10: 
Why the double newlines?

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/ka...
Tools/GardeningServer/karma.conf.js:29: {pattern:
'ui/**/*[^tests].{js,html,css}', watched: true, included: false, served: true},
There's also a model directory now.

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/pa...
File Tools/GardeningServer/package.json (right):

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/pa...
Tools/GardeningServer/package.json:3: "version": "0.1.0",
0.1.0 sgtm. :)

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/te...
File Tools/GardeningServer/test/run-unit-tests.html (right):

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/te...
Tools/GardeningServer/test/run-unit-tests.html:17: <script>
Hmmm...I'm not sure the order of imports + inline script is guaranteed. esprehn
do you know?

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/test/ct-builder-tests.html (right):

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/test/ct-builder-tests.html:20:
requestAnimationFrame(function() { done(); });
Lol. I like that we can say done now instead of start to finish async tests. :)

[michaelpg, 2014-07-23 02:12:01]

Cool, thanks. Just a few responses below.

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/RE...
File Tools/GardeningServer/README-mocha (right):

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/RE...
Tools/GardeningServer/README-mocha:16: 
On 2014/07/23 01:36:57, ojan-only-code-yellow-reviews wrote:
> Maybe add a section to the readme explaining what chai/mocha/karma/sugar are
and
> do? Just a one-sentence description for each is enough.

This is more of a replacement readme for when we switch over testing. I'm ok
with shortening the descriptions if there is a better place to put this
documentation so it's accessible.

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/ka...
File Tools/GardeningServer/karma.conf.js (right):

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/ka...
Tools/GardeningServer/karma.conf.js:11: // frameworks to use
This is how it was formatted when it was generated by "karma init". But it won't
be re-generated so we can format it how we like I guess.

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/test/ct-builder-tests.html (right):

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/test/ct-builder-tests.html:20:
requestAnimationFrame(function() { done(); });
On 2014/07/23 01:36:58, ojan-only-code-yellow-reviews wrote:
> Lol. I like that we can say done now instead of start to finish async tests.
:)

If we wanted, we could do it qunit-style by removing the setup functions and
making the tests themselves async. But yeah, this is so much better.

[ojan, 2014-07-23 02:38:37]

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/RE...
File Tools/GardeningServer/README-mocha (right):

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/RE...
Tools/GardeningServer/README-mocha:16: 
On 2014/07/23 02:12:01, michaelpg wrote:
> On 2014/07/23 01:36:57, ojan-only-code-yellow-reviews wrote:
> > Maybe add a section to the readme explaining what chai/mocha/karma/sugar are
> and
> > do? Just a one-sentence description for each is enough.
> 
> This is more of a replacement readme for when we switch over testing. I'm ok
> with shortening the descriptions if there is a better place to put this
> documentation so it's accessible.

I think you misunderstood me. I was asking for *more* description, not less.

[michaelpg, 2014-07-23 03:06:42]

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/RE...
File Tools/GardeningServer/README-mocha (right):

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/RE...
Tools/GardeningServer/README-mocha:16: 
On 2014/07/23 02:38:37, ojan-only-code-yellow-reviews wrote:
> On 2014/07/23 02:12:01, michaelpg wrote:
> > On 2014/07/23 01:36:57, ojan-only-code-yellow-reviews wrote:
> > > Maybe add a section to the readme explaining what chai/mocha/karma/sugar
are
> > and
> > > do? Just a one-sentence description for each is enough.
> > 
> > This is more of a replacement readme for when we switch over testing. I'm ok
> > with shortening the descriptions if there is a better place to put this
> > documentation so it's accessible.
> 
> I think you misunderstood me. I was asking for *more* description, not less.

Oh, yeah, sorry. I thought you were saying edit the existing README instead.
Will do.

[esprehn, 2014-07-25 23:59:39]

The expect() syntax is really weird, I never use that, I use assert.equals() and
such and describe()/it()/beforeEach()/afterEach(), but you can use this if you
prefer. All the suite() terminology just seems like noise though.

I'd also give the tests better names like:

describe("cr-builder-grid", function() {
  it("should load some stuff", function() {
    // ...
  });
});

Note that you can also return a Promise from a it() and it'll wait for it to
resolve in the test (like the done() thing)

lgtm with nits.

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/te...
File Tools/GardeningServer/test/karma-unit-tests.html (right):

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/te...
Tools/GardeningServer/test/karma-unit-tests.html:8: <head>
omit head

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/te...
Tools/GardeningServer/test/karma-unit-tests.html:11: <body>
omit body

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/te...
File Tools/GardeningServer/test/run-unit-tests.html (right):

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/te...
Tools/GardeningServer/test/run-unit-tests.html:12: </head>
ditto

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/test/ct-builder-grid-tests.html (right):

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/test/ct-builder-grid-tests.html:58:
suiteSetup(function(done) {
beforeEach() I think?

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/test/ct-builder-grid-tests.html:65: test('basic',
function() {
describe() and it(), I don't know what suite() is, but with describe/it you
shouldn't need 3 levels.

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/test/ct-builder-grid-tests.html:67:
expect(rows).lengthOf(2);
I usually use assert.equal(rows.length, 2)

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/test/ct-builder-tests.html (right):

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/test/ct-builder-tests.html:26: expect(a.href);
This is weird.

[michaelpg, 2014-07-28 20:35:05]

I've switched to describe/it instead of suite/test, and use chai.assert instead
of chai.expect per elliott's suggestion.

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/RE...
File Tools/GardeningServer/README-mocha (right):

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/RE...
Tools/GardeningServer/README-mocha:16: 
On 2014/07/23 01:36:57, ojan-only-code-yellow-reviews wrote:
> Maybe add a section to the readme explaining what chai/mocha/karma/sugar are
and
> do? Just a one-sentence description for each is enough.

Done.

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/ka...
File Tools/GardeningServer/karma.conf.js (right):

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/ka...
Tools/GardeningServer/karma.conf.js:11: // frameworks to use
On 2014/07/23 02:12:01, michaelpg wrote:
> This is how it was formatted when it was generated by "karma init". But it
won't
> be re-generated so we can format it how we like I guess.

Done.

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/ka...
Tools/GardeningServer/karma.conf.js:29: {pattern:
'ui/**/*[^tests].{js,html,css}', watched: true, included: false, served: true},
On 2014/07/23 01:36:57, ojan-only-code-yellow-reviews wrote:
> There's also a model directory now.

Acknowledged.

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/te...
File Tools/GardeningServer/test/karma-unit-tests.html (right):

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/te...
Tools/GardeningServer/test/karma-unit-tests.html:8: <head>
On 2014/07/25 23:59:39, esprehn wrote:
> omit head

why? this is a standalone page.

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/te...
File Tools/GardeningServer/test/run-unit-tests.html (right):

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/te...
Tools/GardeningServer/test/run-unit-tests.html:12: </head>
On 2014/07/25 23:59:39, esprehn wrote:
> ditto

why? this is a standalone page intended to be manually opened in the browser. it
provides better interactivity than the command-line (karma) tests.

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/te...
Tools/GardeningServer/test/run-unit-tests.html:17: <script>
On 2014/07/23 01:36:58, ojan-only-code-yellow-reviews wrote:
> Hmmm...I'm not sure the order of imports + inline script is guaranteed.
esprehn
> do you know?

Good question. I've done some testing and attempted to parse the standards and
both lead me to believe that scripts in the import should be run before the
scripts that follow.

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/test/ct-builder-grid-tests.html (right):

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/test/ct-builder-grid-tests.html:58:
suiteSetup(function(done) {
On 2014/07/25 23:59:39, esprehn wrote:
> beforeEach() I think?

suiteSetup = beforeEach in tdd mode.

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/test/ct-builder-grid-tests.html:65: test('basic',
function() {
On 2014/07/25 23:59:39, esprehn wrote:
> describe() and it(), I don't know what suite() is, but with describe/it you
> shouldn't need 3 levels.

suite = describe, test = it.

3 levels seems to be the "canonical" way to do it. hopefully using better
descriptions helps.

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/test/ct-builder-grid-tests.html:67:
expect(rows).lengthOf(2);
On 2014/07/25 23:59:39, esprehn wrote:
> I usually use assert.equal(rows.length, 2)

Done.

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/test/ct-builder-tests.html (right):

https://codereview.chromium.org/411763003/diff/20001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/test/ct-builder-tests.html:26: expect(a.href);
On 2014/07/25 23:59:39, esprehn wrote:
> This is weird.

Acknowledged.

[ojan, 2014-07-28 21:46:29]

This all lgtm.

[esprehn, 2014-07-28 22:03:38]

<html>, <head> and <body> are all optional elements, even in stand alone pages.
You never need to add them, the parser will do it for you. Adding them just adds
noise which is why I was suggesting you to remove them.

[michaelpg, 2014-07-28 22:34:11]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2014-07-28 22:34:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/michaelpg@chromium.org/411763003/80001

[commit-bot: I haz the power, 2014-07-28 23:57:16]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/17417)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/19321)

[michaelpg, 2014-07-29 00:27:00]

The CQ bit was unchecked by michaelpg@chromium.org

[michaelpg, 2014-07-29 00:27:19]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2014-07-29 00:28:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/michaelpg@chromium.org/411763003/80001

[commit-bot: I haz the power, 2014-07-29 00:29:40]

Change committed as 179075