Deprecate assert.expect?

[JamesMGreene]

@Krinkle expressed an opinion that, with the assert.async() addition, we no longer need assert.expect() whatsoever. At first I was a bit skeptical of this statement as I have grown into the habit of always using assert.expect() but I later realized he was correct.

For example, take this "synchronous callback" example from the Cookbook page:

QUnit.test( "a test", function( assert ) {
  assert.expect( 2 );   // `assert.` added

  function calc( x, operation ) {
    return operation( x );
  }

  var result = calc( 2, function( x ) {
    assert.ok( true, "calc() calls operation function" );
    return x * x;
  });

  assert.equal( result, 4, "2 square equals 4" );
});

While this setup can be effectively replaced with the assert.async() approach as well, I would argue that that is semantically invalid as the operation is intended to be synchronous (@Krinkle disagrees, FYI).

However, there are still at least 2 other ways to easily test this without using assert.expect, as @Krinkle demonstrated in a gist:

Simple call counter:

QUnit.test( "a test", function( assert ) {
  var calls = 0;

  function calc( x, operation ) {
    return operation( x );
  }

  var result = calc( 2, function( x ) {
    calls++;
    return x * x;
  });

  assert.equal( result, 4, "2 square equals 4" );
  assert.equal( calls, 1, "calc() calls operation function" );
});

Sinon spies:

QUnit.test( "a test", function( assert ) {
  var multiply = sinon.spy(function( x ) {
    return x * x;
  });

  function calc( x, operation ) {
    return operation( x );
  }

  var result = calc( 2, multiply );

  assert.equal( result, 4, "2 square equals 4" );
  assert.equal( multiply.callCount, 1, "calc() calls operation function" );
});

@jzaefferer @leobalter @gibson042 @scottgonzalez et al: Thoughts? Do you see any other use cases were assert.expect is important to keep around? Does it provide enough convenience over the two alternative approaches above that we should keep it around anyway?

[scottgonzalez]

Tracking call counts manually (even through a spy) seems really annoying if you actually care about this beyond the setup of a spy/mock object.

I'm not sure how async tests avoid the need to define the expectation though. Consider the following:

assert.expect( 2 );
var done = assert.async();
$( "<div>" ).dialog({
    create: function() {
        assert.ok( true, "Create was invoked" );
    },
    open: function() {
        assert.ok( true, "Open was invoked" );
        done();
    }
});

Without the expectation, how do you know if create was called? You can certainly track it manually, but I'm not sure how making this type of testing more verbose improves anything.

As for synchronous tests, here's another example:

assert.expect( 4 );
var headers = element.find( ".foo-headers" );
headers.each(function() {
    assert.equal( this.whatever, ... );
});

The above implicitly expects 4 headers to be found. The verification is done through the expectation. This can be rewritten as:

var headers = element.find( ".foo-headers" );
assert.equal( headers.length, 4, "Correct number of headers found" );
headers.each(...);

[JamesMGreene]

I would probably rewrite the first example using JamesMGreene/qunit-assert-step:

var done = assert.async();
$( "<div>" ).dialog({
    create: function() {
        assert.step( 1, "Create was invoked" );
    },
    open: function() {
        assert.step( 2, "Open was invoked" );
        done();
    }
});

For the latter example, I think its second implementation is far superior because it is both semantically correct and explicit about the details of its assertions.

The implicit form has bitten me many times before, particularly if I have an existing looping assertion + an expect call that implicitly checks the array length. Then I make changes to the code, add 1 assertion to the test outside of the loop but forget to update the expect count. Surprise! The test still passes because I unintentionally changed the length of the array via my code changes. Too bad I probably won't notice the bug for a while because of my overly presumptuous use of expect.

[gibson042]

I agree with @JamesMGreene about preferring the explicit assertions, whether through qunit-assert-step or spies or even simple manual counters.
It's also worth noting that the too-simple expect has nasty interactions with beforeEach and afterEach, and would probably make more sense as expectMore anyway.

In the end, though, expect seems mostly to support a false sense of confidence. While I do like the convenience it can offer on occasion, I think it's caused about as much harm as good.

[gibson042]

Another thought: exposing assert.test.assertions.length would make implementing an ad-hoc and correctly-scoped meta-assertion much less verbose (i.e., only one line more than expect):

var done = assert.async(),
    initialCount = assert.count();
$( "<div>" ).dialog({
    create: function() {
        assert.ok( true, "Create was invoked" );
    },
    open: function() {
        assert.ok( true, "Open was invoked" );
        assert.equal( assert.count() - initialCount, 2, "Correct assertion count" );
        done();
    }
});

[JamesMGreene]

I agree that something like assert.count() would be more useful as it allows you to inspect point-in-time metadata rather than end-of-test metadata.

[JamesMGreene]

@jzaefferer @Krinkle @leobalter: Weigh in?

[Krinkle]

@scottgonzalez

assert.expect( 2 );
var done = assert.async();
$( "<div>" ).dialog({
    create: function() {
        assert.ok( true, "Create was invoked" );
    },
    open: function() {
        assert.ok( true, "Open was invoked" );
        done();
    }
});

Without the expectation, how do you know if create was called?

