Run qunit tests as a pre-commit hook

[samreid]

In phetsims/chipper#921 we streamlined the usage of linting as a pre-commit hook, and it has been working extremely well. I recommend we also add unit tests as a precommit hook.

This was inspired by conversations in https://github.com/phetsims/phet-io/issues/1648 about how we can automatically test phet-io specifications.

[samreid]

I committed a first draft for a script that can be used to automatically run unit tests as a precommit hook. Testing the timing to see if it is fast enough to be used for every commit:

axon: 0m1.919s
joist: 0m0.910s
scenery-phet: 0m1.257s
scenery: 0m11.024s
natural-selection (no unit tests): 0m0.133s

[samreid]

If we move forward with this, here is the commit hook file that we could use. If linting fails, it doesn't run unit tests:

node ../chipper/js/scripts/lint-main.js
if [ $? -ne 0 ]
then
  exit 1
fi

node ../aqua/js/local/qunit-main.js
if [ $? -ne 0 ]
then
  exit 1
fi

[samreid]

@pixelzoom can you please take a look and recommend whether we should adopt this qunit test phase for phet-info/git-template-dir/hooks/pre-commit ?

[pixelzoom]

How long does qunit-main.js take to complete? If it's significantly longer than lint-main.js, then perhaps it should be done in pre-push instead of pre-commit.

[samreid]

I reported timing in #90 (comment)

[pixelzoom]

I wasn't clear on whether the timing you reported in #90 (comment) was for qunit-main.js or the entire pre-commit hook (lint-main.js + qunit-main.js). Assume that the timing is for the pre-commit hook.... I guess I don't mind waiting up to 1 second on each commit. But you might want to run this past other developers. If there's push back, then moving qunit-main.js to pre-push would likely accomplish the same goal.

[pixelzoom]

Doing this in a git hook raises another issue... Is everyone using git hooks? (I suspect not.) Should they be? (I think so.)

[samreid]

The timing I reported is for the qunit tests only. @pixelzoom do you want to bring this to developer meeting?

[pixelzoom]

I missed scenery: 0m11.024s when I first looked at the timing. Imo, that's too long to be done in the pre-commit. My vote would be to do it in pre-push.

[samreid]

Have you experimented with pre-push hooks using the Webstorm push interface? Does it respect the hooks and behave accordingly?

[pixelzoom]

I have not experimented with WebStorm + pre-push hook. I didn't find anything relevant in a Google search for "IntelliJ pre-push hook".

[zepumph]

Having a hardcoded slash after the localTestingURL was causing bugs for me on Windows http-server, and this was not how we used localTestingURL when generating PhET-iO API files. I removed that hard coded slash, and added support for dynamically adding it if it isn't provided from the client's build-local.

I also found it very hard to debug this, so I added some error logging before process.exit( 1 ) calls. I'm not sure if the lint one is needed, but the unit test one was working well in my case!

@samreid please review my changes.

[samreid]

Today @zepumph and I found a circular JSON error when running the unit tests but only in puppeteer and only on @zepumph's machine. It was fine in @zepumph browser and on @samreid machine. @zepumph will work around this mysterious issue by using === instead of assert.equal.

[samreid]

@samreid please review my changes.

Looks good, thanks! Also, I realized we can use the same strategy we proposed for API file generation, to have node serve the files to puppeteer instead of relying on each developer's localhost.

[jessegreenberg]

Discussed on 5/21/20 -

Went over the PSA and all say its OK to remove lint-main.js.

[samreid]

lint-main.js has been deleted.

[chrisklus]

This is super cool! I tried a commit that should break a unit test and the commit failed. Thanks!

[zepumph]

unit tests in phet-io-wrappers are designed to take a sim as a query parameter. This doesn't seem suitable for committing to phet-io-wrappers, so I also turned off that unit test above.

[samreid]

Having a hardcoded slash after the localTestingURL was causing bugs for me on Windows http-server, and this was not how we used localTestingURL when generating PhET-iO API files. I removed that hard coded slash, and added support for dynamically adding it if it isn't provided from the client's build-local.

Another way to address this would be to have node serve the files to puppeteer instead of relying on the developer localhost. We implemented this for https://github.com/phetsims/phet-io/issues/1664 and the same should be possible here. I'll open a side issue for that and close this one.