Should pre-commit hooks compare PhET-iO APIs? #1325

jessegreenberg · 2022-09-09T15:28:00Z

I often forget about checking for breaking changes and commit changes that break CT or interrupt other devs. I think it would be helpful for pre-commit hooks to check apis. This would add some time to commit hooks, my machine takes ~1.5 minutes to `grunt compare-phet-io-apis for every sim in perennial/data/phet-io-api-stable. That almost doubles the time. But we need to be in the habit of doing this anyway. Maybe this can be an option for hook-pre-commit.js or it can only be done for common code repos.

I tried to add it but hit errors using execute , I am out of time for now. Here is a patch for next time (I commented everything else out so I could just test compare-phet-io-api).

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 6646427b0e1f04ae49f39a011ee9f3b383d6937a)
+++ b/js/scripts/hook-pre-commit.js	(date 1662736936051)
@@ -22,6 +22,7 @@
 const puppeteer = require( 'puppeteer' );
 const withServer = require( '../../../perennial-alias/js/common/withServer' );
 const execute = require( '../../../perennial-alias/js/common/execute' );
+const getRepoList = require( '../../../perennial-alias/js/common/getRepoList' );
 
 ( async () => {
 
@@ -32,112 +33,135 @@
   const commandLineArguments = process.argv.slice( 2 );
   const outputToConsole = commandLineArguments.includes( '--console' );
 
-// Run lint tests if they exist in the checked-out SHAs.
-  try {
-    const lint = require( '../../../chipper/js/grunt/lint' );
-    if ( lint.chipperAPIVersion === 'promisesPerRepo1' ) {
-
-      // lint() automatically filters out non-lintable repos
-      const lintReturnValue = await lint( [ repo ] );
+  // looking for differences in phet-io-apis
+  const compareApis = commandLineArguments.includes( '--compare-phet-io-apis' );
 
-      if ( !lintReturnValue.ok ) {
-        process.exit( 1 );
-      }
+// // Run lint tests if they exist in the checked-out SHAs.
+//   try {
+//     const lint = require( '../../../chipper/js/grunt/lint' );
+//     if ( lint.chipperAPIVersion === 'promisesPerRepo1' ) {
+//
+//       // lint() automatically filters out non-lintable repos
+//       const lintReturnValue = await lint( [ repo ] );
+//
+//       if ( !lintReturnValue.ok ) {
+//         process.exit( 1 );
+//       }
+//
+//       outputToConsole && console.log( `Linting passed with results.length: ${lintReturnValue.results.length}` );
+//     }
+//     else {
+//       console.log( 'chipper/js/grunt/lint not compatible' );
+//     }
+//   }
+//   catch( e ) {
+//     console.log( 'chipper/js/grunt/lint not found' );
+//   }
+//
+// // These sims don't have package.json or media that requires checking.
+//   const optOutOfReportMedia = [
+//     'decaf',
+//     'phet-android-app',
+//     'babel',
+//     'phet-info',
+//     'phet-ios-app',
+//     'sherpa',
+//     'smithers',
+//     'tasks',
+//     'weddell'
+//   ];
+//
+// // Make sure license.json for images/audio is up-to-date
+//   if ( !optOutOfReportMedia.includes( repo ) ) {
+//     try {
+//       const reportMedia = require( '../../../chipper/js/grunt/reportMedia' );
+//
+//       const success = await reportMedia( repo );
+//
+//       // At the moment reportMedia uses grunt.fail, but we defensively use the return value here in case that changes.
+//       if ( !success ) {
+//         process.exit( 1 );
+//       }
+//     }
+//     catch( e ) {
+//       console.log( 'chipper/js/grunt/reportMedia not found' );
+//     }
+//   }
+//
+//   // Run typescript type checker if it exists in the checked-out shas
+//   const results = await execute( 'node', [ '../chipper/js/scripts/absolute-tsc.js', '../chipper/tsconfig/all' ], '../chipper', {
+//     errors: 'resolve'
+//   } );
+//
+//   results.stderr.trim().length > 0 && console.log( results.stderr );
+//   results.stdout.trim().length > 0 && console.log( results.stdout );
+//
+//   if ( results.code === 0 ) {
+//     outputToConsole && console.log( 'tsc passed' );
+//   }
+//   else {
+//     console.log( results );
+//     process.exit( results.code );
+//   }
+//
+// // Run qunit tests if puppeteerQUnit exists in the checked-out SHAs and a test HTML exists.
+//   try {
+//     const puppeteerQUnit = require( '../../../aqua/js/local/puppeteerQUnit' );
+//     const CacheLayer = require( '../../../chipper/js/common/CacheLayer' );
+//     const cacheKey = `puppeteerQUnit#${repo}`;
+//
+//     if ( repo !== 'scenery' && repo !== 'phet-io-wrappers' ) { // scenery unit tests take too long, so skip those
+//       const testFilePath = `${repo}/${repo}-tests.html`;
+//       const exists = fs.existsSync( `../${testFilePath}` );
+//       if ( exists ) {
+//
+//         if ( CacheLayer.isCacheSafe( cacheKey ) ) {
+//           console.log( 'unit tests success cached' );
+//         }
+//         else {
+//           const browser = await puppeteer.launch();
+//
+//           const result = await withServer( async port => {
+//             return puppeteerQUnit( browser, `http://localhost:${port}/${testFilePath}?ea&brand=phet-io` );
+//           } );
+//
+//           await browser.close();
+//
+//           outputToConsole && console.log( `${repo}: ${JSON.stringify( result, null, 2 )}` );
+//           if ( !result.ok ) {
+//             console.error( `unit tests failed in ${repo}`, result );
+//             process.exit( 1 ); // fail as soon as there is one problem
+//           }
+//           else {
+//             CacheLayer.onSuccess( cacheKey );
+//           }
+//         }
+//       }
+//
+//       outputToConsole && console.log( 'QUnit: no problems detected' );
+//     }
+//   }
+//   catch( e ) {
+//     console.log( e );
+//   }
 
