How to deal with assert.expect( 0 )?

[samreid]

In #30 we discussed assert.expect. Here's a case where it's used and currently necessary:

// Copyright 2017, University of Colorado Boulder

/**
 * QUnit Tests for BooleanProperty
 *
 * @author Sam Reid (PhET Interactive Simulations)
 */
define( function( require ) {
  'use strict';

  // modules
  var BooleanProperty = require( 'AXON/BooleanProperty' );

  QUnit.module( 'BooleanProperty' );
  QUnit.test( 'BooleanProperty', function( assert ) {
    window.assert && assert.throws( function() {
      new BooleanProperty( 'hello' ); //eslint-disable-line
    }, 'invalid initial value for BooleanProperty' ); // eslint-disable-line
    var c = new BooleanProperty( true );
    c.set( true );
    c.set( false );
    c.set( true );
    window.assert && assert.throws( function() {
      c.set( 123 );
    }, 'set an invalid value for BooleanProperty' );

    if ( !window.assert ) {
      assert.expect( 0 );
    }
  } );
} );

When assertions are enabled, two tests are run (the assert.throws calls). When assertions are disabled, 0 tests are run. If the assert.expect was commented out, QUnit would report a failure with this message:

Expected at least one assertion, but none were run - call expect(0) to accept zero assertions.

Two solutions for dealing with this problem are:

1. Use assert.expect(0) as we have been
2. Add a different assert test, such as assert.ok( c.value, 'boolean value should be true' ); which runs whether assertions are enabled or not.

(1) would fail if we add a test like (2). But (2) seems like a bit of a workaround to deal with QUnit expecting at least one test by default. @pixelzoom which do you recommend?

[pixelzoom]

Why do we need to run unit tests with assertions disabled?

[samreid]

It may be unnecessary to run unit tests with assertions disabled. Is there a class of errors that only manifests when assertions are off? If so, is it a frequent enough kind of error that we need to run tests with assertions disabled?

[pixelzoom]

Is there a class of errors that only manifests when assertions are off? If so, is it a frequent enough kind of error that we need to run tests with assertions disabled?

The only one I can think of is failure to use assert && before a call to assert. And that will certainly be noticed outside of unit tests.

[samreid]

The proposal to only run unit tests with assertions enabled sounds reasonable to me, but I want to run it past @jonathanolson before we proceed.

[pixelzoom]

If you must use this pattern:

    if ( !window.assert ) {
      assert.expect( n );
    }

... then you shouldn't be assuming that there are n tests that don't involve window.assert. You should be keeping a counter that gets incremented whenever any test is run, then check the counter:

assert.expect( numberOfTestsRun );

And yes, that sounds like a colossal error-prone pain.

[pixelzoom]

If we do decide to run tests with assertions enabled, then tests should verify that assert is enabled.

[samreid]

We are also planning to run unit tests on compiled code, where assertions cannot be enabled.

[jonathanolson]

We are also planning to run unit tests on compiled code, where assertions cannot be enabled.

Presumably only for scenery/kite/dot, not the simulations/runnables?

[samreid]

I'm getting confused. In this thread: #28 (comment)

@jonathanolson and I spoke this morning and decided we will skip the built tests for now. We haven't seen a "built-only" bug for a while so we will defer for now.

I meant only scenery/kite/dot. I think we do still occasionally see built sim issues. That looks like it was removed (I restarted on bayes).

Did you mean "We will only skip scenery/kite/dot and will continue to test built sims, since we see issues there"?

[pixelzoom]

I'm removing the call to assert.expect( 5 ) in NumberProperty, since it is unnecessary and confuses this discussion. Slack discussion with @samreid:

Chris Malley [10:27 AM]
I want to make sure that I understand assert.expect. Is the call in NumberPropertyTests necessary? I think not.

Sam Reid [10:29 AM]
It is not necessary because there is at least one other non-short-circuited assert.* call

Chris Malley [10:29 AM]
My understanding is that we only need assert.expect(0) to handle the case where no tests are run.

Sam Reid [10:29 AM]
For instance, the one on line 43 handles it.

Chris Malley [10:29 AM]
OK if I remove it?

Sam Reid [10:29 AM]
yes please

[pixelzoom]

Sam Reid [10:31 AM]
We could avoid the assert.expect() altogether if we add at least one non-short-circuited assert.* call to each test. Should we do that too?

Chris Malley [10:32 AM]
I think having a “dummy” test that is always run and succeeds is preferrable to having assert.expect(0). But surely someone else must have run into this problem….

[pixelzoom]

assert.ok( true, 'so we have at least 1 test in this set' );

[pixelzoom]

@jonathanolson I assume that this is also unnecessary in TrailTests.js:

680    // this may change if for some reason we end up calling more events in the future
681    assert.expect( 4 );

OK to remove it?

[samreid]

In TrailTests.js the assert.expect( 4 ) is checking that exactly 4 of the preceding 6 callbacks were called--it may be necessary (though I must admit I don't understand why we would only expect 4 of those 6 callbacks).

[samreid]

Mind if I replace all assert.expect( 0 ) with assert.ok( true, 'so we have at least 1 test in this set' );? I wasn’t sure if you are working on it or not.

[samreid]

@pixelzoom gave me the go-ahead on slack

[samreid]

I replaced expect( 0 ) with the synthetic test.

[samreid]

@pixelzoom can I create a scenery issue for #31 (comment) and close this issue?

[samreid]

I created phetsims/scenery#720, closing.