Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
zepumph opened this issue Jan 8, 2024 · 3 comments
Closed

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

zepumph opened this issue Jan 8, 2024 · 3 comments

Comments

@zepumph
Copy link
Member

zepumph commented Jan 8, 2024

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

@zepumph
Copy link
Member Author

zepumph commented Jan 18, 2024

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
Copy link
Member Author

zepumph commented Jan 19, 2024

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.<Object.<string, Object>>} - keys are the repos, values are the APIs for each repo.
+ * @returns {Promise.<Object.<string, Object>>} - 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
Copy link
Member Author

zepumph commented Jan 22, 2024

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.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant