CT needs a grunt tsc -b all step when pulling

[samreid]

CT needs a grunt tsc -b all step when pulling

[samreid]

@jonathanolson and I made progress on this today, and I've checked in some intermediate results. However, our overall plan to use output-js-all won't work since building with --build enables noEmitOnError, see https://www.typescriptlang.org/docs/handbook/project-references.html#caveats. This means if there is one type error in one repo, it won't emit any code for any repo. Some options:

1. When there is a typescript error, show the error and skip the "unbuilt" tests in that CT column
2. Switch to a repo-based compilation and avoid the --build step for this. I'm not sure if this would duplicate work or cause other problems.

[samreid]

I followed (2) above and it is working with my locally-running CT. It will need tsconfig files for all dependencies for it to work. Until then, it will say "no tsconfig found" error on repos without tsconfig and won't be able to run a sim that is missing dependencies. Otherwise, the next steps are review and testing on bayes CT.

[samreid]

I was able to commit many of the changes to master. Here is the patch that enables the call in CT. I'll apply that when more tsconfig files are populated, from #1070

Index: js/server/Snapshot.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/server/Snapshot.js b/js/server/Snapshot.js
--- a/js/server/Snapshot.js	(revision fbb71f411a06ea2babe540baebe68768f159e3fc)
+++ b/js/server/Snapshot.js	(date 1630714662033)
@@ -10,6 +10,7 @@
 const copyDirectory = require( '../../../perennial/js/common/copyDirectory' );
 const createDirectory = require( '../../../perennial/js/common/createDirectory' );
 const deleteDirectory = require( '../../../perennial/js/common/deleteDirectory' );
+const outputJS = require( '../../../perennial/js/common/outputJS' );
 const execute = require( '../../../perennial/js/dual/execute' );
 const getRepoList = require( '../../../perennial/js/common/getRepoList' );
 const gitLastCommitTimestamp = require( '../../../perennial/js/common/gitLastCommitTimestamp' );
@@ -79,6 +80,13 @@
       this.shas[ repo ] = await gitRevParse( repo, 'master' );
     }
 
