What brands should we run unit tests for in the precommit hook?

[samreid]

Related to phetsims/aqua#90 and from https://github.com/phetsims/phet-io/issues/1668#issuecomment-632987582,

@pixelzoom asked:

axon unit tests are also broken for brand=phet-io. I guess those unit tests are not being run by the pre-commit hook - should they be?

If the phet-io brand tests were a superset of all the tests, we could just run the phet-io tests. However, it is possible that phet brand tests are not a pure subset. For example, if the phet brand accidentally tries to use an object only defined in phet-io brand, this should fail, but it would pass silently.

If time were no barrier, we would run the unit tests in all brands, and probably add a "does the sim start up" test or a quick fuzz test, but we would like these precommit hooks to run quickly. Brainstorming possible ways to proceed for this issue:

1. Run performance tests to see how much slower the precommit hook is if we run unit tests on both brands.
2. See if there is a way to improve performance of the precommit hooks. Perhaps both brands can run unit tests in parallel?
3. See if there is a way to make the phet brand tests a subset of phet-io brand tests, so all tests can run in phet-io brand.
4. Determine whether we want to run a sim startup test or brief fuzz test as part of precommit.

@pixelzoom any other thoughts or remarks?

[pixelzoom]

Pre-commit hook lives in perennial and is not related to CT. So perhaps this issue should be transferred to perennial?

[pixelzoom]

I'm not familiar with brand=phet-io unit tests, so I have no idea what the performance implications are, or how they overlap with brand=phet unit tests.

Imo, the goals should be:

• Efficient use of CT
• Ensure that pushed code passes lint and unit tests
• Commit and push operations should be quick

If running unit tests significantly impacts commit times, then we should discuss whether developers want to live with that impact, or try to develop good habits and run unit tests manually.

[samreid]

Typically the brand=phet-io unit tests just add a few tests and it doesn't take much longer. For instance, in axon:

brand=phet:
46 tests completed in 877 milliseconds,

brand=phet-io:
51 tests completed in 899 milliseconds

Perhaps we should just run all tests with ?brand=phet-io.

[samreid]

@zepumph what do you recommend?

[zepumph]

Sure seems like we should run in phet-io brand too! Do you think there is a time that would be too long?

Options:

1. We preemptively say that doubling the unit test time across the board for phet-io sims doesn't work for us, and we just add in a few here and there that we think are valuable.
2. Turn it on and wait for complaints on time. There is no reason to over-optimize before it is necessary.

Either is fine with me.

[samreid]

How about if we just run ?brand=phet-io tests and not the ?brand=phet tests, since most of them are a subset?

[zepumph]

For everything? We could try it and see how it goes.

[samreid]

When I investigated this, I saw a failure in balloons and static electricity tests locally that isn't happening on CT:

~/apache-document-root/main/balloons-and-static-electricity$ node ../perennial/js/scripts/hook-pre-commit.js 
unit tests failed in balloons-and-static-electricity {
  ok: false,
  message: 'caught exception Error: Evaluation failed: TypeError: window.qunitLaunchAfterHooks is not a function\n' +
    '    at __puppeteer_evaluation_script__:12:16'
}

[samreid]

Balloons and static electricity is not calling qunitStart synchronously on startup, it wants to load the sim in an iframe first. But qunitStart doesn't support this. I wonder if we should switch to a pattern like https://api.qunitjs.com/config/QUnit.config

// QUnit is not yet loaded here
window.QUnit = {
  config: {
    autostart: false,
    noglobals: true,
  }
};

[samreid]

I think we should switch to a lock pattern like the one that simLauncher uses. In the case of balloons and static electricity running in phet-io brand in puppeteer, there will be 3 locks:

• the dynamic phetioEngine module import
• the iframe load
• the qunit hooks

[samreid]

I'd love to bounce this idea off of @zepumph before proceeding. @zepumph can you please review the last 3 comments and advise?

[jessegreenberg]

In phetsims/balloons-and-static-electricity@ff4e263, balloons-and-static-electricity-tests.js was changed to call qunitStart synchronously, but waits to start tests until after the async work is done.

[zepumph]

I think that qunitStart should not be used in BASE. Instead, since it needs custom logic, it should implement its own logic that loads everything in the order it needs.

Then running only in phet-io brand feels like a good step forward for the precommit hook. I can leave myself assigned and try to get to it soon.

[zepumph]

I think that qunitStart should not be used in BASE. Instead, since it needs custom logic, it should implement its own logic that loads everything in the order it needs.

Reviewing balloons-and-static-electricity-tests.js, it looks like qunitStart seems correct in this case.

Then running only in phet-io brand feels like a good step forward for the precommit hook. I can leave myself assigned and try to get to it soon.

After #191, every unit-test-enabled repo now runs successfully with ?brand=phet-io. I think we are ready to turn this on.

[zepumph]

After the above change, I tried committing this change which I expected to fail out in phet-io brand, and it didn't let me commit:

Index: js/PropertyTests.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/PropertyTests.js	(revision a09c536c544271627845cdec60a23360df746593)
+++ js/PropertyTests.js	(date 1597941570664)
@@ -263,7 +263,6 @@
     const getOrderDependencyLength = () => propertyStateHandlerSingleton.getNumberOfOrderDependencies()  - originalOrderDependencyLength;
 
     const firstProperty = new Property( 1, {
-      tandem: parentTandem.createTandem( 'firstProperty' ),
       phetioType: PropertyIO( NumberIO )
     } );
     const secondProperty = new Property( 1, {

@samreid please review this issue in correlation with #191

[samreid]

This seems like a good solution. I tested by hacking a test in PropertyTests and saw that node ../perennial/js/scripts/hook-pre-commit.js failed. I'll comment on the other aspects in #191, closing.