-      outputToConsole && console.log( `Linting passed with results.length: ${lintReturnValue.results.length}` );
-    }
-    else {
-      console.log( 'chipper/js/grunt/lint not compatible' );
-    }
-  }
-  catch( e ) {
-    console.log( 'chipper/js/grunt/lint not found' );
-  }
+  // compare phet-io-apis
 
-// These sims don't have package.json or media that requires checking.
-  const optOutOfReportMedia = [
-    'decaf',
-    'phet-android-app',
-    'babel',
-    'phet-info',
-    'phet-ios-app',
-    'sherpa',
-    'smithers',
-    'tasks',
-    'weddell'
-  ];
-
-// Make sure license.json for images/audio is up-to-date
-  if ( !optOutOfReportMedia.includes( repo ) ) {
+  // get list of stable sims (getRepoList)
+  // compare phet-io-apis (execute)
+  const stableSims = getRepoList( 'phet-io-api-stable' );
+  console.log( stableSims[ 0 ] );
+  for ( let i = 0; i < stableSims.length; i++ ) {
+    const current = await execute( 'pwd', [], '.' );
+    console.log( current );
     try {
-      const reportMedia = require( '../../../chipper/js/grunt/reportMedia' );
-
-      const success = await reportMedia( repo );
-
-      // At the moment reportMedia uses grunt.fail, but we defensively use the return value here in case that changes.
-      if ( !success ) {
-        process.exit( 1 );
-      }
-    }
-    catch( e ) {
-      console.log( 'chipper/js/grunt/reportMedia not found' );
-    }
-  }
-
-  // Run typescript type checker if it exists in the checked-out shas
-  const results = await execute( 'node', [ '../chipper/js/scripts/absolute-tsc.js', '../chipper/tsconfig/all' ], '../chipper', {
-    errors: 'resolve'
-  } );
-
-  results.stderr.trim().length > 0 && console.log( results.stderr );
-  results.stdout.trim().length > 0 && console.log( results.stdout );
-
-  if ( results.code === 0 ) {
-    outputToConsole && console.log( 'tsc passed' );
-  }
-  else {
-    console.log( results );
-    process.exit( results.code );
-  }
-
-// Run qunit tests if puppeteerQUnit exists in the checked-out SHAs and a test HTML exists.
-  try {
-    const puppeteerQUnit = require( '../../../aqua/js/local/puppeteerQUnit' );
-    const CacheLayer = require( '../../../chipper/js/common/CacheLayer' );
-    const cacheKey = `puppeteerQUnit#${repo}`;
-
-    if ( repo !== 'scenery' && repo !== 'phet-io-wrappers' ) { // scenery unit tests take too long, so skip those
-      const testFilePath = `${repo}/${repo}-tests.html`;
-      const exists = fs.existsSync( `../${testFilePath}` );
-      if ( exists ) {
-
-        if ( CacheLayer.isCacheSafe( cacheKey ) ) {
-          console.log( 'unit tests success cached' );
-        }
-        else {
-          const browser = await puppeteer.launch();
 
-          const result = await withServer( async port => {
-            return puppeteerQUnit( browser, `http://localhost:${port}/${testFilePath}?ea&brand=phet-io` );
-          } );
-
-          await browser.close();
-
-          outputToConsole && console.log( `${repo}: ${JSON.stringify( result, null, 2 )}` );
-          if ( !result.ok ) {
-            console.error( `unit tests failed in ${repo}`, result );
-            process.exit( 1 ); // fail as soon as there is one problem
-          }
-          else {
-            CacheLayer.onSuccess( cacheKey );
-          }
-        }
-      }
-
-      outputToConsole && console.log( 'QUnit: no problems detected' );
+      // TODO: execute errors here
+      const results = await execute( 'grunt', [ 'compare-phet-io-api' ], `../${stableSims[ i ]}` );
+      console.log( 'success' );
+    }
+    catch( error ) {
+      console.log( error );
     }
   }
-  catch( e ) {
-    console.log( e );
-  }
 } )();