+    for ( const repo of this.repos ) {
+      this.setStatus( `Compiling TS: ${repo}` );
+      const outputJSResults = await outputJS( `${this.rootDir}/${repo}` );
+      winston.info( `${repo} outputJS complete: ${JSON.stringify( outputJSResults )}` );
+      this.setStatus( `${repo} outputJS complete: ${JSON.stringify( outputJSResults )}` );
+    }
+
     if ( !useRootDir ) {
       for ( const repo of this.repos ) {
         this.setStatus( `Copying snapshot files: ${repo}` );

@jonathanolson everything above here is ready for review.

[samreid]

Basic support is working on phettest. That helped us identify a case sensitivity problem, which has been addressed in #1089. tsconfig files have been populated in #1070. The patch above is working ok locally, so I think we can test it on the server next.

[samreid]

I pulled aqua and restarted CT with the change above.

[samreid]

It appears to be working as prescribed. The UI shows:

The logs show:

2|continuous-server  | info: status: [11:45:32am] Compiling TS: hookes-law
2|continuous-server  | info: running outputJS
2|continuous-server  | info: running grunt output-js
2|continuous-server  | info: hookes-law outputJS complete: {"code":0,"stdout":"Running \"output-js\" task\n>> TypeScript compilation complete: 2340ms\n\nDone.\n","stderr":"","cwd":"/data/share/phet/continuous-testing//hookes-law","time":3608}

etc.

I will leave this self-assigned to try out some TS things on CT, like:

• Adding TS code in blast, and seeing if it runs
• Adding a type error in blast, seeing if it runs and is reported.

But I also think this issue is ready for review from @jonathanolson.

[samreid]

I noticed a lot of time is being spent in Compiling TS, even when there are no changes. The time is significant even though we have incremental builds enabled. Could we move to a pattern where it runs output-js on any repo that needs a pull (and no others)?

Here is a proposed patch for that strategy:

Index: js/server/Snapshot.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/server/Snapshot.js b/js/server/Snapshot.js
--- a/js/server/Snapshot.js	(revision 5101edb34f7ea8aa296634b92b5b453328528c96)
+++ b/js/server/Snapshot.js	(date 1630781986132)
@@ -10,7 +10,6 @@
 const copyDirectory = require( '../../../perennial/js/common/copyDirectory' );
 const createDirectory = require( '../../../perennial/js/common/createDirectory' );
 const deleteDirectory = require( '../../../perennial/js/common/deleteDirectory' );
-const outputJS = require( '../../../perennial/js/common/outputJS' );
 const execute = require( '../../../perennial/js/dual/execute' );
 const getRepoList = require( '../../../perennial/js/common/getRepoList' );
 const gitLastCommitTimestamp = require( '../../../perennial/js/common/gitLastCommitTimestamp' );
@@ -80,13 +79,6 @@
       this.shas[ repo ] = await gitRevParse( repo, 'master' );
     }
 
-    for ( const repo of this.repos ) {
-      this.setStatus( `Compiling TS: ${repo}` );
-      const outputJSResults = await outputJS( `${this.rootDir}/${repo}` );
-      winston.info( `${repo} outputJS complete: ${JSON.stringify( outputJSResults )}` );
-      this.setStatus( `${repo} outputJS complete: ${JSON.stringify( outputJSResults )}` );
-    }
-
     if ( !useRootDir ) {
       for ( const repo of this.repos ) {
         this.setStatus( `Copying snapshot files: ${repo}` );
Index: js/server/ContinuousServer.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/server/ContinuousServer.js b/js/server/ContinuousServer.js
--- a/js/server/ContinuousServer.js	(revision 5101edb34f7ea8aa296634b92b5b453328528c96)
+++ b/js/server/ContinuousServer.js	(date 1630780752164)
@@ -16,6 +16,7 @@
 const gruntCommand = require( '../../../perennial/js/common/gruntCommand' );
 const isStale = require( '../../../perennial/js/common/isStale' );
 const npmUpdate = require( '../../../perennial/js/common/npmUpdate' );
+const outputJS = require( '../../../perennial/js/common/outputJS' );
 const sleep = require( '../../../perennial/js/common/sleep' );
 const Snapshot = require( './Snapshot' );
 const assert = require( 'assert' );
@@ -507,11 +508,16 @@
           }
           const clonedRepos = await cloneMissingRepos();
 
-          // npm prune/update on any changed repos, so we can keep our npm status good in our checked out version
+          // Run the following updates on any changed repos, so we can keep our npm status good in our checked out version
+          // 1. npm prune/update
+          // 2. output JS
           for ( const repo of [ ...staleRepos, ...clonedRepos ] ) {
             if ( fs.existsSync( `../${repo}/package.json` ) ) {
               await npmUpdate( repo );
             }
+            if ( fs.existsSync( `../${repo}/tsconfig.json` ) ) {
+              await outputJS( `../${repo}` );
+            }
           }
         }
         else {

[samreid]

CT seemed to handle a correct blast TypeScript correctly. Next I'll see if it handles a type error.

[samreid]

Excellent, lint is passing, type check is failing and unbuilt mode is passing. The code was:

const x: number = 'hello';
console.log( x );

Summarizing this issue: it seems to be working as desired, but takes longer than expected. There is a proposal for a speed improvement in #1072 (comment) but it should be reviewed by @jonathanolson since I don't know how it relates to the primary checkout and the snapshots.

[jonathanolson]

In phetsims/perennial@7107af1, can we move the ExecuteError definition below the main execute definition? It's nice to see the args/docs for execute near the top of the file.

[jonathanolson]

I don't see outputJS's return value being particularly helpful (it's not currently used). If it is kept, return Promise.all( [] ); can just be return []; (if you really want to return an array, it doesn't match with the type docs), we're an async function so our result gets wrapped with a promise anyway.

So could we remove the return value?

Additionally, I've added type docs in a few places. Otherwise looks good once the above things are resolved.

[samreid]

Thanks for the recommendations! I noticed a lot of time is taken in TypeScript compilation. There is a proposal in #1072 (comment) to improve that. What do you think?

[jonathanolson]

Could we move to a pattern where it runs output-js on any repo that needs a pull (and no others)?

Would that catch cases where I make a scenery change that breaks types in a sim? Is that just for the "unbuilt" runs? My understanding was that output-js also built dependency code, no?

[samreid]

@jonathanolson and I worked on this today. In https://www.typescriptlang.org/docs/handbook/project-references.html#caveats we found that project references cannot emit when there are errors, so output-js just outputs for a single repo, even if there are errors. We confirmed this is working well in mobius. We added the patch above so that CT will tsc on each pull rather than each snapshot. It seems to be behaving properly on bayes. Next, we addressed the code review recommendations above. Closing.

[samreid]

Path changed, we will need to make a commit to fix the path, and restart bayes CT with the new code.

[samreid]

@zepumph reports that he pulled aqua and restarted CT with the new code. Thanks!