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. |
|
From discussion with @samreid:
This will be a good starting point to have a conversation next week at dev meeting. |
|
Above steps committed and tested Improvements from discussion with SR:
@samreid will you please review? I'm still excited for discussion about this Monday at dev meeting, but wanted to get things in a more stable spot. |
|
I reviewed the changes, made one incredible minor doc update, tested with Want to rename the keys to align with terminology eslewhere? Everything else seems good. Back to @zepumph to close or leave open for discussion at meeting, or any next steps. |
|
Great idea. Unassinging until dev meeting discussion/psa. |
|
Discussed during dev meeting. PSA in place about how to opt out of api comparison, like so: Ready to close. Future improvements to the pre-commit hooks should be created in separate issues. |
|
The existing tooling made it impossible for me to do the main thing I wanted to do: run precommit hooks on demand without the phet-io-API comparison. I will add a |
|
I'm not happy about the added complexity, but the above option behaves as desired. @zepumph can you please review, in particular double checking that pre-existing behavior has not been disrupted? Please close if all is well. |
|
I'm not really sure I understand the use case. Let's discuss this. Especially since there is already a |
|
@samreid and I discussed this and will pick things up in phetsims/perennial#404. Closing |
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: