npm run test during precommit hooks

[samreid]

From #384, we would like the precommit hooks to run any node tests that the package.json provides, like npm run test.

[samreid]

How about an optional fuzz test too? (maybe separate issue)

[samreid]

This patch works well on mac:

Subject: [PATCH] Rename tsc => check, see https://github.com/phetsims/perennial/issues/404
---
Index: js/common/pre-commit-main.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/pre-commit-main.ts b/js/common/pre-commit-main.ts
--- a/js/common/pre-commit-main.ts	(revision 87c763b7fa9ed79642f67e1a4e6305dcb43ca96f)
+++ b/js/common/pre-commit-main.ts	(date 1733271353882)
@@ -7,6 +7,7 @@
  * @author Michael Kauzmann (PhET Interactive Simulations)
  */
 
+import { spawnSync } from 'child_process';
 import fs from 'fs';
 import CacheLayer from '../../../chipper/js/common/CacheLayer.js';
 import reportMedia from '../../../chipper/js/grunt/reportMedia.js';
@@ -20,6 +21,7 @@
 import getPhetLibs from '../grunt/getPhetLibs.js';
 import generatePhetioMacroAPI from '../phet-io/generatePhetioMacroAPI.js';
 import phetioCompareAPISets from '../phet-io/phetioCompareAPISets.js';
+import npmCommand from '../../../perennial-alias/js/common/npmCommand.js';
 
 type Repo = string;
 
@@ -189,4 +191,16 @@
 
     process.exit( phetioAPIOK ? 0 : 1 );
   }
+  else if ( command === 'npm-run-test' ) {
+
+    const result = spawnSync( npmCommand, [ 'run', 'test' ], {
+      cwd: `../${repo}`,
+      stdio: 'inherit'
+    } );
+
+    process.exit( result.status );
+  }
+  else {
+    throw new Error( `Unknown command: ${command}` );
+  }
 } )();
\ No newline at end of file
Index: js/grunt/tasks/pre-commit.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/grunt/tasks/pre-commit.ts b/js/grunt/tasks/pre-commit.ts
--- a/js/grunt/tasks/pre-commit.ts	(revision 87c763b7fa9ed79642f67e1a4e6305dcb43ca96f)
+++ b/js/grunt/tasks/pre-commit.ts	(date 1733270915618)
@@ -33,12 +33,13 @@
  */
 
 import assert from 'assert';
+import fs from 'fs';
 import path from 'path';
+import { Repo } from '../../../../perennial-alias/js/browser-and-node/PerennialTypes.js';
 import buildLocal from '../../../../perennial-alias/js/common/buildLocal.js';
 import dirname from '../../../../perennial-alias/js/common/dirname.js';
 import execute from '../../../../perennial-alias/js/common/execute.js';
 import getActiveRepos from '../../../../perennial-alias/js/common/getActiveRepos.js';
-import { Repo } from '../../../../perennial-alias/js/browser-and-node/PerennialTypes.js';
 import phetTimingLog from '../../../../perennial-alias/js/common/phetTimingLog.js';
 import tsxCommand from '../../../../perennial-alias/js/common/tsxCommand.js';
 import getOption, { isOptionKeyProvided } from '../../../../perennial-alias/js/grunt/tasks/util/getOption.js';
@@ -82,6 +83,17 @@
 
   const possibleTasks = [ 'lint', 'report-media', 'type-check', 'unit-test', 'phet-io-api' ];
 
+  try {
+    const packageString = fs.readFileSync( `../${repo}/package.json`, 'utf8' );
+    const packageJSON = JSON.parse( packageString );
+    if ( packageJSON.scripts?.hasOwnProperty( 'test' ) ) {
+      possibleTasks.push( 'npm-run-test' );
+    }
+  }
+  catch( e ) {
+    // no package.json or not parseable
+  }
+
   // By default, run all tasks
   let tasksToRun = [ ...possibleTasks ];
   const OPT_OUT_ALL = '*'; // Key to opt out of all tasks

please be aware of #421 during your review. I confirmed this correctly ignores repos that do not have npm run test, and that a failed test correctly gives a bad exit code. I didn't think we want to silence any of the console.log, so right now it is just outputting everything.

@zepumph can you please review and see what adjustments are necessary to run it on Windows? Also it appears this would be our first usage of spawnSync outside of tests.

[zepumph]

This was done differently from unit-test. In that case, we wait to the subprocess to test to see if there is a tests.html file. If not, we just return true. I don't care too much how it is done, but I feel like they should be similar.

Expanding on that thought, what if this was just a sub-section of unit-test? That task can look for the repo-tests.html and the npm script, and do between 0 and 2 of them. I can't think of a reason from a pre-commit context that I would want to treat these differently.

I also recommend using execute and console logging only with 'outputToConsole'.

[samreid]

Implemented as prescribed and ready for review in main.

[zepumph]

Oh sorry. I'm changing a few things I see in pre-commit code.

[zepumph]

Ok sorry, I kinda tore some things apart. I felt like we needed separate files for a lot of things, so I did a larger code review of the pre-commit task as part of this. I also added chipper browser unit tests to ensure we were well set up to test both.

@samreid how do you feel about this? I also see a big problem here because of phetsims/chipper#1549. It was hard to tests chipper's browser qunit tests because the cache kept hitting. I ended up stubbing out CacheLayer.isCacheSafe() to always return false, there is probably a better way, but oh well. Let me know if you want to discuss.

[samreid]

I skimmed through the commits and the direction seems good. I'm glad chipper has browser unit tests now. I did not run any sanity checks yet, wasn't sure how thorough a review was warranted. Want to specify or close?

[zepumph]

All good here. Thanks!