Should pre-commit hooks compare PhET-iO APIs?

[jessegreenberg]

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).

```patch 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 ```

[samreid]

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]

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]

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

[samreid]

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 https://github.com/phetsims/sun/issues/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 ​​https://github.com/phetsims/chipper/issues/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]

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]

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]

Also, designers will need to be able to regenerate API files since they develop the overrides, like in https://github.com/phetsims/gravity-and-orbits/issues/440

[arouinfar]

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]

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]

See meeting notes for Sept. 29.

[pixelzoom]

@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]

Yes, thank you, @pixelzoom.

[zepumph]

@samreid there is no one assigned to this and it is "done" in the dev meeting project board. What is next here? Is https://github.com/phetsims/perennial/commit/6c0647099b364dcc1d9fb862282d858ea780e707 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?

[pixelzoom]

Just a note that this has been working nicely for me. It's caught a couple of inadvertent API changes that I made. And it has not had a noticeable impact on my commit times.

[samreid]

In https://github.com/phetsims/sun/issues/790#issuecomment-1268696241 @zepumph said:

I can't help but feel like part of the issue is removing many sims from the API checking conversation in https://github.com/phetsims/perennial/commit/6c0647099b364dcc1d9fb862282d858ea780e707.

[samreid]

@samreid there is no one assigned to this and it is "done" in the dev meeting project board. What is next here? Is https://github.com/phetsims/perennial/commit/6c0647099b364dcc1d9fb862282d858ea780e707 our tactic for "fast-enough" pre-commit hooks? That will not scale.

I'm glad the list has more complete coverage at the moment while we are making lots of common code changes, in the future when the list takes too long to check everything then we can introduce a pruned list. But I also feel one day we should do more comprehensive timing testing of the precommit hooks and make sure our time is being spent wisely.

[zepumph]

That is all fine and good, but doesn't really get to the bottom of if it was acceptable to remove the sims in https://github.com/phetsims/perennial/commit/6c0647099b364dcc1d9fb862282d858ea780e707 from our list. Are EFAC and SOM acceptable to have API changes because they haven't been published in a year and may not be for another year? That seems like a hard sell to the person that will have to do a full review of the API when next it is visited.

in the future when the list takes too long to check everything then we can introduce a pruned list.

What do you mean by "too long"? It is run by each dev potentially >10 times a day. Shouldn't it be perfectly sized from now in perpetuity?

[samreid]

That is all fine and good, but doesn't really get to the bottom of if it was acceptable to remove the sims in https://github.com/phetsims/perennial/commit/6c0647099b364dcc1d9fb862282d858ea780e707 from our list. Are EFAC and SOM acceptable to have API changes because they haven't been published in a year and may not be for another year?

That decision was made during the developer meeting and I'm surprised to see it was not well-documented in the notes. We removed EFAC and SOM because they were not in the "forward migration" channel of the spreadsheet. Sun was removed because of https://github.com/phetsims/scenery-phet/issues/439. I think density was removed since it hasn't had a phet-io release yet, but if I were the lead dev, I may opt to leave it checked so I could keep track of the changes.

What do you mean by "too long"? It is run by each dev potentially >10 times a day. Shouldn't it be perfectly sized from now in perpetuity?

I am not a big fan of the time it takes to run all the precommit hooks, but it seems the dev team generally votes in favor of more complete testing. Happy to discuss further, perhaps at an upcoming phet-io meeting may be most efficient?

[zepumph]

I am sorry I wasn't able to attend that meeting, because it likely would have cleared up much of this confusion on my end. Thanks for your patience.

I think density was removed since it hasn't had a phet-io release yet

Density has been released, and the "Batch release" was originally only to add Studio support for setting mystery blocks in that sim (oh how time has changed that).

Part of my frustration is that with your amazing CacheLayer implementation, I had ~2 weeks of incredibly nice and fast git hooks, and now it is back to >1-2 minutes per run. That makes me sad. Happy to go with what the team prefers, but I'm skeptical that others wouldn't prefer this middle ground proposed in https://github.com/phetsims/chipper/issues/1325#issuecomment-1267229939

[samreid]

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]

@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. https://github.com/phetsims/chipper/issues/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]

• [x] 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]

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]

This issue falls in the scope of https://github.com/phetsims/chipper/issues/1350 and does not need a separate entry in the "subgroups" column, so I'll move it to "done discussing".

[samreid]

@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]

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 https://github.com/phetsims/natural-selection/commit/482f421eb9213365e86c0e88ac2d0d3473e4f129

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]

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 https://github.com/phetsims/natural-selection/commit/482f421eb9213365e86c0e88ac2d0d3473e4f129.

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]

@zepumph volunteered to take a look

[zepumph]

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]

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

[marlitas]

@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]

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 https://github.com/phetsims/chipper/issues/1350#issuecomment-1344767576 (test less sims for pre-commit-hooks).

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