I know this pattern is common, but in my opinion this is a poor quality assertion. Aside from following asynchronous flow, there should never be assertions outside the main body of the test function. Execute source code, then make assertions; not nested like above.

There's quite a few different approaches one could take that are more robust and correct from a my point of view.

test('dialog - bool', function (assert) {
  var created, opened;
  $('<div>').dialog({
    create: function () {
      created = true;
    },
    open: function () {
      opened = true;
    }
  });
  assert.ok(created1, 'create');
  assert.ok(opened, 'create');
});

The plain boolean assertion works, but is not very strict (doesn't care about execution order or whether callbacks are intentionally or accidentally invoked multiple times).

test('dialog - count', function (assert) {
  var created = 0, opened = 0;
  $('<div>').dialog({
    create: function () {
      created++;
    },
    open: function () {
      opened++;
    }
  });
  assert.equal(created, 1, 'create');
  assert.equal(opened, 1, 'create');
});

The counter approach is more strict (guards against unexpected executions), but is a bit verbose and tedious to maintain and setup.

test('dialog - flow', function (assert) {
  var flow = [];
  $('<div>').dialog({
    create: function () {
      flow.push('create');
    },
    open: function () {
      flow.push('open');
    }
  });
  assert.deepEqual(
    flow,
    ['create', 'open'],
    'callback flow'
  );
});

The flow approach is my personal favourite and recommendation. It doesn't feel like a workaround and has minimal overhead and boilerplate. It strictly asserts the execution order and will notice any unexpected executions. It scales easily when you add more complexity.

[leobalter]

There're now many points on this discussion

• assert.expect: I agree it should be deprecated now that we offer more control on async tests with assert.async. Like @JamesMGreene, I was skeptical at first but it makes sense.
• assert.step is good but doesn't look it's meant for unit tests, maybe functional. I prefer to have it as a plugin.
• asser.count: we can make this through scoped variables.
• I agree partially with @Krinkle on @scottgonzalez's example, but I don't agree with it on async situations. Supposing that the $('<div>').dialog method is async, it would be ok to have the assertions on it's callback. Better with promise like methods.

[leobalter]

about the async example, I've remembering of this on our current tests and it should properly on the .dialog exemple with async functionality.

var done1 = assert.async();
var done2 = assert.async();
setTimeout(function() {
  assert.ok( true, "test resumed from async operation 1" );
  done1();
}, 500 );
setTimeout(function() {
  assert.ok( true, "test resumed from async operation 2" );
  done2();
}, 150);

[jzaefferer]

@Krinkle the "flow array" approach looks good for synchronous tests, but Scott's example was about asynchronous tests. How would you implement that?

[JamesMGreene]

You'd do it with 2 assert.async() trackers like in the example @leobalter copied from the v1.16.0 Cookbook/API docs in his previous comment.

[jzaefferer]

That example doesn't check the order of execution. In Scott's example, if open runs before create, only one assertion runs during the execution of the test, correctly failing the test.

[JamesMGreene]

QUnit assert.step plugin + 2 assert.async trackers, then.

[jzaefferer]

Removing an existing method in favour of adding a plugin for a common use case is not an improvement.

[JamesMGreene]

Also, if we want to get technical, Scott's example does not guarantee verification of the order of execution. It only verifies that one of the following occurs:

1. create is called before open (execution order verified as intended, yay!)
2. create and open are called in the same [synchronous] event loop iteration but open could be called before create and the test would still pass

[JamesMGreene]

We can leave assert.expect intact if it is just too popular to remove but the way that most consumers use it is definitely an anti-pattern, IMHO.

[scottgonzalez]

create and open are called in the same [synchronous] event loop iteration but open could be called before create and the test would still pass

Sounds like a bug in QUnit if that's true. Once the done callback is invoked, no assertions should be allowed to occur.

[JamesMGreene]

Sounds like a bug in QUnit if that's true. Once the [final] done callback is invoked, no assertions should be allowed to occur.

That's never been a requirement to date, AFAIK. assert.async was just based on replacing the QUnit.start/QUnit.stop functionality in a more test-centric fashion but those operations always allowed for additional operations within the same event loop iteration.

The behavior you describe is what I would expect from something like a theoretical assert.done() (a la NodeUnit) but not necessarily from a callback like var done = assert.async(); /* ... */ done();

If we want to make it a requirement, that's fine [and now is the time to do it!], but we'll have to document that behavior very explicitly as it will definitely a potential gotcha for existing QUnit users.

[jzaefferer]

Making Timo's example async:

test('dialog - flow', function (assert) {
  var flow = [],
    done = assert.async();
  $('<div>').dialog({
    create: function () {
      flow.push('create');
    },
    open: function () {
      flow.push('open');
      assert.deepEqual(
        flow,
        ['create', 'open'],
        'callback flow'
      );
      done();
    }
  });
});

This can be extended to any number of callbacks, though I'm not sure how valuable that kind of test really would be.

Sounds like a bug in QUnit if that's true. Once the done callback is invoked, no assertions should be allowed to occur.

Indeed. I've filed that as a separate ticket, #668

[jzaefferer]

We discussed this at the IRC meeting today. We're closing this, keeping assert.expect. We'll improve the documentation though, via jquery/api.qunitjs.com#87 #1354