phetsims / axon

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

Improve assertion messages for validation failures #442

Closed pixelzoom closed 7 months ago

pixelzoom commented 8 months ago

https://github.com/phetsims/keplers-laws/issues/197 took an unnecessarily long time to identify the problem because the assertion message wasn't useful. It contained a mountain of JSON, but almost no useful info to help identify the problematic NumberProperty.

Can we improve validation error messages by noting the phetioID of the offending Property? Or including other useful info like the rangeProperty.value when there is a range failure?

pixelzoom commented 8 months ago

In the above commit, I added the rangeProperty's value to the range validation assertion message.

pixelzoom commented 8 months ago

In https://github.com/phetsims/keplers-laws/issues/197#issuecomment-1777742416, @zepumph said:

I don't think https://github.com/phetsims/axon/commit/461f740de7cb67b6343b2dfb71ebc333307423ac will do it, it is just a string, not in a closure for the current value when the validator fails. Perhaps something like this instead?

Subject: [PATCH] add CompositeStateIOTypeChange, https://github.com/phetsims/phet-io-wrappers/issues/563
---
Index: js/NumberProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/NumberProperty.ts b/js/NumberProperty.ts
--- a/js/NumberProperty.ts  (revision fcc15b241f459acdd9aac596b9c6d3339fcd7646)
+++ b/js/NumberProperty.ts  (date 1698170089530)
@@ -115,7 +115,7 @@
     }
     options.validators.push( {
       isValidValue: v => rangeProperty.value.contains( v ),
-      validationMessage: 'Number must be within rangeProperty value.'
+      validationMessage: `Number must be within rangeProperty value. ${rangeProperty.isPhetioInstrumented() ? rangeProperty.phetioID : ''}`
     } );

     super( value, options );
pixelzoom commented 8 months ago

https://github.com/phetsims/axon/commit/461f740de7cb67b6343b2dfb71ebc333307423ac was not intended to add the phetioID. That change added the value of rangeProperty.

pixelzoom commented 8 months ago

Here's an example for Vector2Property validation failure: https://github.com/phetsims/my-solar-system/issues/267. Pages of JSON and no information that's really useful for isolating the problem.

zepumph commented 8 months ago

I can take a look at this starting next iteration, in a week.

zepumph commented 7 months ago

The above commit turned this amount of console logged junk:

``` Assertion failed: validation failed: Property value not valid: Failed validation for validators[1]: Number value: 0.0001 must be within rangeProperty value: [Range (min:100 max:400)]: value failed isValidValue: 0.0001 prunedValidator: { "valueType": "number", "phetioType": { "typeName": "NumberIO", "supertype": { "typeName": "ObjectIO", "documentation": "The root of the IO Type hierarchy", "methods": {}, "events": [], "metadataDefaults": { "phetioTypeName": "ObjectIO", "phetioDocumentation": "", "phetioState": true, "phetioReadOnly": false, "phetioEventType": "MODEL", "phetioHighFrequency": false, "phetioPlayback": false, "phetioDynamicElement": false, "phetioIsArchetype": false, "phetioFeatured": false, "phetioDesigned": false, "phetioArchetypePhetioID": null }, "dataDefaults": { "initialState": null }, "methodOrder": [], "parameterTypes": [], "validator": { "validationMessage": "Validation failed IOType Validator: ObjectIO" }, "defaultDeserializationMethod": "fromStateObject", "stateSchema": null, "isFunctionType": false }, "documentation": "IO Type for Javascript's number primitive type", "methods": {}, "events": [], "metadataDefaults": {}, "dataDefaults": {}, "methodOrder": [], "parameterTypes": [], "validator": { "valueType": "number", "validationMessage": "Validation failed IOType Validator: NumberIO" }, "defaultDeserializationMethod": "fromStateObject", "stateSchema": { "displayString": "number", "validator": {}, "compositeSchema": null }, "isFunctionType": false }, "validators": [ { "validationMessage": "Should not be NaN" }, {} ], "validationMessage": "Property value not valid" } ```

into this:

``` Assertion failed: validation failed: Property value not valid: Failed validation for validators[1]: Number value: 0.0001 must be within rangeProperty value: [Range (min:100 max:400)]: value failed isValidValue: 0.0001 prunedValidator: { "valueType": "number", "phetioType": { "validator": { "valueType": "number", "validationMessage": "Validation failed IOType Validator: NumberIO" }, "typeName": "NumberIO" }, "validators": [ { "validationMessage": "Should not be NaN" }, {} ] } ```
zepumph commented 7 months ago

Above validationMessage now supports functions so that you can include helpful closure variables like @pixelzoom's original commit. I tested with this patch. @pixelzoom please review.

Subject: [PATCH] add documentation for QueryParameters that need it, https://github.com/phetsims/tasks/issues/1127
---
Index: js/common/model/KeplersLawsModel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/KeplersLawsModel.ts b/js/common/model/KeplersLawsModel.ts
--- a/js/common/model/KeplersLawsModel.ts   (revision 9a918a902f73290eee5cd1340efc946a61e9acb8)
+++ b/js/common/model/KeplersLawsModel.ts   (date 1700247077465)
@@ -113,7 +113,7 @@
       defaultBodyInfo: [
         new BodyInfo( {
           isActive: true,
-          mass: KeplersLawsConstants.MASS_OF_OUR_SUN,
+          mass: .0001,
           massRange: new Range( 0.5 * KeplersLawsConstants.MASS_OF_OUR_SUN, 2 * KeplersLawsConstants.MASS_OF_OUR_SUN ),
           position: new Vector2( 0, 0 ),
           velocity: new Vector2( 0, 0 ),
zepumph commented 7 months ago

I wasn't in love with the way I moved the range validator until after the call anyways, but especially after seeing that unit tests failed that test on each validValue. Above I made things much nicer. Back to you @pixelzoom.

pixelzoom commented 7 months ago

👍🏻