Precommit checks should run all checks even if some early ones fail
Closed this issue · 7 comments
Currently the precommit hook process short-circuits at the first type of failure. This may be confusing if a developer addresses the reported problems from the precommit hooks and believes all tests to be passing. Instead of short-circuiting and failing early, the precommit checks should run all checks each time.
Here is a patch that implements the proposal, it seems to be working properly in my tests. I'll reach out for a reviewer.
Subject: [PATCH] Precommit check runs all tests even if early ones fail, see https://github.com/phetsims/chipper/issues/1442
---
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 739e543ddef46bcda67e058ed0e0e2a422142fbd)
+++ b/js/scripts/hook-pre-commit.js (date 1718115497617)
@@ -26,59 +26,72 @@
const phetTimingLog = require( '../../../perennial-alias/js/common/phetTimingLog' );
( async () => {
-
// Identify the current repo
const repo = process.cwd().split( path.sep ).pop();
const precommitSuccess = await phetTimingLog.startAsync( `hook-pre-commit repo="${repo}"`, async () => {
-
// Console logging via --console
const commandLineArguments = process.argv.slice( 2 );
const outputToConsole = commandLineArguments.includes( '--console' );
outputToConsole && console.log( 'repo:', repo );
- const promises = [ '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-task.js',
- `--command=${task}`,
- `--repo=${repo}`,
- outputToConsole ? '--console' : '' ], '../chipper', {
- errors: 'resolve'
- } );
- results.stdout && results.stdout.trim().length > 0 && console.log( results.stdout );
- results.stderr && results.stderr.trim().length > 0 && console.log( results.stderr );
+ const taskResults = await Promise.allSettled(
+ [ '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-task.js',
+ `--command=${task}`,
+ `--repo=${repo}`,
+ outputToConsole ? '--console' : ''
+ ],
+ '../chipper',
+ {
+ errors: 'resolve'
+ }
+ );
+ results.stdout && results.stdout.trim().length > 0 && console.log( results.stdout );
+ results.stderr && results.stderr.trim().length > 0 && console.log( results.stderr );
- if ( results.code === 0 ) {
- return 0;
- }
- else {
- let message = 'Task failed: ' + task;
- if ( results.stdout && results.stdout.trim().length > 0 ) {
- message = message + ', ' + results.stdout;
- }
- if ( results.stderr && results.stderr.trim().length > 0 ) {
- message = message + ', ' + results.stderr;
- }
- throw new Error( message );
- }
- }, {
- depth: 1
- } );
- } );
-
- try {
- await Promise.all( promises );
- console.log( 'All tasks succeeded' );
- return true;
- }
- catch( e ) {
+ if ( results.code === 0 ) {
+ return { task: task, success: true };
+ }
+ else {
+ let message = 'Task failed: ' + task;
+ if ( results.stdout && results.stdout.trim().length > 0 ) {
+ message = message + ': ' + results.stdout;
+ }
+ if ( results.stderr && results.stderr.trim().length > 0 ) {
+ message = message + ': ' + results.stderr;
+ }
+ return { task: task, success: false, message: message };
+ }
+ },
+ {
+ depth: 1
+ }
+ );
+ } )
+ );
- // Exit as soon as any one promise fails
- // Each task is responsible for outputting its error to the console, so the console should already
- // be showing the error by now
- return false;
- }
+ taskResults.forEach( result => {
+ if ( result.status === 'fulfilled' ) {
+ if ( result.value.success ) {
+ console.log( `Task ${result.value.task} succeeded` );
+ }
+ else {
+ console.error( result.value.message );
+ }
+ }
+ else {
+ console.error( `Task ${result.reason.task} encountered an error: ${result.reason.message}` );
+ }
+ } );
+
+ return taskResults.every( result => result.status === 'fulfilled' && result.value.success );
} );
// generatePhetioMacroAPI is preventing exit for unknown reasons, so manually exit here
This change makes sense, thanks!
I tested by addint a tsc error, lint error, and qunit error to a single repo and expected to see them all print to the console.
I did see the lint and tsc errors. I did not see the qunit error - it reported a success for this task, even though it fails in the browser. I reverted back to main and saw this problem before your patch. EDIT: See #1443
The old code had
// Each task is responsible for outputting its error to the console, so the console should already
// be showing the error by now
Are we going to get duplicate mesages now that we always print task results?
Otherwise, the change looks good.
Regarding the unit tests, I missed this line in hook-pre-commit-task.js
// scenery unit tests take too long, so skip thoseSo no problem there.
Are we going to get duplicate messages now that we always print task results?
I wondered that too but only saw single messages from the reporting. Thanks for the review, let's close for now.
I don't think the patch was committed, I will apply it.
It has been pushed, closing.
Thanks @jessegreenberg !