phetsims / aqua

Automatic QUality Assurance
MIT License
2 stars 4 forks source link

PhET-iO API compatibility tests fail on non-puppeteer browsers #200

Closed zepumph closed 10 months ago

zepumph commented 10 months ago

This is because small things like pixel drawing may change an initial state positionProperty. For example, https://github.com/phetsims/gravity-and-orbits/issues/486. This was caused by turning firefox testing back on in #188.

zepumph commented 10 months ago

A productive conversation with @jonathanolson today helped me see that this isn't about the testing side of things (like "dont test api comparison on firefox"), but instead about the comparison test. Why is it so fragile to be browser-specific? Let's keep cracking at that, but first I will confirm that the above fixed the molecule shapes issue on ct.

zepumph commented 10 months ago

I had another good talk with @jonathanolson and @samreid. To come back to:

  1. General way to opt out of number comparison for phet-io api compare
  2. The most "api-like" part of the initialState is Property's validValues/units/etc that don't change for the whole instance.
  3. Still though, even the "value" of the Property is worth communicating to the client if it changes. We want to be able to know and track this.
  4. It would be nice if generating many apis was graceful if one failed, and was still able to update all the others.
  5. The heuristic above is not general enough. Nothing will ever be error proof. Imagine fonts changing position in a cross browser way.
  6. This comparison is just a dev feature for testing, so we control how much effort to put into workarounds vs solutions.
``` Subject: [PATCH] better shutoff for popupable, https://github.com/phetsims/sun/issues/855 --- Index: chipper/js/phet-io/generatePhetioMacroAPI.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/chipper/js/phet-io/generatePhetioMacroAPI.js b/chipper/js/phet-io/generatePhetioMacroAPI.js --- a/chipper/js/phet-io/generatePhetioMacroAPI.js (revision f2a5fd05182d7211f0a35a864d17d31d7f831867) +++ b/chipper/js/phet-io/generatePhetioMacroAPI.js (date 1705687129482) @@ -19,7 +19,7 @@ * Load each sim provided and get the * @param {string[]} repos * @param {Object} [options] - * @returns {Promise.>} - keys are the repos, values are the APIs for each repo. + * @returns {Promise.>} - keys are the repos, values are the APIs for each repo. If there was a problem with getting the API, then it will return null for that repo. */ const generatePhetioMacroAPI = async ( repos, options ) => { @@ -136,8 +136,7 @@ chunkResults.forEach( chunkResult => { if ( chunkResult.status === 'fulfilled' ) { - assert( chunkResult.value.api instanceof Object, 'api expected from Promise results' ); - macroAPI[ chunkResult.value.repo ] = chunkResult.value.api; + macroAPI[ chunkResult.value.repo ] = chunkResult.value.api || null; } else { console.error( 'Error in fulfilling chunk Promise:', chunkResult.reason ); Index: scenery/js/nodes/Text.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/scenery/js/nodes/Text.ts b/scenery/js/nodes/Text.ts --- a/scenery/js/nodes/Text.ts (revision fd2d5856f4a558a4a70dd284037b3f6d2ab5db7e) +++ b/scenery/js/nodes/Text.ts (date 1705685946237) @@ -383,6 +383,7 @@ if ( this.hasStroke() ) { selfBounds.dilate( this.getLineWidth() / 2 ); } + selfBounds.dilate( 5 ); const changed = !selfBounds.equals( this.selfBoundsProperty._value ); if ( changed ) {
zepumph commented 10 months ago

Alright. The above commit is working well for our current cases. I think that https://github.com/phetsims/phet-io/issues/1951 should stay open because it is likely to run into trouble in the future. For example, with this patch (changes text size everywhere), we see that Keplers Laws has an API regression based on the initial state of some positions. No matter. The work of this issue (making CT pass right now in firefox), has been solved. Closing.

```diff Subject: [PATCH] update TODO, https://github.com/phetsims/scenery-phet/issues/815 --- Index: js/nodes/Text.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/nodes/Text.ts b/js/nodes/Text.ts --- a/js/nodes/Text.ts (revision 618d3d782735ae81dcb6f28664b2fb40312c3eb7) +++ b/js/nodes/Text.ts (date 1705965907500) @@ -383,6 +383,7 @@ if ( this.hasStroke() ) { selfBounds.dilate( this.getLineWidth() / 2 ); } + selfBounds.dilate( 5 ); const changed = !selfBounds.equals( this.selfBoundsProperty._value ); if ( changed ) {