Should we transition from precommit hooks to prepush hooks? Or can we speed up the precommit hooks? #1269
Comments
|
Two more CONS:
|
Yes, that's true if implemented as a pre-push hook. But as mentioned in #1269 (comment), this may be better implemented as a single script that runs before any pushes begin. Then the checks will be run only once and commits + pushes will be very fast. |
|
I also wanted to mention that if we move to a push/hooks script, we could have the script ping CTQ when all pushes are complete, thus triggering CTQ to run a test. Moving to triggered instead of polling could speed things up and reduce false positives. |
|
Patch with intermediate progress: Index: js/scripts/push-all.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/scripts/push-all.js b/js/scripts/push-all.js
--- a/js/scripts/push-all.js (revision 5740812a7b9e88e89ecaf7052b0cd154ed5c542b)
+++ b/js/scripts/push-all.js (date 1658330757450)
@@ -1,27 +1,28 @@
// Copyright 2021, University of Colorado Boulder
const execute = require( '../common/execute' );
+const lintRepos = require( '../../../chipper/js/grunt/lintRepos' );
const _ = require( 'lodash' ); // eslint-disable-line
const fs = require( 'fs' );
// constants
// Don't use getActiveRepos() since it cannot be run from the root
-const contents = fs.readFileSync( 'perennial/data/active-repos', 'utf8' ).trim();
+const contents = fs.readFileSync( '../perennial/data/active-repos', 'utf8' ).trim();
const repos = contents.split( '\n' ).map( sim => sim.trim() );
/**
* Push all active-repos
*
* USAGE:
- * cd ${root containing all repos}
- * node perennial/js/scripts/push-all.js
+ * cd perennial or any repo
+ * node ../perennial/js/scripts/push-all.js
*
* @author Sam Reid (PhET Interactive Simulations)
*/
( async () => {
// const a = repos.map( repo => execute( 'git', 'log --branches --not --remotes --simplify-by-decoration --decorate --oneline'.split(' '), `${repo}`, {
- const promises = repos.map( repo => execute( 'git', 'log --branches --not --remotes --simplify-by-decoration --decorate --oneline'.split( ' ' ), `${repo}`, {
+ const promises = repos.map( repo => execute( 'git', 'log --branches --not --remotes --simplify-by-decoration --decorate --oneline'.split( ' ' ), `../${repo}`, {
// resolve errors so Promise.all doesn't fail on first repo that cannot pull/rebase
errors: 'resolve'
@@ -45,24 +46,41 @@
}
}
- const pushPromises = pushRepos.map( repo => execute( 'git', [ 'push' ], `${repo}`, {
+ if ( pushRepos.length === 0 ) {
+ console.log( 'Nothing to push' );
+ }
+ else {
- // resolve errors so Promise.all doesn't fail on first repo that cannot pull/rebase
- errors: 'resolve'
- } ) );
- const pushResults = await Promise.all( pushPromises );
+ // Batch tests
+ console.log( 'linting: ' + pushRepos.join( ', ' ) );
+ const totalProblems = await lintRepos( pushRepos );
+ console.log( 'total problems: ' + totalProblems );
- // Report results
- for ( let i = 0; i < pushRepos.length; i++ ) {
- const repo = pushRepos[ i ];
- const returnObject = pushResults[ i ];
+ console.log( 'type checking everything' );
- console.log( repo );
- if ( returnObject.stdout.trim().length > 0 ) {
- console.log( returnObject.stdout );
- }
- if ( returnObject.stderr.trim().length > 0 ) {
- console.log( returnObject.stderr );
- }
+
+ // Individual tests
+
+
+ // const pushPromises = pushRepos.map( repo => execute( 'git', [ 'push' ], `${repo}`, {
+ //
+ // // resolve errors so Promise.all doesn't fail on first repo that cannot pull/rebase
+ // errors: 'resolve'
+ // } ) );
+ // const pushResults = await Promise.all( pushPromises );
+ //
+ // // Report results
+ // for ( let i = 0; i < pushRepos.length; i++ ) {
+ // const repo = pushRepos[ i ];
+ // const returnObject = pushResults[ i ];
+ //
+ // console.log( repo );
+ // if ( returnObject.stdout.trim().length > 0 ) {
+ // console.log( returnObject.stdout );
+ // }
+ // if ( returnObject.stderr.trim().length > 0 ) {
+ // console.log( returnObject.stderr );
+ // }
+ // }
}
} )();
\ No newline at end of file
</details. |
|
Discussed in the 7/21/2022 dev meeting and decided it was too large of a discussion for this TS-focused meeting because changing when we do the hook checks (pre-commit versus pre-push) could be a significant change to our process. There are concerns with there being broken commits in the code base, since these could mess up bisect operations that grab code at arbitrary points in the past. There may be a way to work around this, such as squashing commits when push-hooks block a push, but this would require some investigation. |
|
A few more thoughts about this topic, some of which I expressed at 7/21/2022 dev meeting ...
|
|
Here's a tutorial on rewriting git history (includes |
|
@zepumph and I discussed a caching strategy that will streamline the precommit hook process. We tested a prototype and it is working nicely. We will commit soon, and after the commit, we want to add a feature that will make it easy to
I'll have some merge conflicts since other work is being done in these files, but will try to commit soon. |
samreid
changed the title
|
Here is my working patch on a new cache layer that was reviewed with @zepumph. I'm stashing it here, anticipating merge conflicts when I pull other changes from chipper and perennial linting. Index: main/perennial/js/scripts/absolute-tsc.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/perennial/js/scripts/absolute-tsc.js b/main/perennial/js/scripts/absolute-tsc.js
--- a/main/perennial/js/scripts/absolute-tsc.js (revision b5604a599f8285fe825ac1d9b17a0c3126497e87)
+++ b/main/perennial/js/scripts/absolute-tsc.js (date 1658437499932)
@@ -17,6 +17,7 @@
*/
const start = Date.now();
const execute = require( '../common/execute' );
+const CacheLayer = require( './CacheLayer' );
const os = require( 'os' );
const path = require( 'path' );
const { resolve } = require( 'path' ); // eslint-disable-line
@@ -27,10 +28,24 @@
}
( async () => {
- const results = await execute( 'node', [ `${process.cwd()}${path.sep}chipper/node_modules/typescript/bin/tsc` ], args[ 0 ], {
+
+ const cacheKey = 'absolute-tsc#' + args[ 0 ];
+
+ if ( CacheLayer.isCacheSafe( cacheKey ) ) {
+ console.log( 'no changes' );
+ return;
+ }
+ else {
+ console.log( 'changes detected...' );
+ }
+
+ const results = await execute( 'node', [ '../../../chipper/node_modules/typescript/bin/tsc' ], args[ 0 ], {
errors: 'resolve'
} );
+ console.log( results.stdout );
+ console.log( results.stderr );
+
// If there was a problem running tsc, report it here. The type errors are reported on stdout below.
if ( results.stderr.length > 0 ) {
console.log( results );
@@ -41,6 +56,7 @@
if ( results.stdout.trim().length === 0 ) {
console.log( `0 errors in ${elapsed}ms` );
+ CacheLayer.onSuccess( cacheKey );
}
else {
const lines = results.stdout.trim().split( os.EOL );
Index: main/chipper/js/grunt/lint.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/chipper/js/grunt/lint.js b/main/chipper/js/grunt/lint.js
--- a/main/chipper/js/grunt/lint.js (revision 3afed4c15b9161dc707b13cb9c5ceca694dea4b1)
+++ b/main/chipper/js/grunt/lint.js (date 1658439239779)
@@ -15,6 +15,8 @@
const grunt = require( 'grunt' );
const crypto = require( 'crypto' );
const chipAway = require( './chipAway' );
+const CacheLayer = require( '../../../perennial/js/scripts/CacheLayer' ); // eslint-disable-line
+
// constants
const EXCLUDE_PATTERNS = [ // patterns that have no code that should be linted
@@ -40,7 +42,13 @@
*/
const lint = async ( patterns, options ) => {
- options = _.assignIn( {
+ // if ( patterns.length === 0 ) {
+ // console.log( 'how about a pattern though' );
+ // throw new Error( 'how about a pattern, though' );
+ // }
+ // console.log( options );
+
+ options = _.merge( {
cache: true,
format: false, // append an extra set of rules for formatting code.
fix: false, // whether fixes should be written to disk
@@ -48,6 +56,9 @@
chipAway: false // returns responsible dev info for easier chipping.
}, options );
+ // why aren't all these keys appearing????
+ // console.log( options );
+
// filter out all unlintable pattern. An unlintable repo is one that has no `js` in it, so it will fail when trying to
// lint it. Also, if the user doesn't have some repos checked out, those should be skipped
patterns = patterns.filter( pattern => !EXCLUDE_PATTERNS.includes( pattern ) &&
@@ -89,6 +100,16 @@
config.extends = configExtends;
eslintConfig.baseConfig = config;
+ const cacheLayerKey = 'lint#' + patterns.join( ', ' ) + '#' + JSON.stringify( options );
+ if ( CacheLayer.isCacheSafe( cacheLayerKey ) ) {
+ // console.log( 'cache was safe' );
+ return [];
+ }
+ else {
+ console.log( 'cache unsafe: ' + cacheLayerKey );
+ console.log( patterns );
+ }
+
const eslint = new ESLint( eslintConfig );
grunt.verbose.writeln( `linting: ${patterns.join( ', ' )}` );
@@ -120,6 +141,9 @@
options.warn && grunt.fail.warn( `${totalErrors} errors and ${totalWarnings} warnings` );
}
+ else {
+ CacheLayer.onSuccess( cacheLayerKey );
+ }
return results;
};
Index: main/perennial/js/scripts/hook-pre-commit.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/perennial/js/scripts/hook-pre-commit.js b/main/perennial/js/scripts/hook-pre-commit.js
--- a/main/perennial/js/scripts/hook-pre-commit.js (revision b5604a599f8285fe825ac1d9b17a0c3126497e87)
+++ b/main/perennial/js/scripts/hook-pre-commit.js (date 1658440351075)
@@ -30,19 +30,18 @@
// Console logging via --console
const commandLineArguments = process.argv.slice( 2 );
- const outputToConsole = commandLineArguments.includes( '--console' );
+ const outputToConsole = commandLineArguments.includes( '--console' ) || true;
// Run lint tests if they exist in the checked-out SHAs.
+ const lintStart = Date.now();
+ console.log( 'starting lint' );
+
try {
const lint = require( '../../../chipper/js/grunt/lint' );
if ( lint.chipperAPIVersion === 'promises1' ) {
// lint() automatically filters out non-lintable repos
- const results = await lint( [ `../${repo}` ], {
- cache: true,
- fix: false,
- warn: false
- } );
+ const results = await lint( [ `../${repo}` ] );
const problems = results.filter( result => result.errorCount > 0 || result.warningCount > 0 );
problems.forEach( result => console.error( `lint failed in ${repo}`, result.filePath, result.messages.map( m => JSON.stringify( m, null, 2 ) ).join( '\n' ) ) );
@@ -50,7 +49,7 @@
process.exit( 1 );
}
- outputToConsole && console.log( `Linting passed with results.length: ${results.length}` );
+ outputToConsole && console.log( `Linting passed with results.length: ${results.length}, problems.length: ${problems.length}` );
}
else {
console.log( 'chipper/js/grunt/lint not compatible' );
@@ -59,6 +58,10 @@
catch( e ) {
console.log( 'chipper/js/grunt/lint not found' );
}
+ console.log( 'lint time: ' + ( Date.now() - lintStart ) );
+
+ const reportMediaStart = Date.now();
+ console.log( 'report media:' );
// These sims don't have package.json or media that requires checking.
const optOutOfReportMedia = [
@@ -89,16 +92,22 @@
console.log( 'chipper/js/grunt/reportMedia not found' );
}
}
+ console.log( 'report media time: ' + ( Date.now() - reportMediaStart ) );
+
+ const tscStart = Date.now();
+ console.log( 'starting tsc' );
// Run typescript type checker if it exists in the checked-out shas
if ( fs.existsSync( '../chipper/tsconfig/all/tsconfig.json' ) ) {
try {
- const results = await execute( 'node', [ '../../../chipper/node_modules/typescript/bin/tsc' ], '../chipper/tsconfig/all', {
+ const results = await execute( 'node', [ '../perennial/js/scripts/absolute-tsc.js', '../chipper/tsconfig/all' ], './', {
errors: 'resolve'
} );
-
+ results.stdout.trim().length > 0 && console.log( results.stdout );
+ results.stderr.trim().length > 0 && console.log( results.stderr );
if ( results.code === 0 ) {
+
outputToConsole && console.log( 'tsc passed' );
}
else {
@@ -113,7 +122,10 @@
else {
outputToConsole && console.log( 'tsconfig.json not found, skipping tsc' );
}
+ console.log( 'tsc time: ' + ( Date.now() - tscStart ) );
+ const unitTestStart = Date.now();
+ console.log( 'starting unit tests' );
// Run qunit tests if puppeteerQUnit exists in the checked-out SHAs and a test HTML exists.
try {
@@ -122,6 +134,7 @@
const testFilePath = `${repo}/${repo}-tests.html`;
const exists = fs.existsSync( `../${testFilePath}` );
if ( exists ) {
+ console.log( 'running unit tests: ' + testFilePath );
const browser = await puppeteer.launch();
const result = await withServer( async port => {
@@ -143,4 +156,6 @@
catch( e ) {
console.log( e );
}
+
+ console.log( 'unit test time: ' + ( Date.now() - unitTestStart ) );
} )();
\ No newline at end of file
Index: main/chipper/js/common/Transpiler.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/chipper/js/common/Transpiler.js b/main/chipper/js/common/Transpiler.js
--- a/main/chipper/js/common/Transpiler.js (revision 3afed4c15b9161dc707b13cb9c5ceca694dea4b1)
+++ b/main/chipper/js/common/Transpiler.js (date 1658443269381)
@@ -13,6 +13,7 @@
const fs = require( 'fs' );
const path = require( 'path' );
const crypto = require( 'crypto' );
+const CacheLayer = require( '../../../perennial/js/scripts/CacheLayer' );// eslint-disable-line
const core = require( '@babel/core' );
const assert = require( 'assert' );
const _ = require( 'lodash' ); // eslint-disable-line require-statement-match
@@ -227,7 +228,11 @@
withForwardSlashes.includes( 'transpile/cache/status.json' ) ||
// Temporary files sometimes saved by the IDE
- withForwardSlashes.endsWith( '~' );
+ withForwardSlashes.endsWith( '~' ) ||
+
+ // eslint cache files
+ withForwardSlashes.includes( '/chipper/eslint/cache/' ) ||
+ withForwardSlashes.endsWith( '.eslintcache' );
}
// @private
@@ -263,6 +268,7 @@
// @public
transpileAll() {
this.transpileRepos( this.activeRepos );
+ CacheLayer.updateLastChangedTimestamp();
}
// @private
@@ -282,6 +288,8 @@
return;
}
+ CacheLayer.updateLastChangedTimestamp();
+
const pathExists = fs.existsSync( filePath );
if ( !pathExists ) {
Index: main/perennial/js/scripts/CacheLayer.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/perennial/js/scripts/CacheLayer.js b/main/perennial/js/scripts/CacheLayer.js
new file mode 100644
--- /dev/null (date 1658438485173)
+++ b/main/perennial/js/scripts/CacheLayer.js (date 1658438485173)
@@ -0,0 +1,50 @@
+// Copyright 2022, University of Colorado Boulder
+
+const fs = require( 'fs' );
+
+const readFileAsJSON = () => {
+ try {
+ return JSON.parse( fs.readFileSync( '../chipper/dist/cache-layer.json' ) );
+ }
+ catch( e ) {
+ return {};
+ }
+};
+
+const latestChangeTimestamp = 'latestChangeTimestamp';
+
+const writeFileAsJSON = json => {
+ fs.writeFileSync( '../chipper/dist/cache-layer.json', JSON.stringify( json, null, 2 ) );
+};
+
+module.exports = {
+ updateLastChangedTimestamp() {
+ const json = readFileAsJSON();
+ json[ latestChangeTimestamp ] = Date.now();
+ writeFileAsJSON( json );
+ },
+
+ // When a process succeeds, save the timestamp
+ onSuccess( keyName ) {
+ const json = readFileAsJSON();
+ json[ keyName ] = Date.now();
+ writeFileAsJSON( json );
+ },
+
+ // Check whether we need to re-run a process
+ isCacheStale( keyName ) {
+ return !this.isCacheSafe( keyName );
+ },
+
+ isCacheSafe( keyName ) {
+ const json = readFileAsJSON();
+ const time = json[ keyName ];
+ const lastChanged = json[ latestChangeTimestamp ];
+ if ( typeof time === 'number' && typeof lastChanged === 'number' && lastChanged < time ) {
+ return true;
+ }
+ else {
+ return false;
+ }
+ }
+};
\ No newline at end of file
|
|
In our dev meeting, we affirmed that we want to continue with precommit hooks to help ensure high code quality at each commit. Therefore, @zepumph and I are pursuing a different strategy in #1289 to speed up and streamline the precommit hooks (by eliminating duplicated work overhead). We may still want a step to notify CT after a batch of pushes is complete, but that will be worked on in phetsims/aqua#152. This issue can be closed. UPDATE: Additionally we do not guarantee that checking out SHAs at a specific time will be consistent. It could be between pushes. It seem the only way to address that part would be with a monorepo #1242 or with more metadata to indicate times where the repos are unstable (say, between pushes). |
Previously, our team had a preference that each commit should pass type checks and be correct and consistent. However, in phetsims/sun#732, we decided that it is too much work to make sure every single commit is correct and consistent and passes type checking, as long as each push is correct/consistent/passes type checking. We decided this based on (a) the effort in making sure every commit is type-correct, particularly when converting js=>ts and (b) the fact that it is rare to want to check out arbitrary SHAs and type check them.
While waiting for N sets of precommit hooks to finish, I started to wonder if we want to apply that same philosophy to the rest of our commit/push process.
Our current process is: (I) run linting, type checking, unit tests, etc. on every commit.
The proposal for discussion in this issue is (II): We would disable all precommit hooks and move that effort to pre-push hooks or an equivalent part of the push process.
PROS:
tscwithout project references and withincremental: truetakes around 3.7 seconds with no code changes. That means if I am committing 3 batches of changes across 10 repos,tscno-ops alone take 3.7 * 3 * 10 = 111 seconds. (or I am incentivized to disable precommit hooks altogether).perennial/js/scripts/push-all.jsso that it runs the type checker on everything once, runs lint on everything once, and runs unit tests on changed repos.CONS:
The text was updated successfully, but these errors were encountered: