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

CT: Assertion failed: should not be in a before value list #175

Closed
pixelzoom opened this issue Aug 13, 2020 · 4 comments
Closed

CT: Assertion failed: should not be in a before value list #175

pixelzoom opened this issue Aug 13, 2020 · 4 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Aug 13, 2020

Caused by PhET-iO changes in phetsims/axon#316 (comment) for PropertyStateHandler.unregisterOrderDependenciesForProperty. On hold until that issue is fixed. This blocks publication of the next prototype RC.

natural-selection : phet-io-fuzz : unbuilt
https://bayes.colorado.edu/continuous-testing/ct-snapshots/1597313842663/natural-selection/natural-selection_en.html?continuousTest=%7B%22test%22%3A%5B%22natural-selection%22%2C%22phet-io-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1597313842663%22%2C%22timestamp%22%3A1597314830220%7D&ea&brand=phet-io&phetioStandalone&fuzz&memoryLimit=1000
Query: ea&brand=phet-io&phetioStandalone&fuzz&memoryLimit=1000
Uncaught Error: Assertion failed: should not be in a before value list
Error: Assertion failed: should not be in a before value list
at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1597313842663/assert/js/assert.js:22:13)
at https://bayes.colorado.edu/continuous-testing/ct-snapshots/1597313842663/axon/js/PropertyStateHandler.js:245:21
at Array.forEach (<anonymous>)
at PropertyStateHandler.unregisterOrderDependenciesForProperty (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1597313842663/axon/js/PropertyStateHandler.js:242:23)
at Property.dispose (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1597313842663/axon/js/Property.js:483:37)
at Property.PhetioObject.dispose (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1597313842663/tandem/js/PhetioObject.js:199:20)
at Bunny.Organism.disposeOrganism (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1597313842663/natural-selection/js/common/model/Organism.js:64:29)
at Bunny.dispose (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1597313842663/natural-selection/js/common/model/Organism.js:82:10)
at Bunny.dispose (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1597313842663/natural-selection/js/common/model/Bunny.js:173:11)
at Bunny.PhetioObject.dispose (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1597313842663/tandem/js/PhetioObject.js:199:20)
id: Bayes Chrome
Snapshot from 8/13/2020, 4:17:22 AM
@zepumph
Copy link
Member

zepumph commented Aug 13, 2020

Here is a large amount of logging I wrote out to try to get more info about this. It made me realize that there is a good change that I missed a case in the unregister case. I want to do a refactor back in phetsims/axon#316 that will likely make this easier to work on

Index: js/PropertyStateHandler.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/PropertyStateHandler.js	(revision 4e26beaf72d8abc505211d045f32814b827ee6ca)
+++ js/PropertyStateHandler.js	(date 1597362993045)
@@ -51,8 +51,8 @@
     // @private
     this.afterMaps = [
       this.undeferAfterUndeferMap,
-      this.undeferAfterNotifyMap,
       this.notifyAfterUndeferMap,
+      this.undeferAfterNotifyMap,
       this.notifyAfterNotifyMap
     ];
 
@@ -217,25 +217,45 @@
 
     const phetioIDToRemove = property.tandem.phetioID;
 
+    console.clear();
+    console.log( 'before', this.beforeMaps.map( map => new Map( map ) ) );
+    console.log( 'after', this.afterMaps.map( map => new Map( map ) ) );
     this.beforeMaps.forEach( beforeMap => {
       if ( beforeMap.has( phetioIDToRemove ) ) {
         beforeMap.get( phetioIDToRemove ).forEach( phetioID => {
           const setOfAfterMapIDs = this.getMapFromPhases( 'after', beforeMap.beforePhase, beforeMap.afterPhase ).get( phetioID );
+          if ( setOfAfterMapIDs && setOfAfterMapIDs.has( phetioIDToRemove ) ) {
+            console.log( 'deleting in Set:', phetioIDToRemove, ' from:', beforeMap.varName );
+          }
           setOfAfterMapIDs && setOfAfterMapIDs.delete( phetioIDToRemove );
         } );
       }
     } );
-    this.beforeMaps.forEach( map => map.delete( phetioIDToRemove ) );
+    this.beforeMaps.forEach( map => {
+      if ( map.has( phetioIDToRemove ) ) {
+        console.log( 'deleting key', phetioIDToRemove, '  from', map.varName );
+      }
+      map.delete( phetioIDToRemove );
+    } );
 
     this.afterMaps.forEach( afterMap => {
       if ( afterMap.has( phetioIDToRemove ) ) {
         afterMap.get( phetioIDToRemove ).forEach( phetioID => {
           const listOfBeforeMapIDs = this.getMapFromPhases( 'before', afterMap.beforePhase, afterMap.afterPhase ).get( phetioID );
+
+          if ( listOfBeforeMapIDs && listOfBeforeMapIDs.includes( phetioIDToRemove ) ) {
+            console.log( 'deleting in list:', phetioIDToRemove, ' from:', afterMap.varName );
+          }
           listOfBeforeMapIDs && arrayRemove( listOfBeforeMapIDs, phetioIDToRemove );
         } );
       }
     } );
-    this.afterMaps.forEach( map => map.delete( phetioIDToRemove ) );
+    this.afterMaps.forEach( map => {
+      if ( map.has( phetioIDToRemove ) ) {
+        console.log( 'deleting key', phetioIDToRemove, '  from', map.varName, '  value', map.get( phetioIDToRemove ) );
+      }
+      map.delete( phetioIDToRemove );
+    } );
 
     // TODO: we may want to remove this for performance, even though it is hidden behind assert, https://github.com/phetsims/axon/issues/316
     if ( assert ) {

@zepumph zepumph self-assigned this Aug 13, 2020
@zepumph
Copy link
Member

zepumph commented Aug 14, 2020

I have really no idea how, but after phetsims/axon@b6e00c9, I can't seem to reproduce this bug. It appears that NS is now fuzzing in phet-io brand. I'll wait to confirm with CT.

@pixelzoom
Copy link
Contributor Author

@zepumph is this an error that's possible to hit in brand=phet? If not, then this issue isn't blocking, because the prototype is brand=phet only.

@zepumph
Copy link
Member

zepumph commented Aug 14, 2020

I think it would have only been possible in phet-io brand. Anywho, this is fixed, and CT is clear since yesterday. Closing

@zepumph zepumph closed this as completed Aug 14, 2020
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

2 participants