phetsims / scenery-phet

Reusable components based on Scenery that are specific to PhET simulations.
MIT License
8 stars 6 forks source link

NumberAccumulator gives NaN errors in ?listenerOrder=reverse #833

Closed samreid closed 6 months ago

samreid commented 6 months ago

From CT:

``` collision-lab : fuzz : unbuilt : listenerOrderRandom http://128.138.93.172/continuous-testing/ct-snapshots/1705633142723/collision-lab/collision-lab_en.html?continuousTest=%7B%22test%22%3A%5B%22collision-lab%22%2C%22fuzz%22%2C%22unbuilt%22%2C%22listenerOrderRandom%22%5D%2C%22snapshotName%22%3A%22snapshot-1705633142723%22%2C%22timestamp%22%3A1705634355277%7D&brand=phet&ea&fuzz&listenerOrder=random Query: brand=phet&ea&fuzz&listenerOrder=random Uncaught Error: Assertion failed: reentry detected, value=8,7, oldValue=8 Error: Assertion failed: reentry detected, value=8,7, oldValue=8 at window.assertions.assertFunction (http://128.138.93.172/continuous-testing/ct-snapshots/1705633142723/assert/js/assert.js:28:13) at assert (ReadOnlyProperty.ts:324:14) at _notifyListeners (ReadOnlyProperty.ts:275:13) at unguardedSet (ReadOnlyProperty.ts:259:11) at set (Property.ts:65:10) at set (AbstractKeyAccumulator.ts:81:33) at updateKeys (NumberAccumulator.ts:124:42) at handleKeyPressed (Keypad.ts:344:35) at listener (TinyEmitter.ts:123:8) at emit (Emitter.ts:62:21) [URL] http://128.138.93.172/continuous-testing/aqua/html/sim-test.html?url=..%2F..%2Fct-snapshots%2F1705633142723%2Fcollision-lab%2Fcollision-lab_en.html&simQueryParameters=brand%3Dphet%26ea%26fuzz%26listenerOrder%3Drandom&testInfo=%7B%22test%22%3A%5B%22collision-lab%22%2C%22fuzz%22%2C%22unbuilt%22%2C%22listenerOrderRandom%22%5D%2C%22snapshotName%22%3A%22snapshot-1705633142723%22%2C%22timestamp%22%3A1705634355277%7D [NAVIGATED] http://128.138.93.172/continuous-testing/aqua/html/sim-test.html?url=..%2F..%2Fct-snapshots%2F1705633142723%2Fcollision-lab%2Fcollision-lab_en.html&simQueryParameters=brand%3Dphet%26ea%26fuzz%26listenerOrder%3Drandom&testInfo=%7B%22test%22%3A%5B%22collision-lab%22%2C%22fuzz%22%2C%22unbuilt%22%2C%22listenerOrderRandom%22%5D%2C%22snapshotName%22%3A%22snapshot-1705633142723%22%2C%22timestamp%22%3A1705634355277%7D [ATTACHED] [NAVIGATED] about:blank [NAVIGATED] http://128.138.93.172/continuous-testing/ct-snapshots/1705633142723/collision-lab/collision-lab_en.html?continuousTest=%7B%22test%22%3A%5B%22collision-lab%22%2C%22fuzz%22%2C%22unbuilt%22%2C%22listenerOrderRandom%22%5D%2C%22snapshotName%22%3A%22snapshot-1705633142723%22%2C%22timestamp%22%3A1705634355277%7D&brand=phet&ea&fuzz&listenerOrder=random [CONSOLE] enabling assert [CONSOLE] listenerOrder random seed: 61357 [CONSOLE] continuous-test-load [CONSOLE] Assertion failed: invalid number: NaN [PAGE ERROR] Error: Error: Assertion failed: invalid number: NaN at window.assertions.assertFunction (http://128.138.93.172/continuous-testing/ct-snapshots/1705633142723/assert/js/assert.js:28:13) at NumberAccumulator.stringToInteger (http://128.138.93.172/continuous-testing/ct-snapshots/1705633142723/chipper/dist/js/scenery-phet/js/keypad/NumberAccumulator.js:139:17) at http://128.138.93.172/continuous-testing/ct-snapshots/1705633142723/chipper/dist/js/scenery-phet/js/keypad/NumberAccumulator.js:58:97 at getDerivedValue (http://128.138.93.172/continuous-testing/ct-snapshots/1705633142723/chipper/dist/js/axon/js/DerivedProperty.js:32:12) at DerivedProperty.getDerivedPropertyListener (http://128.138.93.172/continuous-testing/ct-snapshots/1705633142723/chipper/dist/js/axon/js/DerivedProperty.js:121:17) at TinyProperty.emit (http://128.138.93.172/continuous-testing/ct-snapshots/1705633142723/chipper/dist/js/axon/js/TinyEmitter.js:96:9) at Property._notifyListeners (http://128.138.93.172/continuous-testing/ct-snapshots/1705633142723/chipper/dist/js/axon/js/ReadOnlyProperty.js:252:23) at Property.unguardedSet (http://128.138.93.172/continuous-testing/ct-snapshots/1705633142723/chipper/dist/js/axon/js/ReadOnlyProperty.js:201:14) at Property.set (http://128.138.93.172/continuous-testing/ct-snapshots/1705633142723/chipper/dist/js/axon/js/ReadOnlyProperty.js:186:12) at Property.set (http://128.138.93.172/continuous-testing/ct-snapshots/1705633142723/chipper/dist/js/axon/js/Property.js:58:11) [CONSOLE] continuous-test-error [CONSOLE] Assertion failed: reentry detected, value=, oldValue=DECIMAL,4 [PAGE ERROR] Error: Error: Assertion failed: reentry detected, value=, oldValue=DECIMAL,4 ```

