phetsims / chipper

Tools for developing and building PhET interactive simulations.
http://scenerystack.org/
MIT License
11 stars 14 forks source link

precommit-hook-multi truncates errors #1306

Open samreid opened 2 years ago

samreid commented 2 years ago

While working on https://github.com/phetsims/chipper/issues/1305, @marlitas and I observed 2x in a row that precommit-hook-multi truncates errors like so:

  5521:1  error  Line contains bad text: 'asserting values are instanceof or typeof in typescript is redundant'  no-simple-type-checking-assertions
  5561:1  error  Line contains bad text: 'asserting values are instanceof or typeof in typescript is redundant'  no-simple-type-checking-assertions
  5875:1  error  Line contains bad text: 'asserti
scenery-phet: Success
simula-rasa: Success
samreid commented 2 years ago

This was easy to replicate by enabling @typescript-eslint/no-unnecessary-condition and triggering a change in one scenery file, then running the script.

samreid commented 2 years ago

@marlitas and I wrote this variation, but still saw truncation in webstorm:

```diff Index: js/scripts/precommit-hook-multi.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/scripts/precommit-hook-multi.js b/js/scripts/precommit-hook-multi.js --- a/js/scripts/precommit-hook-multi.js (revision 491318f81bae915aa9d12f200010a14af7557bb2) +++ b/js/scripts/precommit-hook-multi.js (date 1660583025360) @@ -1,7 +1,6 @@ // Copyright 2022, University of Colorado Boulder const startTime = Date.now(); -const execute = require( '../../../perennial-alias/js/common/execute' ); const _ = require( 'lodash' ); // eslint-disable-line const fs = require( 'fs' ); const child_process = require( 'child_process' ); @@ -72,25 +71,28 @@ // This is done sequentially so we don't spawn a bunch of uncached tsc at once, but in the future we may want to optimize // to run one sequentially then the rest in parallel - for ( let i = 0; i < reposToTest.length; i++ ) { - process.stdout.write( reposToTest[ i ] + ': ' ); + setTimeout( () => {}, 10000000 ); + const visit = () => { - const result = await execute( 'node', [ '../chipper/js/scripts/hook-pre-commit.js' ], `${reposToTest[ i ]}`, { + const next = reposToTest.shift(); + process.stdout.write( next + ': ' ); - // resolve errors so Promise.all doesn't fail on first repo that cannot pull/rebase - errors: 'resolve' + + const childProcess = child_process.spawn( 'node', [ '../chipper/js/scripts/hook-pre-commit.js' ], { + cwd: `${next}`, + stdio: 'inherit' } ); - if ( result.code === 0 ) { - console.log( 'Success' ); - } - else { - console.log(); - result.stdout.trim().length > 0 && console.log( result.stdout.trim() ); - result.stderr.trim().length > 0 && console.log( result.stderr.trim() ); - } - } - - console.log( 'Done in ' + ( Date.now() - startTime ) + 'ms' ); + childProcess.on( 'close', () => { + if ( reposToTest.length > 0 ) { + visit(); + } + else { + console.log( 'Done in ' + ( Date.now() - startTime ) + 'ms' ); + } + } ); + }; + + visit(); } )(); \ No newline at end of file ```
samreid commented 2 years ago

With the patch above, there is truncation in WebStorm but no truncation from the terminal. I changed the WebStorm console buffer size following these instructions, but it had no impact on when the data was truncated: https://stackoverflow.com/questions/11763996/output-window-of-intellij-idea-cuts-output

samreid commented 2 years ago
// numbers.js
const array = [];

for ( let i = 0; i < 1000000; i++ ) {
  array.push( i+'\n' );
}

console.log( array.join( ', ' ) );

A code comment claims: // No need to have the same ESLint just to format but we aren't convinced of that. We also think there may be some problem with promises? Or maybe WebStorm is somehow running a different version of lint? We are really grasping at straws.

We would like to reach out to @zepumph for next steps.

I'm self-unassigning to take it off of my "to do" list, but not intending this to mean @zepumph should necessarily work on it alone. Please reach out as appropriate.

zepumph commented 1 year ago

I am not sure how to reproduce this. I haven't encountered this when running the precommit multi in my webstorm over the past 6 months. Weird!.

samreid commented 1 year ago

I followed the same steps above and saw the same problem:

image

This is likely a webstorm problem that would be expensive to fix or work around. It is pretty clean when things are truncated, so it's not too worrisome. But we could alway report it to JetBrains if it leads to trouble. Perhaps leave the issue open and unassigned for now?