phetsims / axon

Axon provides powerful and concise models for interactive simulations, based on observable Properties and related patterns.
MIT License
11 stars 8 forks source link

NumberProperty should validate for NaN automatically #426

Closed zepumph closed 1 year ago

zepumph commented 1 year ago

I ran into this in https://github.com/phetsims/friction/issues/313 which was exposed from work in https://github.com/phetsims/axon/issues/425. I think this makes sense. All phet brand fuzzing passed, so I think we can commit.

zepumph commented 1 year ago

I'll check back to see if CT is clear tomorrow.

zepumph commented 1 year ago

One showed up on CT:


curve-fitting : fuzz : unbuilt
https://bayes.colorado.edu/continuous-testing/ct-snapshots/1670274270337/curve-fitting/curve-fitting_en.html?continuousTest=%7B%22test%22%3A%5B%22curve-fitting%22%2C%22fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1670274270337%22%2C%22timestamp%22%3A1670277987823%7D&brand=phet&ea&fuzz&memoryLimit=1000
Query: brand=phet&ea&fuzz&memoryLimit=1000
Uncaught Error: Assertion failed: validation failed:
Property value not valid: Failed validation for validators[0]: Should not be NaN: value failed isValidValue: NaN
prunedValidator:
[object Object]
Error: Assertion failed: validation failed:
at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1670274270337/assert/js/assert.js:28:13)
at assert (validate.ts:25:16)
at validate (ReadOnlyProperty.ts:205:33)
at listener (TinyEmitter.ts:95:8)
at emit (ReadOnlyProperty.ts:311:22)
at _notifyListeners (ReadOnlyProperty.ts:262:13)
at unguardedSet (ReadOnlyProperty.ts:246:11)
at set (Property.ts:54:10)
at (Curve.js:232:6)
at updateRAndChiSquared (Curve.js:119:9)
id: Bayes Puppeteer
Snapshot from 12/5/2022, 2:04:30 PM
zepumph commented 1 year ago

This one is kinda fun, NaN was an acceptable and supported value for the rSquaredProperty, so I made it a Property. I think that is quite reasonable to gain the support across the whole project for NumberProperty.

zepumph commented 1 year ago

CT looks clear, closing

zepumph commented 1 year ago

I would like to do this for Vector2Property x/y values also.

zepumph commented 1 year ago

Local testing is passing, but I will check back in on CT soon.

zepumph commented 1 year ago

CT looks clear!