Oh, it’s an example of https://github.com/phetsims/axon/issues/303 where we get a callback when the accumulated keys and string are out of sync, when the listener order is reversed.

samreid commented 6 months ago

I've been testing with these query parameters: http://localhost/collision-lab/collision-lab_en.html?brand=phet&ea&debugger&screens=1&listenerOrder=reverse&fuzz

This patch uncouples the DerivedProperty instances and seems to correct the fuzzing failure by avoiding the inconsistent intermediate state. I would like to request a review before committing.

```diff Subject: [PATCH] Convert isLauncherCustom to mysteryOrCustom, see https://github.com/phetsims/projectile-data-lab/issues/93 --- Index: js/keypad/NumberAccumulator.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/keypad/NumberAccumulator.ts b/js/keypad/NumberAccumulator.ts --- a/js/keypad/NumberAccumulator.ts (revision 8f6d0b45f5f8992e9298519098f667b5f1e34251) +++ b/js/keypad/NumberAccumulator.ts (date 1705677060194) @@ -80,8 +80,11 @@ this.valueProperty = new DerivedProperty( // this.accumulatedKeysProperty is used by this.stringToInteger - [ this.stringProperty, this.accumulatedKeysProperty ], - ( stringValue, accumulatedKeys ) => this.stringToInteger( stringValue ), { + [ this.accumulatedKeysProperty ], + accumulatedKeys => { + const stringValue = this.keysToString( accumulatedKeys ); + return this.stringToInteger( stringValue ); + }, { tandem: options.tandem.createTandem( 'valueProperty' ), phetioValueType: NullableIO( NumberIO ) } ); ```

@jbphet and @pixelzoom are listed as authors on NumberAccumulator, can one of you please volunteer to review? Feel free to commit if it is good.

pixelzoom commented 6 months ago

The above patch looks OK to me. But I'm not really familiar with NumberAccumulator, so @jbphet should doublecheck.

pixelzoom commented 6 months ago

A couple of other thoughts about the above patch:

(1) Add a comment that references this issue, so future devs will know why stringProperty is not used directly in the derivation.

(2) Consider moving stringProperty from NumberAcumulator to Keypad. That's the only place where it's currently used or needed. This would also eliminate the need for (1).

jbphet commented 6 months ago

I've committed the patch that @samreid suggested. This seems like a reasonable way to work around the dependency issues. I then added the comment suggested in item 1 above from @pixelzoom. I didn't do item 2 because the accumulators were designed to be the encapsulated place where keys are accumulated and values interpreted. That way user of this object don't need to know much about its nature, they can just deal with well-know things like strings and numbers.

I'm going to go ahead and close this, but @pixelzoom and @samreid are welcome to review and re-open if they feel the need.