\ No newline at end of file

The text was updated successfully, but these errors were encountered:

samreid · 2022-09-09T16:02:31Z

I agree that would be helpful for our current process with the main drawback being the time to regenerate the API each time. But I also started brainstorming notes in https://github.com/phetsims/phet-io-wrappers/issues/555 about the fact that we may not want to continue with checking in API files all the time anyways, if we can move toward a more migration-oriented approach. Then this issue could morph into "on precommit, make sure there are migration rules to accompany API changes" somehow.

jessegreenberg · 2022-09-12T20:19:10Z

OK, thanks for commenting! I tried for a while to use execute to run grunt compare-phet-io-api for repos with stable apis but for some reason could not get execute to work with grunt. I am starting to see diminishing returns from this so I will let it go. And it also sounds like there may be some process changes coming related to this so Ill close.

samreid · 2022-09-14T15:14:43Z

@zepumph and @pixelzoom and I discussed this yesterday, I'd like to self-assign for consideration.

samreid · 2022-09-22T18:14:37Z

From today's meeting:

✓JB: At the moment I’m writing this, I just addressed a CT error related to the phet-io API, see CT: Uncaught Error: Assertion failed: Designed API changes detected, please roll them back or revise the reference API sun#790. There are a number of similar failures for other sims on CT at the moment. Can we take a moment to review what the phet-io API is, why we need it, and what we developers are supposed to do to keep it from getting outdated and causing CT errors?
- SR: CT API checking was intended to raise awareness for when devs accidentally change the PhET-iO API of something that was supposed to be stable. I don’t think this process has been working well. Here are two ends of a spectrum–not sure which is more appropriate at the moment
  - SR: (A) We are in a (hopefully temporary?) period of rapid evolution. I wonder if we should turn off CT checking of API files until things stabilize again?
    - CK: It feels like this has been the sentiment for 2-3 years, would it be risky to turn off CT checking?
  - SR: (B) Alternatively, we could add precommit hooks for this to make it more obvious what is happening before committing.
  - SR: Also, it’s unclear what the relationship is between API files and migration rules. Perhaps we should be able to change APIs anytime we want, as long as we specify a compensatory migration rule (as opposed to a new API).
  - SR: Both of these topics are discussed in Should pre-commit hooks compare PhET-iO APIs? #1325
  - MS: What are these API files?
    - SR gave a tour of GAO’s API file
    - JB: Are these only being used by external devs writing wrappers?
      - Yes, when sims are republished, they have to migrate their wrappers
- Should we keep going as we are, stop checking these in CT for a few months, or start precommit hooks for them?
  - AV
  - CM (out)
  - CK unsure of the consequences of not checking, also not affecting me rn
  - JB: fine with me to stop checking, but I’m not sure if there are consequences
  - JG: Fine to disable API checking for now. I trust the PhET-iO team to decide!
  - JO: fine to stop checking for now
  - KP
  - LM this doesn't affect me; no preference
  - LV
  - MK (out)
  - MP
  - MS - ??? Hasn’t hit me yet so not sure…
  - SR - Not sure. Maybe stop checking for now???? Or precommit hooks.
  - MB
  - MV - I havent run across it yet.
  - SR: I’ll open an issue to disable CT tests for now, and when we bring it back, it will hopefully be with precommit hooks.
    - The complexity with precommit hooks is that: (a) common code will require testing all dependent sims and (b) managing the time–don’t want precommit hooks taking that long.
    - JO: Testing everything could be a precommit hook of 15 minutes.
      - KP: We could just test a subset of sims.

