How to deal with assert.expect( 0 )? about aqua HOT 19 CLOSED

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

from aqua.

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

from aqua.

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?

from aqua.

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.

from aqua.

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.

from aqua.

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.

from aqua.

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

from aqua.

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

from aqua.

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?

from aqua.

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"?

from aqua.

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

from aqua.

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….

from aqua.

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

from aqua.

@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?

from aqua.

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).

from aqua.

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.

from aqua.

@pixelzoom gave me the go-ahead on slack

from aqua.

I replaced expect( 0 ) with the synthetic test.

from aqua.

I created phetsims/scenery#720, closing.

from aqua.