phettest should run output-js

[samreid]

phettest needs a grunt tsc -b all step when pulling

[samreid]

We may be able to use phetsims/phetmarks#56 for local testing before we push to the remote.

[samreid]

I messaged on slack:

The TypeScript step for CT and phettest are working well in local testing. Next step is to test them on the remote. I don’t expect this to be a seamless process, there will be hiccups. But please note that none of these changes introduce any different code into the dependencies for sims that are in-progress, such as density, fourier or CCK

[samreid]

I'm adding npm install for each repo on phettest, so that we can run grunt commands in the idiomatic way.

[samreid]

I pushed support for TS code on phettest, and ran the following experiments:

• Converted blast to TS
• Added TS code to blast
• Added TS code that fails the type checker. Press "Pull". It says "failed" but did compile the code and code was runnable
• Fixed type errors and pressed "Pull". It says succeeded and runs the new code
• Reverted blast to JS

For reference, here are the basic steps to convert a project to TypeScript:

1. add "typescript":true to package.json's phet section.
2. run grunt update

Running "pull-all" will run grunt output-js-all but that has the known problem that if there are any type errors, no code will be emitted. Running pull on individual directories does not have this constraint.

So I would consider basic support for this to be working, but I anticipate numerous problems related to:

• How do I know if there is a type error?
• How do I know if a fail was during pull or during compilation?
• How should we report type errors?
• If you press "pull all" and the pull all succeeds, but there are errors in the output-js-all, it will say "pull failed"
  etc.

I'm at a good point to check in with @jonathanolson or @zepumph.

[samreid]

Patch from investigation with @jonathanolson

Index: phettest/phettest-server.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/phettest/phettest-server.js b/phettest/phettest-server.js
--- a/phettest/phettest-server.js	(revision f6ad184d9171c882d0265a84fa7f902c0d394e0b)
+++ b/phettest/phettest-server.js	(date 1631132798022)
@@ -8,6 +8,8 @@
 const path = require( 'path' );
 const url = require( 'url' );
 const fs = require( 'fs' );
+const gitStatus = require( '../../perennial/js/common/gitStatus' );
+const gitPull = require( '../../perennial/js/common/gitPull' );
 
 const port = 45362;
 
@@ -80,6 +82,10 @@
   execute( ON_WIN ? 'grunt.cmd' : 'grunt', [ 'output-js', '--no-color' ], rootDir + repo, callback, errCallback );
 }
 
+async function gruntOutputJSAsync( repo, callback, errCallback ) {
+  execute( ON_WIN ? 'grunt.cmd' : 'grunt', [ 'output-js', '--no-color' ], rootDir + repo, callback, errCallback );
+}
+
 function gruntOutputJSAll( callback, errCallback ) {
   execute( ON_WIN ? 'grunt.cmd' : 'grunt', [ 'output-js-all', '--no-color' ], rootDir + 'chipper', callback, errCallback );
 }
@@ -217,9 +223,21 @@
   pull( simName, successFunction( req, res, `pull ${simName}` ), errorFunction( req, res, `pull ${simName}` ) );
 }
 
-function taskPullAll( req, res, query ) {
+async function taskPullAll( req, res, query ) {
 
-  execute( 'bash', [ `${rootDir}perennial/bin/pull-all.sh`, '-p' ], rootDir,
+  const activeRepos = getActiveRepos();
+  const reposToPull = [];
+  for ( const repo of activeRepos ) {
+    const status = await gitStatus( repo );
+    if ( status.behind > 0 ) {
+      reposToPull.push( repo );
+    }
+  }
+  for ( const repo of reposToPull ) {
+    await gitPull( repo );
+    await outputJS();
+  }
+  await execute( 'bash', [ `${rootDir}perennial/bin/pull-all.sh`, '-p' ], rootDir,
 
     // After pull-all succeeds, run output-js-all
     () => gruntOutputJSAll( successFunction( req, res, 'pulled' ), errorFunction( req, res, 'output-js-all failed' ) ),

[zepumph]

How do I know if there is a type error?
How do I know if a fail was during pull or during compilation?

Can we just bank on the fact that master may be unstable, but will ALWAYS be compilable? Can we do a git commit hook for that?

[samreid]

There is a git hook that will prevent developers from committing code that doesn't pass the type checker.

[samreid]

@jonathanolson and I made progress on this today, leveraging more of perennial tools. @zepumph pointed out that since phettest-server runs from 2 levels deep, its paths are incompatible with a lot of perennial tooling. @zepumph recommended I create a branch with phettest-server moved up one directory, then in the future he can move that to a new top-level repo (instead of nested under phetmarks).

[samreid]

OK the behavior seems to be testing properly on my side, and I committed to a branch as @zepumph and I discussed. @zepumph can you please take the next steps, reaching out to me or @jonathanolson as appropriate?

[zepumph]

caf9eb7 can almost certainly be improved on, but I was blocked because many tasks in phettest are failing in certain cases due to output-js, so I tried to just get past it. Will you please make sure it is as good as it should be? Perhaps we should check on package.json instead of just the presence of tsconfig.json. This would fail when pulling the single repo aqua, because of no typescript support.

[zepumph]

@samreid and I looked at #1073 (comment) together, and we realized that this commit should be reverted, and that changes to outputJS() weren't committed yesterday, so he committed them.

[zepumph]

Much of this work was put on master as part of https://github.com/phetsims/special-ops/issues/209 too (though most of the commits are linked here). output-js has been confirmed to be working on phettest on bayes for pull, pull all, and perennial refresh.

@samreid, is there anything else for this issue?

[samreid]

Nice work on https://github.com/phetsims/special-ops/issues/209, I think everything is working nicely here and in good shape. Next steps are in #1095. Closing.