pixelzoom · 2022-09-22T18:30:26Z

I don't agree with the decision to turn off CT testing of APIs. And I find it strange that you've made that decision without consulting the person (me) who is responsible for the majority of the sims in the current batch release https://github.com/orgs/phetsims/projects/44. It's easy for developers who aren't trying to get things ready for this release to save "I don't care about API changes". For me, unintentional API changes have been disruptive and confusing. Addressing the problem by hiding it is not helpful.

samreid · 2022-09-22T18:49:04Z

We discussed this further with @pixelzoom @kathy-phet @jonathanolson and @arouinfar. We agreed to:

Leave testing enabled
See if we can get a fast-enough precommit hook
Improve education for the team about the API files and how to deal with them.

We are hoping to run checks in 15 seconds or less (of additional time).

samreid · 2022-09-22T19:28:38Z

Also, designers will need to be able to regenerate API files since they develop the overrides, like in phetsims/gravity-and-orbits#440

arouinfar · 2022-09-22T19:36:58Z

Also, designers will need to be able to regenerate API files since they develop the overrides.

@samreid can you provide more information? I do not use my local development environment to make commits. I do it directly through the GitHub web interface.

samreid · 2022-09-22T21:33:47Z

I added pre-commit hooks that test to make sure there was no change to the PhET-iO API file for any repo that depends on the repo. If there is any change, the pre-commit hook will fail and the developer will need to check why the API has changed. If it is a desirable change and the developer has already regenerated the API file, the pre-commit hook will pass.

On my Macbook air M1, testing one repo takes around 5 seconds. Testing sun which compares 8 repos takes about 13.5 seconds.

Results are cached based on the batch. If one batch has "friction, gravity and orbits, states of matter" and another batch has "friction, gravity and orbits, and states of matter basics" that would be a cache miss. That will probably be OK for now.

Let's try this and see how it goes for a while. I'll schedule a dev meeting agenda item to discuss it with the team, and I'll also mention it on slack since it is live.

liammulh · 2022-09-29T18:00:39Z

See meeting notes for Sept. 29.

pixelzoom · 2022-09-29T18:50:22Z

@liammulh are these the "meeting notes" that you're referring to? In the PhET Developer Meeting 2022 doc?

CM: Let’s leave the CT tests for API files enabled, and empower the team to deal with it accordingly.
- SR: 5 more seconds OK?
  - 5 more seconds for pre-commit hook
  - batch is ~15 seconds
  - in master as of this writing
- CM: That would be OK. But more importantly, informing the team about dealing with the API files.
- JB: Who is maintaining the list of sims for which the pre-commit hook that validates the API files?
  - SR is maintaining this list.
  - perennial/data/phet-io-api-stable
- AV: What are the API files?
  - Gives elements, metadata for PhET-iO.
- JB: This is on now?
  - SR: Yes, it's been on for a week.

liammulh · 2022-09-29T18:53:08Z

Yes, thank you, @pixelzoom.

zepumph · 2022-10-04T16:02:21Z

@samreid there is no one assigned to this and it is "done" in the dev meeting project board. What is next here? Is phetsims/perennial@6c06470 our tactic for "fast-enough" pre-commit hooks? That will not scale. If we close now we will just need to discuss this again in some months or years. What about a list of sims that is pruned that we use for pre-commit. This "shortened" list can be a living list of the sims that embody the most churn or possibility for breakage. Then we can still have a stable list that takes all designed sims that we use to test on CT. That should cover the vast majority of what this issue is trying to solve. What do you think?

samreid · 2022-10-10T22:14:03Z

Perhaps a dev meeting for check-in or scheduling a subteam is the next step? But not sure if that topic should be "should we prune the PhET-iO APIs listed in the precommit checks" or "how can we improve the precommit hook developer experience??

matthew-blackman · 2022-10-13T18:39:28Z

@samreid will revisit the caching of files, especially PhET-IO API files. We are discussing the idea of a log of the time it takes to run precommit hooks. #1342

In keeping a pruned list, we would want to prioritize 3 or 4 stable PhET-IO sims that include full coverage of common code components used in PhET-IO sims.

