Improve precommit hook development experience #1350
Comments
I used that feature this week to search for the break in #1323 (comment). Only one of the dozen samples was broken, which was great and made it easy to trace the problem.
Pros: OK to commit unstable/broken code quickly without using commit hooks. People checking out master won't get that unstable code. |
|
Some strategies, brainstorming and paper trail from discussions with @marlitas and @zepumph yesterday: OK to commit broken code locally, then use a pre-push hook to make sure everything is consistent before pushing. How would that help people that basically push every commit? Would we change our workflow? What about checking out broken code by date or bisect? Commit broken code locally, then squash (bad?) commits before push? But then you lose granularity and have to make up new commit messages. Persist CT values and use that as a guide when git bisecting How can we speed up tests??? phet-io API tests only took 25% on Michael's machine. It is not the bottleneck, especially if it runs in parallel with other things. Would restoring project references help at all? Other TypeScript type checking performance improvement ideas: Should we run lints in different processes? Can we just check in with WebStorm and ask "hey webstorm, are there any type errors or lint errors?" There is a Before Commit "analyze code" inspection? For sims, only type check sims. You can profile the type checker to see where the bottlenecks are. Should we transition from precommit hooks to prepush hooks? Or can we speed up the precommit hooks? Type check downstream Type checking takes too long to be a precommit hook: Precommit hooks should check in with watch process We did project references in #1055 Table with timing is |
Index: js/scripts/hook-pre-commit-implementation.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/scripts/hook-pre-commit-implementation.js b/js/scripts/hook-pre-commit-implementation.js
new file mode 100644
--- /dev/null (date 1668034306470)
+++ b/js/scripts/hook-pre-commit-implementation.js (date 1668034306470)
@@ -0,0 +1,203 @@
+// Copyright 2020-2022, University of Colorado Boulder
+
+/**
+ * Runs tasks for pre-commit, including lint and qunit testing. Avoids the overhead of grunt and Gruntfile.js for speed.
+ *
+ * Should only be run when developing in master, because when dependency shas are checked out for one sim,
+ * they will likely be inconsistent for other repos which would cause failures for processes like type checking.
+ * This means when running maintenance release steps, you may need to run git commands with --no-verify.
+ *
+ * Timing data is streamed through phetTimingLog, please see that file for how to see the results live and/or afterwards.
+ *
+ * USAGE:
+ * cd ${repo}
+ * node ../chipper/js/scripts/hook-pre-commit.js
+ *
+ * OPTIONS:
+ * --console: outputs information to the console for debugging
+ *
+ * See also phet-info/git-template-dir/hooks/pre-commit for how this is used in precommit hooks.
+ *
+ * @author Sam Reid (PhET Interactive Simulations)
+ */
+
+const fs = require( 'fs' );
+const puppeteer = require( 'puppeteer' );
+const withServer = require( '../../../perennial-alias/js/common/withServer' );
+const execute = require( '../../../perennial-alias/js/common/execute' );
+const getPhetLibs = require( '../grunt/getPhetLibs' );
+const getRepoList = require( '../../../perennial-alias/js/common/getRepoList' );
+const generatePhetioMacroAPI = require( '../phet-io/generatePhetioMacroAPI' );
+const CacheLayer = require( '../../../chipper/js/common/CacheLayer' );
+const phetioCompareAPISets = require( '../phet-io/phetioCompareAPISets' );
+const lint = require( '../../../chipper/js/grunt/lint' );
+const reportMedia = require( '../../../chipper/js/grunt/reportMedia' );
+const puppeteerQUnit = require( '../../../aqua/js/local/puppeteerQUnit' );
+
+( async () => {
+
+ const commandLineArguments = process.argv.slice( 2 );
+ const outputToConsole = commandLineArguments.includes( '--console' );
+
+ const getArg = arg => {
+ const args = commandLineArguments.filter( commandLineArg => commandLineArg.startsWith( `--${arg}=` ) );
+ if ( args.length !== 1 ) {
+ throw new Error( `expected only one arg: ${args}` );
+ }
+ return args[ 0 ].split( '=' )[ 1 ];
+ };
+
+ const command = getArg( 'command' );
+ const repo = getArg( 'repo' );
+
+ if ( command === 'lint' ) {
+ // Run lint tests if they exist in the checked-out SHAs.
+
+ // lint() automatically filters out non-lintable repos
+ const lintReturnValue = await lint( [ repo ] );
+ outputToConsole && console.log( `Linting passed with results.length: ${lintReturnValue.results.length}` );
+ process.exit( lintReturnValue.ok ? 0 : 1 );
+ }
+
+ if ( command === 'report-media' ) {
+
+ // These sims don't have package.json or media that requires checking.
+ const optOutOfReportMedia = [
+ 'decaf',
+ 'phet-android-app',
+ 'babel',
+ 'phet-info',
+ 'phet-ios-app',
+ 'qa',
+ 'sherpa',
+ 'smithers',
+ 'tasks',
+ 'weddell'
+ ];
+
+ // Make sure license.json for images/audio is up-to-date
+ if ( !optOutOfReportMedia.includes( repo ) ) {
+
+ const success = await reportMedia( repo );
+ process.exit( success ? 0 : 1 );
+ }
+ else {
+
+ // no need to check
+ return process.exit( 0 );
+ }
+ }
+
+ if ( command === 'tsc' ) {
+
+
+ // Run typescript type checker if it exists in the checked-out shas
+ const results = await execute( 'node', [ '../chipper/js/scripts/absolute-tsc.js', '../chipper/tsconfig/all' ], '../chipper', {
+ errors: 'resolve'
+ } );
+
+ results.stderr.trim().length > 0 && console.log( results.stderr );
+ results.stdout.trim().length > 0 && console.log( results.stdout );
+
+ if ( results.code === 0 ) {
+ outputToConsole && console.log( 'tsc passed' );
+ process.exit( 0 );
+ }
+ else {
+ console.log( results );
+ process.exit( 1 );
+ }
+ }
+
+ if ( command === 'qunit' ) {
+
+ // Run qunit tests if puppeteerQUnit exists in the checked-out SHAs and a test HTML exists.
+ const qUnitOK = await ( async () => {
+
+ const cacheKey = `puppeteerQUnit#${repo}`;
+
+ if ( repo !== 'scenery' && repo !== 'phet-io-wrappers' ) { // scenery unit tests take too long, so skip those
+ const testFilePath = `${repo}/${repo}-tests.html`;
+ const exists = fs.existsSync( `../${testFilePath}` );
+ if ( exists ) {
+
+ if ( CacheLayer.isCacheSafe( cacheKey ) ) {
+ return true;
+ }
+ else {
+ const browser = await puppeteer.launch();
+
+ const result = await withServer( async port => {
+ return puppeteerQUnit( browser, `http://localhost:${port}/${testFilePath}?ea&brand=phet-io` );
+ } );
+
+ await browser.close();
+
+ outputToConsole && console.log( `${repo}: ${JSON.stringify( result, null, 2 )}` );
+ if ( !result.ok ) {
+ console.error( `unit tests failed in ${repo}`, result );
+ return false;
+ }
+ else {
+ CacheLayer.onSuccess( cacheKey );
+ return true;
+ }
+ }
+ }
+
+ outputToConsole && console.log( 'QUnit: no problems detected' );
+ return true;
+ }
+ return true;
+ } )();
+
+ process.exit( qUnitOK ? 0 : 1 );
+ }
+
+ if ( command === 'phet-io-api-compare' ) {
+
+ ////////////////////////////////////////////////////////////////////////////////
+ // Compare PhET-iO APIs for this repo and anything that has it as a dependency
+ //
+ const phetioAPIOK = await ( async () => {
+
+ const getCacheKey = repo => `phet-io-api-compare#${repo}`;
+
+ // Test this repo and all phet-io sims that have it as a dependency. For instance, changing sun would test
+ // every phet-io stable sim.
+ const phetioAPIStable = getRepoList( 'phet-io-api-stable' );
+ const reposToTest = phetioAPIStable
+ .filter( phetioSimRepo => getPhetLibs( phetioSimRepo ).includes( repo ) )
+
+ // Only worry about the ones that are not cached
+ .filter( repo => !CacheLayer.isCacheSafe( getCacheKey( repo ) ) );
+
+ if ( reposToTest.length > 0 ) {
+ console.log( 'PhET-iO API testing: ' + reposToTest );
+
+ const proposedAPIs = await generatePhetioMacroAPI( reposToTest, {
+ showProgressBar: reposToTest.length > 1,
+ showMessagesFromSim: false
+ } );
+
+ const phetioAPIComparisonSuccessful = await phetioCompareAPISets( reposToTest, proposedAPIs, {} );
+
+ if ( phetioAPIComparisonSuccessful ) {
+ reposToTest.forEach( repo => CacheLayer.onSuccess( getCacheKey( repo ) ) );
+ }
+
+ return phetioAPIComparisonSuccessful;
+ }
+ else {
+ console.log( 'PhET-iO API testing: no repos to test' );
+ return true;
+ }
+ } )();
+
+ process.exit( phetioAPIOK ? 0 : 1 );
+ }
+
+ // OTHER TESTS GO HERE
+
+ // NOTE: if adding or rearranging rules, be careful about the early exit above
+} )();
\ No newline at end of file
Index: js/scripts/hook-pre-commit.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/scripts/hook-pre-commit.js b/js/scripts/hook-pre-commit.js
--- a/js/scripts/hook-pre-commit.js (revision 6ce9bf076772c2c7094bfed1a1efc3a2e5fa0325)
+++ b/js/scripts/hook-pre-commit.js (date 1668034388940)
@@ -21,20 +21,10 @@
* @author Sam Reid (PhET Interactive Simulations)
*/
-const fs = require( 'fs' );
const path = require( 'path' );
-const puppeteer = require( 'puppeteer' );
-const withServer = require( '../../../perennial-alias/js/common/withServer' );
const execute = require( '../../../perennial-alias/js/common/execute' );
-const getPhetLibs = require( '../grunt/getPhetLibs' );
-const getRepoList = require( '../../../perennial-alias/js/common/getRepoList' );
-const generatePhetioMacroAPI = require( '../phet-io/generatePhetioMacroAPI' );
-const CacheLayer = require( '../../../chipper/js/common/CacheLayer' );
-const phetioCompareAPISets = require( '../phet-io/phetioCompareAPISets' );
const phetTimingLog = require( '../../../perennial-alias/js/common/phetTimingLog' );
-const lint = require( '../../../chipper/js/grunt/lint' );
-const reportMedia = require( '../../../chipper/js/grunt/reportMedia' );
-const puppeteerQUnit = require( '../../../aqua/js/local/puppeteerQUnit' );
+const _ = require( 'lodash' ); // eslint-disable-line require-statement-match
( async () => {
@@ -47,168 +37,40 @@
const commandLineArguments = process.argv.slice( 2 );
const outputToConsole = commandLineArguments.includes( '--console' );
- // Run lint tests if they exist in the checked-out SHAs.
- const lintOK = await phetTimingLog.startAsync( 'lint', async () => {
-
- // lint() automatically filters out non-lintable repos
- const lintReturnValue = await lint( [ repo ] );
- outputToConsole && console.log( `Linting passed with results.length: ${lintReturnValue.results.length}` );
- return lintReturnValue.ok;
- } );
-
- if ( !lintOK ) {
- return false;
- }
-
- const reportMediaOK = await phetTimingLog.startAsync( 'report-media', async () => {
-
- // These sims don't have package.json or media that requires checking.
- const optOutOfReportMedia = [
- 'decaf',
- 'phet-android-app',
- 'babel',
- 'phet-info',
- 'phet-ios-app',
- 'qa',
- 'sherpa',
- 'smithers',
- 'tasks',
- 'weddell'
- ];
-
- // Make sure license.json for images/audio is up-to-date
- if ( !optOutOfReportMedia.includes( repo ) ) {
-
- const success = await reportMedia( repo );
- return success;
- }
- else {
-
- // no need to check
- return true;
- }
- } );
-
- if ( !reportMediaOK ) {
- return false;
- }
-
- const tscOK = await phetTimingLog.startAsync( 'tsc', async () => {
-
- // Run typescript type checker if it exists in the checked-out shas
- const results = await execute( 'node', [ '../chipper/js/scripts/absolute-tsc.js', '../chipper/tsconfig/all' ], '../chipper', {
- errors: 'resolve'
- } );
-
- results.stderr.trim().length > 0 && console.log( results.stderr );
- results.stdout.trim().length > 0 && console.log( results.stdout );
-
- if ( results.code === 0 ) {
- outputToConsole && console.log( 'tsc passed' );
- return true;
- }
- else {
- console.log( results );
- return false;
- }
- } );
-
- if ( !tscOK ) {
- return false;
- }
-
- const qunitOK = await phetTimingLog.startAsync( 'qunit', async () => {
-// Run qunit tests if puppeteerQUnit exists in the checked-out SHAs and a test HTML exists.
-
- const cacheKey = `puppeteerQUnit#${repo}`;
-
- if ( repo !== 'scenery' && repo !== 'phet-io-wrappers' ) { // scenery unit tests take too long, so skip those
- const testFilePath = `${repo}/${repo}-tests.html`;
- const exists = fs.existsSync( `../${testFilePath}` );
- if ( exists ) {
-
- if ( CacheLayer.isCacheSafe( cacheKey ) ) {
- console.log( 'unit tests success cached' );
- return true;
- }
- else {
- const browser = await puppeteer.launch();
-
- const result = await withServer( async port => {
- return puppeteerQUnit( browser, `http://localhost:${port}/${testFilePath}?ea&brand=phet-io` );
- } );
+ // Ordered!
+ const manyPromises = [ 'lint', 'report-media', 'tsc', 'qunit', 'phet-io-api-compare' ].map( task => {
+ return phetTimingLog.startAsync( task, async () => {
+ const results = await execute( 'node', [ '../chipper/js/scripts/hook-pre-commit-implementation.js', `--command=${task}`, `--repo=${repo}`,
+ outputToConsole ? '--console' : '' ], '../chipper', {
+ errors: 'resolve'
+ } );
+ results.stdout && console.log( results.stdout );
+ results.stderr && console.log( results.stderr );
+ return results.code;
+ } );
+ } );
- await browser.close();
- outputToConsole && console.log( `${repo}: ${JSON.stringify( result, null, 2 )}` );
- if ( !result.ok ) {
- console.error( `unit tests failed in ${repo}`, result );
- return false;
- }
- else {
- CacheLayer.onSuccess( cacheKey );
- return true;
- }
- }
- }
+ let resolved = false;
- outputToConsole && console.log( 'QUnit: no problems detected' );
- return true;
- }
- return true;
- } );
-
- if ( !qunitOK ) {
- return false;
- }
-
- ////////////////////////////////////////////////////////////////////////////////
- // Compare PhET-iO APIs for this repo and anything that has it as a dependency
- //
- const phetioAPIOK = await phetTimingLog.startAsync( 'phet-io-api-compare', async () => {
-
- const getCacheKey = repo => `phet-io-api-compare#${repo}`;
-
- // Test this repo and all phet-io sims that have it as a dependency. For instance, changing sun would test
- // every phet-io stable sim.
- const phetioAPIStable = getRepoList( 'phet-io-api-stable' );
- const reposToTest = phetioAPIStable
- .filter( phetioSimRepo => getPhetLibs( phetioSimRepo ).includes( repo ) )
-
- // Only worry about the ones that are not cached
- .filter( repo => !CacheLayer.isCacheSafe( getCacheKey( repo ) ) );
-
- if ( reposToTest.length > 0 ) {
- console.log( 'PhET-iO API testing: ' + reposToTest );
-
- const proposedAPIs = await generatePhetioMacroAPI( reposToTest, {
- showProgressBar: reposToTest.length > 1,
- showMessagesFromSim: false
+ // TODO!!!! Reject and use promise.all?
+ return new Promise( resolve => {
+ for ( let i = 0; i < manyPromises.length; i++ ) {
+ const manyPromise = manyPromises[ i ];
+ manyPromise.then( exitCode => {
+ if ( exitCode !== 0 ) {
+ if ( !resolved ) {
+ resolved = true;
+ resolve();
+ }
+ }
} );
-
- const phetioAPIComparisonSuccessful = await phetioCompareAPISets( reposToTest, proposedAPIs, {} );
-
- if ( phetioAPIComparisonSuccessful ) {
- reposToTest.forEach( repo => CacheLayer.onSuccess( getCacheKey( repo ) ) );
- }
-
- return phetioAPIComparisonSuccessful;
- }
- else {
- console.log( 'PhET-iO API testing: no repos to test' );
- return true;
}
} );
- if ( !phetioAPIOK ) {
- return false;
- }
-
- // OTHER TESTS GO HERE
-
- // NOTE: if adding or rearranging rules, be careful about the early exit above
// If everything passed, return true for success
- return true;
+ // const exitCodes = await Promise.all( manyPromises );
+ // return _.every( exitCodes, code => code === 0 );
} );
// generatePhetioMacroAPI is preventing exit for unknown reasons, so manually exit here |
|
I switched to Promise.all, and tested with <!-- 11/9/2022, 4:49:08 PM -->
<hook-pre-commit repo="chipper">
<lint>
<report-media>
<tsc>
<qunit>
<phet-io-api-compare>
</qunit> <!-- 310ms -->
</report-media> <!-- 335ms -->
</tsc> <!-- 416ms -->
</hook-pre-commit repo="chipper"> <!-- 1206ms --> |
|
OK I also fixed the depth. Looks like this now: <!-- 11/9/2022, 5:06:46 PM -->
<hook-pre-commit repo="chipper">
<lint>
<report-media>
<tsc>
<qunit>
<phet-io-api-compare>
</lint> <!-- 378ms -->
</phet-io-api-compare> <!-- 375ms -->
</report-media> <!-- 394ms -->
</qunit> <!-- 399ms -->
</tsc> <!-- 468ms -->
</hook-pre-commit repo="chipper"> <!-- 473ms -->This part of the issue is ready for review. Maybe for next steps, we could gather a few days of data for how long things are taking now, and focus on the bottleneck task? |
|
Slack#developer: Chris Malley
Michael Kauzmann |
|
Not sure what else to do about this issue at the moment, @zepumph or @AgustinVallejo do you recommend further changes? Want to meet again? Check in with dev team? |
|
No comments from me! I think it's ready for dev team |
|
I think it would be awesome to meet again together in the next week or two, and perhaps work on another part of the speed up potential. Running things in parallel is working well for me so far, but I haven't made many commits in the last week, so I would like more data still. |
|
Can we please get the duplicate error messages fixed, reported 12 days ago in #1350 (comment)? It's not only annoying, but it's actually making me miss errors. EDIT: Move to #1359 |
|
I added a calendar meeting for 12/9 for 1 hour. Let's touch base then. |
|
From above: How can we speed up tests??? general improvement
phet-io-api compare test:
tsc
|
|
We worked a bit more on project references #1356 and are still figuring out how to handle mixins. |
…ong), #1350 Signed-off-by: Michael Kauzmann <michael.kauzmann@colorado.edu>
|
The above commits by @zepumph have a markdown formatting problem, reported in phetsims/aqua#210. |
|
Today we had another dev meeting discussion on phet-io api checking in pre commit hooks. Here is a copy of the discussion:
Basically, it seems like we may want to allow git hooks to be more flexible, and rely on running more checks manually before commit, or face the wrath of CTQ. |
|
Should we include an option to git hooks if people do want api checking? That may help let everyone be happy from the vote above. @samreid saw that there was a Webstorm hook for "after commit", but likely we instead would want it to be before the commit happens locally. |
|
@samreid just had a great idea to add build-local.json customization so that each dev can have exactly the pre-commits that they want. I love it! |
|
You can now opt out of tasks in build-local.json. By default, all tasks run. You can opt out of individual tasks like so: "gitHooks": {
"lint": "off",
"report-media": "off",
"tsc": "off",
"qunit": "off",
"phet-io-api-compare": "off"
}The In the comment #1350 (comment), @zepumph @jessegreenberg @pixelzoom @jonathanolson indicated preference to avoid "gitHooks": {
"phet-io-api-compare": "off"
}@zepumph can you please spot check review/test? |
|
Well, the proposed solution isn't very satisfying because it doesn't offer an easy way to manually trigger precommit hooks, like with precommit-hook-multi. We should also add an entry point like |
|
Opting out at the build-local.json levels seems like a "one size fits all" approach, because it will apply to all repos. Did you consider a way to opt out per repo? |
|
That sounds like a good topic to discuss over zoom (so we can more easily cover proposed use cases, complexity tradeoff, implementation details and other related aspects). Want to meet or raise it at dev meeting? |
|
Dev meeting sounds like a good forum. |
|
I believe that this solution is of the appropriate granularity. Adding further complexity and power to an opt-out that in general PhET doesn't believe in or want is not where we should go. This solution (buildLocal flags) was based on a decision by a quorum of devs at dev meeting that we are ok turning off phet-io-api-comparison in git hooks was acceptable. Later, @samreid and I noted that in that vote, devs that had been here less time unanimously voted to keep these in, and so we decided that having a setting to support that felt valuable to meet the needs of everyone. I do not think that a repo-specific solution is warranted given the design constraints of the problem. |
|
Review:
Thanks for doing this! |
|
From discussion with @samreid:
This will be a good starting point to have a conversation next week at dev meeting. |
From #1342 (comment) which came from #1325, we would like to improve the precommit hook process.
@zepumph: How should we use this tool? I often have commit hooks that take longer than 100 seconds. What is an acceptable amount of time?
@zepumph - sometimes to get around commit hook timing I commit locally without hooks and then run tests before pushing.
@marlitas - I thought we shouldn't be making breaking changes even locally?
@jonathanolson - Yes, it is better for checking out SHAs at a certain date to not have breaks committed to master.
@jonathanolson - Have the commit hooks been successfully saving us from pushing breaking changes?
@pixelzoom @jbphet @jessegreenberg - Yes!
@marlitas - I think it would be nice to be able to make rapid commits locally or to a branch without waiting for commit hooks.
@jbphet - You could make commits to a feature branch. Then when merge to master when the changes are complete.
@samreid - Lets form a sub team for this. We have log data to look through now, and need a sub team to make decisions about this.
@zepumph - I waited for built tools for 18.5 minutes yesterday. The time could be longer. Sometimes run without commit hooks.
@jessegreenberg - I am going to try feature branches without commit hooks when working in a single repo for a bit!
@kathy-phet - After the POSE convening we can have a sub-group discuss this.
The text was updated successfully, but these errors were encountered: