phetsims / sun

User-interface components for PhET simulations, built on top of Scenery.
MIT License
4 stars 12 forks source link

CT: Uncaught Error: Assertion failed: Designed API changes detected, please roll them back or revise the reference API #790

Closed jbphet closed 1 year ago

jbphet commented 1 year ago

There is a CT failure on sun, and it has been there since yesterday, so I should probably investigate. Here's the error:

``` sun : phet-io-api-compatibility : unbuilt https://bayes.colorado.edu/continuous-testing/ct-snapshots/1663690747459/sun/sun_en.html?continuousTest=%7B%22test%22%3A%5B%22sun%22%2C%22phet-io-api-compatibility%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1663690747459%22%2C%22timestamp%22%3A1663696359086%7D&ea&brand=phet-io&phetioStandalone&phetioCompareAPI&randomSeed=332211 Query: ea&brand=phet-io&phetioStandalone&phetioCompareAPI&randomSeed=332211 Uncaught Error: Assertion failed: Designed API changes detected, please roll them back or revise the reference API: New PhET-iO Element not in reference: sun.general.model.displayedSimNameProperty.dependencies.sun-general-model-screens-availableScreensProperty New PhET-iO Element not in reference: sun.general.model.displayedSimNameProperty.dependencies.sun-general-model-screens-selectedScreenProperty New PhET-iO Element not in reference: sun.general.model.displayedSimNameProperty.dependencies.sun-general-model-strings-joist-simTitleWithScreenNamePatternStringProperty sun.general.model.preferencesModel.inputModel.gestureControlsEnabledProperty.phetioReadOnly changed from true to false sun.general.model.preferencesModel.visualModel.interactiveHighlightsEnabledProperty.phetioReadOnly changed from true to false sun.general.model.preferencesModel.visualModel.toolbarEnabledProperty.phetioReadOnly changed from true to false sun.general.model.responseCollector.contextResponsesEnabledProperty.phetioReadOnly changed from true to false sun.general.model.responseCollector.hintResponsesEnabledProperty.phetioReadOnly changed from true to false sun.general.model.responseCollector.nameResponsesEnabledProperty.phetioReadOnly changed from true to false sun.general.model.responseCollector.objectResponsesEnabledProperty.phetioReadOnly changed from true to false sun.general.model.strings.joist.credits.contributorsStringProperty.phetioReadOnly changed from false to true sun.general.model.strings.joist.credits.graphicArtsStringProperty.phetioReadOnly changed from false to true sun.general.model.strings.joist.credits.leadDesignStringProperty.phetioReadOnly changed from false to true sun.general.model.strings.joist.credits.qualityAssuranceStringProperty.phetioReadOnly changed from false to true sun.general.model.strings.joist.credits.softwareDevelopmentStringProperty.phetioReadOnly changed from false to true sun.general.model.strings.joist.credits.soundDesignStringProperty.phetioReadOnly changed from false to true sun.general.model.strings.joist.credits.teamStringProperty.phetioReadOnly changed from false to true sun.general.model.strings.joist.credits.thanksStringProperty.phetioReadOnly changed from false to true sun.general.model.strings.joist.credits.titleStringProperty.phetioReadOnly changed from false to true sun.general.model.strings.joist.credits.translationStringProperty.phetioReadOnly changed from false to true sun.general.model.strings.joist.options.titleStringProperty.phetioReadOnly changed from false to true sun.general.model.strings.joist.termsPrivacyAndLicensingStringProperty.phetioReadOnly changed from false to true sun.general.model.strings.joist.thirdParty.credits.linkStringProperty.phetioReadOnly changed from false to true sun.general.model.strings.joist.translation.credits.linkStringProperty.phetioReadOnly changed from false to true sun.general.model.strings.joist.versionPatternStringProperty.phetioReadOnly changed from false to true sun.global.view.voicingManager.enabledProperty.enabledProperty.phetioReadOnly changed from true to false sun.global.view.voicingManager.mainWindowVoicingEnabledProperty.phetioReadOnly changed from true to false sun.global.view.voicingManager.voicePitchProperty.phetioReadOnly changed from true to false sun.global.view.voicingManager.voiceProperty.phetioReadOnly changed from true to false sun.global.view.voicingManager.voiceRateProperty.phetioReadOnly changed from true to false sun.global.view.voicingManager.voiceVolumeProperty.phetioReadOnly changed from true to false Error: Assertion failed: Designed API changes detected, please roll them back or revise the reference API: at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1663690747459/assert/js/assert.js:28:13) at assert (phetioEngine.ts:365:22) id: Bayes Puppeteer Snapshot from 9/20/2022, 10:19:07 AM ```
jbphet commented 1 year ago

I can duplicate the issue locally using the query parameter string in the issue, so my local URL is http://localhost:8080/sun/sun_en.html?ea&brand=phet-io&phetioStandalone&phetioCompareAPI&randomSeed=332211.

Running Running "generate-phet-io-api" task makes the problem go away locally, so I'll commit the resultant changes and keep an eye on CT and see if it clears up.

jbphet commented 1 year ago

CT is now clear. Closing.

jbphet commented 1 year ago

During today's developer meeting we discussed CT issues related to the phet-io API, and used this issue as an example. I mentioned that I fixed this CT issue without really investigating why the API had changed, and it was recommended that I dig a little deeper and make sure that the API change is okay. I'll reopen the issue.

jbphet commented 1 year ago

I've looked over the commit that resulted from the regeneration of the phet-io API file (see above), and the most significant portions seem to be related to availableScreensProperty, selectedScreenProperty, and simTitleWithScreenNamePatternStringProperty, which originate in joist. Since @samreid is the responsible dev for joist, I'm going to assign this issue to him to determine whether the API change that occurred here is expected and reasonable.

samreid commented 1 year ago

Looks like things were added and phetioReadOnly values were revised. The string changes are to be expected. @jessegreenberg can you comment on the a11y ones?

jessegreenberg commented 1 year ago

Sorry, I am not familiar with this change. @zepumph can you comment on whether these Properties related to Voicing should be phetioReadonly?

zepumph commented 1 year ago

@samreid the "a11y ones" are from https://github.com/phetsims/utterance-queue/commit/6a48435a915bd55435dd94cdb7fb2c9853e873e6.

Looks like sun wasn't updated when we added the new screen Properties as well.

The readonly string changes are from https://github.com/phetsims/joist/commit/1adfb13b169f9176343386497b913c3d313d19a7

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.

At least we have pre-commit hooks checking this stuff now. . .

samreid commented 1 year ago

I skimmed the API delta and wasn't worried about any of the a11y or readonly changes. If there is indeed a problem, I would hope it resurfaces during design review or QA testing. Closing.