zepumph · 2022-10-13T18:41:23Z

We want to further investigate how the api generation caching works. We can improve this to mostly optimize across sharing results when committing to many repos in the same commit.

samreid · 2022-10-17T21:36:11Z

I switched to per-repo caching. It made things simpler and seems to be working well. I tested by running node ../chipper/js/scripts/hook-pre-commit.js --console in gravity-and-orbits, then again in sun, and saw that sun no longer needed to test gravity-and-orbits with its batch. It seems the next step is discussion of pruned lists.

samreid · 2022-12-21T17:25:17Z

This issue falls in the scope of #1350 and does not need a separate entry in the "subgroups" column, so I'll move it to "done discussing".

…date defaults, see #1325

samreid · 2023-02-21T20:10:47Z

@zepumph and @matthew-blackman and I noticed that the API comparison tests weren't running like we thought, so we updated that in the commits. Being listed in perennial/phet-io-api-stable means it will check for breaking API changes in precommit hooks and on CT.

marlitas · 2023-03-06T03:20:34Z

While doing the review work for this issue I kept running into the below pre-commit-hook error

Seems related to the changes @zepumph made in phetsims/natural-selection@482f421

These went away after running grunt generate-phet-io-api, however no api files changed.
@pixelzoom and I investigated and couldn't understand why these errors are occurring in the first place.

@samreid could this be the result of caching? It seemed like unexpected results the whole way through the debugging process without reaching an answer. Is there more insight you can provide?

natural-selection BREAKING PROBLEMS
naturalSelection.introScreen.view.graphs.proportionsNode.proportionsGraphNode.columns.earsColumn.endBarNode.mutantPercentageText.stringProperty._data.initialState differs. 
Expected:
{"units":null,"validValues":null,"value":"‪NaN%‬"}
 actual:
{"value":"","validValues":null,"units":null}

naturalSelection.introScreen.view.graphs.proportionsNode.proportionsGraphNode.columns.earsColumn.endBarNode.normalPercentageText.stringProperty._data.initialState differs. 
Expected:
{"units":null,"validValues":null,"value":"‪NaN%‬"}
 actual:
{"value":"","validValues":null,"units":null}

naturalSelection.introScreen.view.graphs.proportionsNode.proportionsGraphNode.columns.earsColumn.startBarNode.mutantPercentageText.stringProperty._data.initialState differs. 
Expected:
{"units":null,"validValues":null,"value":"‪NaN%‬"}
 actual:
{"value":"","validValues":null,"units":null}

naturalSelection.introScreen.view.graphs.proportionsNode.proportionsGraphNode.columns.earsColumn.startBarNode.normalPercentageText.stringProperty._data.initialState differs. 
Expected:
{"units":null,"validValues":null,"value":"‪NaN%‬"}
 actual:
{"value":"","validValues":null,"units":null}

naturalSelection.introScreen.view.graphs.proportionsNode.proportionsGraphNode.columns.furColumn.endBarNode.mutantPercentageText.stringProperty._data.initialState differs. 
Expected:
{"units":null,"validValues":null,"value":"‪NaN%‬"}
 actual:
{"value":"","validValues":null,"units":null}

naturalSelection.introScreen.view.graphs.proportionsNode.proportionsGraphNode.columns.furColumn.endBarNode.normalPercentageText.stringProperty._data.initialState differs. 
Expected:
{"units":null,"validValues":null,"value":"‪NaN%‬"}
 actual:
{"value":"","validValues":null,"units":null}

naturalSelection.introScreen.view.graphs.proportionsNode.proportionsGraphNode.columns.furColumn.startBarNode.mutantPercentageText.stringProperty._data.initialState differs. 
Expected:
{"units":null,"validValues":null,"value":"‪NaN%‬"}
 actual:
{"value":"","validValues":null,"units":null}

naturalSelection.introScreen.view.graphs.proportionsNode.proportionsGraphNode.columns.furColumn.startBarNode.normalPercentageText.stringProperty._data.initialState differs. 
Expected:
{"units":null,"validValues":null,"value":"‪NaN%‬"}
 actual:
{"value":"","validValues":null,"units":null}

naturalSelection.introScreen.view.graphs.proportionsNode.proportionsGraphNode.columns.teethColumn.endBarNode.mutantPercentageText.stringProperty._data.initialState differs. 
Expected:
{"units":null,"validValues":null,"value":"‪NaN%‬"}
 actual:
{"value":"","validValues":null,"units":null}

