phetsims / assert Goto Github PK

In https://github.com/phetsims/phet-io-wrappers/issues/506#issuecomment-1561656667 @zepumph said:

Perhaps assert should console.error instead of console.log

I agree we should use console.error instead of console.log. Here's a picture of a test assertion with assert the way it is now:

Using console.error, it looks like this:

And you can pop open the triangle like so:

@zepumph or @jonathanolson ok to commit this change? Or should we check in at dev meeting today?

During phetsims/scenery#1407 I noticed around 10 occurrences like:

assert && assert( 'Need an active Node to update line width' );

Note that the string 'Need an active Node to update line width' is truthy, so this is equivalent to calling assert && assert(true) with no error message. Therefore it will never trigger. This can probably be also caught through type checking, but the type signature must allow string|null (for cases like assert(this.myNullableString,'string should not be null by now) so it may not be trivial. But a bad-text lint rule would be very easy. Here are the current failures:

Running "lint-everything" task

/Users/samreid/apache-document-root/main/circuit-construction-kit-common/js/view/CircuitElementNumberControl.ts
  84:15  error  Line contains bad text: 'assert( ''  bad-text

/Users/samreid/apache-document-root/main/equality-explorer/js/common/view/ObjectPicker.js
  368:21  error  Line contains bad text: 'assert( ''  bad-text

/Users/samreid/apache-document-root/main/neuron/js/neuron/model/NeuronModel.js
   852:17  error  Line contains bad text: 'assert( ''  bad-text
  1073:19  error  Line contains bad text: 'assert( ''  bad-text

/Users/samreid/apache-document-root/main/scenery-phet/js/NumberPicker.js
  532:21  error  Line contains bad text: 'assert( ''  bad-text

/Users/samreid/apache-document-root/main/scenery/js/input/SimpleDragHandler.js
  343:15  error  Line contains bad text: 'assert( ''  bad-text

/Users/samreid/apache-document-root/main/scenery/js/listeners/KeyboardDragListener.ts
  886:17  error  Line contains bad text: 'assert( ''  bad-text

/Users/samreid/apache-document-root/main/scenery/js/nodes/WebGLNode.ts
  152:15  error  Line contains bad text: 'assert( ''  bad-text

/Users/samreid/apache-document-root/main/scenery/js/overlays/HighlightOverlay.ts
  652:17  error  Line contains bad text: 'assert( ''  bad-text
  676:15  error  Line contains bad text: 'assert( ''  bad-text

✖ 10 problems (10 errors, 0 warnings)

Warning: 10 errors and 0 warnings Use --force to continue.

In phetsims/chipper#912 we discussed converting assert to an ES6 module.

we would be able to use assert in phet-io wrappers (if we add a build step that traverses modules).

We even discussed converting assert and initialize-globals and _ to be loaded via modules. It's not clear whether we would miss the ability to just access them as globals.

assert and QSM are both used in the phet-io front end library, and may need consideration before turning into modules. That said, devs didn't mind seeing these transformed.

Webstorm has been very nice at automatically and semi-automatically adding imports for ES6 modules as needed. Still, I'm not sure of the benefits vs risks/costs for this issue. Leaving unassigned for now.

Google chrome dev tools used to pause with a navigable stack frame for iframe assertion failures. However, in the last few months, it instead prints the iframe stack trace to the console instead, and you cannot access any of the stack frames or variable values. To work around this, I have been temporarily adding debugger to assert.js, but this feels hackish. I wonder if we should (a) find out how to get chrome to pause stack frames again in iframes or (b) add code to assert.js that basically says:

      console && console.log && console.log( 'Assertion failed: ' + message );
      if (isInIFrameCode()){
        debugger;
      }
      throw new Error( 'Assertion failed: ' + message );

When running with ?ea and the console open, it takes me longer than necessary to find the assertion message. How do others get around (or not have) this problem?

Should we print the assertion error to the console before throwing error?

Adding this line to assert.js looks like it will make my life much easier:

console.log( 'Assertion failed: ' + message );

Assigned to @jonathanolson for discussion, but @pixelzoom @jbphet @jessegreenberg and @phetsims/aadish may want to discuss as well.

From dev meeting today. We don't require that assertion messages are included in every assert call. Therefore it would be nice if the error message handled this case without saying: Assertion failed: undefined.

@jonathanolson had the recommendation to try to add the lineNum/file that the assert came from, otherwise we can just say Assertion failed or something.

While working on https://github.com/phetsims/studio/issues/253, @samreid and I really liked the idea of having assert support var args for message(s). This would equate well with the console.log before the error, which supports varargs also, then we can concat as strings for the error itself

Should just return 'true' if a function is passed.

I'll check for any places the old function behavior is being used, and will convert it.