naturalSelection.introScreen.view.graphs.proportionsNode.proportionsGraphNode.columns.teethColumn.endBarNode.normalPercentageText.stringProperty._data.initialState differs. 
Expected:
{"units":null,"validValues":null,"value":"‪NaN%‬"}
 actual:
{"value":"","validValues":null,"units":null}

naturalSelection.introScreen.view.graphs.proportionsNode.proportionsGraphNode.columns.teethColumn.startBarNode.mutantPercentageText.stringProperty._data.initialState differs. 
Expected:
{"units":null,"validValues":null,"value":"‪NaN%‬"}
 actual:
{"value":"","validValues":null,"units":null}

naturalSelection.introScreen.view.graphs.proportionsNode.proportionsGraphNode.columns.teethColumn.startBarNode.normalPercentageText.stringProperty._data.initialState differs. 
Expected:
{"units":null,"validValues":null,"value":"‪NaN%‬"}
 actual:
{"value":"","validValues":null,"units":null}

naturalSelection.labScreen.view.graphs.proportionsNode.proportionsGraphNode.columns.earsColumn.endBarNode.mutantPercentageText.stringProperty._data.initialState differs. 
Expected:
{"units":null,"validValues":null,"value":"‪NaN%‬"}
 actual:
{"value":"","validValues":null,"units":null}

naturalSelection.labScreen.view.graphs.proportionsNode.proportionsGraphNode.columns.earsColumn.endBarNode.normalPercentageText.stringProperty._data.initialState differs. 
Expected:
{"units":null,"validValues":null,"value":"‪NaN%‬"}
 actual:
{"value":"","validValues":null,"units":null}

naturalSelection.labScreen.view.graphs.proportionsNode.proportionsGraphNode.columns.earsColumn.startBarNode.mutantPercentageText.stringProperty._data.initialState differs. 
Expected:
{"units":null,"validValues":null,"value":"‪NaN%‬"}
 actual:
{"value":"","validValues":null,"units":null}

naturalSelection.labScreen.view.graphs.proportionsNode.proportionsGraphNode.columns.earsColumn.startBarNode.normalPercentageText.stringProperty._data.initialState differs. 
Expected:
{"units":null,"validValues":null,"value":"‪NaN%‬"}
 actual:
{"value":"","validValues":null,"units":null}

naturalSelection.labScreen.view.graphs.proportionsNode.proportionsGraphNode.columns.furColumn.endBarNode.mutantPercentageText.stringProperty._data.initialState differs. 
Expected:
{"units":null,"validValues":null,"value":"‪NaN%‬"}
 actual:
{"value":"","validValues":null,"units":null}

naturalSelection.labScreen.view.graphs.proportionsNode.proportionsGraphNode.columns.furColumn.endBarNode.normalPercentageText.stringProperty._data.initialState differs. 
Expected:
{"units":null,"validValues":null,"value":"‪NaN%‬"}
 actual:
{"value":"","validValues":null,"units":null}

naturalSelection.labScreen.view.graphs.proportionsNode.proportionsGraphNode.columns.furColumn.startBarNode.mutantPercentageText.stringProperty._data.initialState differs. 
Expected:
{"units":null,"validValues":null,"value":"‪NaN%‬"}
 actual:
{"value":"","validValues":null,"units":null}

naturalSelection.labScreen.view.graphs.proportionsNode.proportionsGraphNode.columns.furColumn.startBarNode.normalPercentageText.stringProperty._data.initialState differs. 
Expected:
{"units":null,"validValues":null,"value":"‪NaN%‬"}
 actual:
{"value":"","validValues":null,"units":null}

naturalSelection.labScreen.view.graphs.proportionsNode.proportionsGraphNode.columns.teethColumn.endBarNode.mutantPercentageText.stringProperty._data.initialState differs. 
Expected:
{"units":null,"validValues":null,"value":"‪NaN%‬"}
 actual:
{"value":"","validValues":null,"units":null}

naturalSelection.labScreen.view.graphs.proportionsNode.proportionsGraphNode.columns.teethColumn.endBarNode.normalPercentageText.stringProperty._data.initialState differs. 
Expected:
{"units":null,"validValues":null,"value":"‪NaN%‬"}
 actual:
{"value":"","validValues":null,"units":null}

naturalSelection.labScreen.view.graphs.proportionsNode.proportionsGraphNode.columns.teethColumn.startBarNode.mutantPercentageText.stringProperty._data.initialState differs. 
Expected:
{"units":null,"validValues":null,"value":"‪NaN%‬"}
 actual:
{"value":"","validValues":null,"units":null}

naturalSelection.labScreen.view.graphs.proportionsNode.proportionsGraphNode.columns.teethColumn.startBarNode.normalPercentageText.stringProperty._data.initialState differs. 
Expected:
{"units":null,"validValues":null,"value":"‪NaN%‬"}
 actual:
{"value":"","validValues":null,"units":null}



natural-selection DESIGN PROBLEMS

pixelzoom · 2023-03-06T03:36:45Z

The errors that @marlitas reported are presumably coming from the phetioCompareAPIs.js task -- that's the only place that I could find _data.initialState. What's curious is that every one of those errors is from ProportionsBarNode.ts, and they correspond to the lines that @zepumph changed in phetsims/natural-selection@482f421.

Here's an example of ProportionsBarNode in the Intro screen; they are the green bars with "{{value}}%" labels above them.

Every error that @marlitas reported is related to the "{{value}}%" label. And those errors are all identical:

Expected:
{"units":null,"validValues":null,"value":"‪NaN%‬"}
actual:
{"value":"","validValues":null,"units":null}

I don't understand why the "value" was expected to be "NaN%‬", and why is was actually "".

samreid · 2023-03-06T15:59:12Z

@zepumph volunteered to take a look

zepumph · 2023-03-06T17:24:43Z

I don't understand why the "value" was expected to be "NaN%‬

Looks like the initial value of these bar graphs involves dividing by 0, because there are 0 total bunnies. I didn't get a callback back to set these again until "adding a mate" which put the total at 2. This doesn't seem like a problem to me unless @pixelzoom wants to fix it. It is just nice to. . . "understand the NaN"(™). @pixelzoom please create an NS issue if you would prefer a different initial value (I personally wouldn't do that work).

I cannot reproduce the original issue with an empty string as the "actual" value. Next steps are to work with @marlitas to see if she can still reproduce.

In the future I think these kinds of bugs would be better if created as standalone issues, rather than contributing to this already giant issue.

zepumph · 2023-03-06T17:25:33Z

@marlitas over to you to schedule some time with me to reproduce.

marlitas · 2023-03-06T22:05:08Z

@zepumph I have tried reproducing on my end by checking out the commit as a branch, and I can't reproduce. Maybe there's a better way to try this, but I'm skeptical since after running grunt generate-phet-io-api there were no file changes but the error went away. Because of that I don't know what state the code needs to be in to hit that error...

Happy to meet tomorrow and try this together as well.

zepumph · 2023-03-06T22:57:42Z

My guess is a caching problem or something weird with the transpiler. If you encounter it again please open a separate issue.

As for the main point of this issue, I believe all that I have left here is covered by #1350 (comment) (test less sims for pre-commit-hooks).

@samreid, I'm ready to close. How about you?

jessegreenberg self-assigned this Sep 9, 2022

jessegreenberg closed this as completed Sep 12, 2022

samreid reopened this Sep 14, 2022

samreid assigned samreid and unassigned jessegreenberg Sep 14, 2022

samreid added the status:on-hold label Sep 22, 2022

samreid added a commit to phetsims/perennial that referenced this issue Sep 22, 2022

Prune phet-io-api-stable list, see phetsims/chipper#1325

6c06470

samreid removed the status:on-hold label Sep 22, 2022

samreid added the dev:phet-io label Sep 22, 2022

samreid added a commit that referenced this issue Sep 22, 2022

Check PhET-iO API files in pre-commit hooks, see #1325

be73dc6

samreid added a commit to phetsims/states-of-matter-basics that referenced this issue Sep 23, 2022

Prune API stable sims, see phetsims/chipper#1325

6bcd8a9

samreid added a commit to phetsims/sun that referenced this issue Sep 23, 2022

Prune API stable sims, see phetsims/chipper#1325

26f43a5

samreid added a commit to phetsims/states-of-matter that referenced this issue Sep 23, 2022

Prune API stable sims, see phetsims/chipper#1325

b565c73

samreid added a commit to phetsims/density that referenced this issue Sep 23, 2022

Prune API stable sims, see phetsims/chipper#1325

9a7f34d

samreid added a commit to phetsims/energy-forms-and-changes that referenced this issue Sep 23, 2022

Prune API stable sims, see phetsims/chipper#1325

de6d280

samreid removed their assignment Sep 27, 2022

zepumph assigned samreid Oct 4, 2022

zepumph removed their assignment Oct 10, 2022

samreid removed their assignment Oct 10, 2022

zepumph mentioned this issue Oct 13, 2022

pre-commit-hooks should keep a log for diagnostics #1342

Closed

samreid added a commit that referenced this issue Oct 17, 2022

Cache phet-io-api-comparison results on a repo-by-repo basis, see #1325

6f70287

samreid mentioned this issue Oct 29, 2022

Improve precommit hook development experience #1350

Open

samreid added a commit that referenced this issue Feb 21, 2023

Also compareBreakingAPIChanges, see #1325

6b22854

samreid added a commit that referenced this issue Feb 21, 2023

Check for breaking API changes when running ?phetioCompareAPI, and up…

e1a210a

…date defaults, see #1325

marlitas assigned samreid Mar 6, 2023

marlitas mentioned this issue Mar 6, 2023

Rename (Rich)Text.text => string phetsims/scenery#1472

Closed

8 tasks

samreid assigned zepumph and unassigned samreid Mar 6, 2023

zepumph assigned marlitas and unassigned zepumph Mar 6, 2023

zepumph added the status:ready-for-review label Mar 6, 2023

zepumph assigned samreid and unassigned marlitas Mar 6, 2023

samreid closed this as completed Mar 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Should pre-commit hooks compare PhET-iO APIs? #1325

Should pre-commit hooks compare PhET-iO APIs? #1325

jessegreenberg commented Sep 9, 2022

samreid commented Sep 9, 2022

jessegreenberg commented Sep 12, 2022

samreid commented Sep 14, 2022

samreid commented Sep 22, 2022

pixelzoom commented Sep 22, 2022

samreid commented Sep 22, 2022 •

edited

Loading

samreid commented Sep 22, 2022

arouinfar commented Sep 22, 2022

samreid commented Sep 22, 2022

liammulh commented Sep 29, 2022

pixelzoom commented Sep 29, 2022

liammulh commented Sep 29, 2022

zepumph commented Oct 4, 2022

samreid commented Oct 10, 2022

matthew-blackman commented Oct 13, 2022

zepumph commented Oct 13, 2022 •

edited by samreid

Loading

samreid commented Oct 17, 2022

samreid commented Dec 21, 2022

samreid commented Feb 21, 2023

marlitas commented Mar 6, 2023 •

edited by pixelzoom

Loading

pixelzoom commented Mar 6, 2023

samreid commented Mar 6, 2023

zepumph commented Mar 6, 2023 •

edited

Loading

zepumph commented Mar 6, 2023

marlitas commented Mar 6, 2023

zepumph commented Mar 6, 2023

Should pre-commit hooks compare PhET-iO APIs? #1325

Should pre-commit hooks compare PhET-iO APIs? #1325

Comments

jessegreenberg commented Sep 9, 2022

samreid commented Sep 9, 2022

jessegreenberg commented Sep 12, 2022

samreid commented Sep 14, 2022

samreid commented Sep 22, 2022

pixelzoom commented Sep 22, 2022

samreid commented Sep 22, 2022 • edited Loading

samreid commented Sep 22, 2022

arouinfar commented Sep 22, 2022

samreid commented Sep 22, 2022

liammulh commented Sep 29, 2022

pixelzoom commented Sep 29, 2022

liammulh commented Sep 29, 2022

zepumph commented Oct 4, 2022

samreid commented Oct 10, 2022

matthew-blackman commented Oct 13, 2022

zepumph commented Oct 13, 2022 • edited by samreid Loading

samreid commented Oct 17, 2022

samreid commented Dec 21, 2022

samreid commented Feb 21, 2023

marlitas commented Mar 6, 2023 • edited by pixelzoom Loading

pixelzoom commented Mar 6, 2023

samreid commented Mar 6, 2023

zepumph commented Mar 6, 2023 • edited Loading

zepumph commented Mar 6, 2023

marlitas commented Mar 6, 2023

zepumph commented Mar 6, 2023

samreid commented Sep 22, 2022 •

edited

Loading

zepumph commented Oct 13, 2022 •

edited by samreid

Loading

marlitas commented Mar 6, 2023 •

edited by pixelzoom

Loading

zepumph commented Mar 6, 2023 •

edited

Loading