Docs: Recommend async functions over assert.async()

[lucaswerkmeister]

In the simple example from the documentation, the timeout problem doesn’t actually occur (if fetchDouble() synchronously throws an Error, QUnit reports a test failure despite the assert.async() still being outstanding), but in more complicated tests it can still happen. We’ve recently migrated some tests in MediaWiki core and extensions to async+await syntax – see 1, 2, 3.

[linux-foundation-easycla]

• ✅login: lucaswerkmeister / name: Lucas Werkmeister / (f5e706b)

The committers listed above are authorized under a signed CLA.

[Krinkle]

Thanks @lucaswerkmeister, this is long over-due to recommend in the docs!

The advice is good. But I think we need a better examples to demonstrate why. I want to avoid making a bold claim that seems easy to contradict for a well-meaning person with contrary experience.

In the linked examples, most do not trade timeouts for fast failures. In the common case of Promise-like or Node.js-style result callbacks, this doesn't happen easily. When there is an easy translation from assert.async() to an async function, then the assert.async() code was likely already safe from timeouts:

function fetchDouble(input, callback) {
  if (Math.random() < 0.5) {
    callback(null, input * 2);
  } else {
    callback('Error');
}

QUnit.test('async example', function (assert) {
  const done = assert.async();
  fetchDouble(21, function (err, result) {
    assert.strictEqual(err, null);
    assert.strictEqual(result, 42);
    done();
  });
});

Use of assert.async() would create a timeout if fetchDouble has a bug does not invoke the callback at all. Likewise, if you're implementing your own Promise-returning function, one might also not invoke resolve or reject. Same problem. When people write simple Promise chains or native async-await, such mistake is unlikely.

Another example of assert.async() going wrong, is in an event system or other callback system where the callback is not strictly for conveying a single future result/error. Your third example (mediawiki/Wikibase) has these event callbacks. While it is a serious bug for an async function not to settle, or a result-callback to never be called (you'd expect at least a rejection or error parameter), it is entirely normal for an event emitter not to emit a certain event if the thing in question did not happen. Plus, there's a bunch of other problems with placing assertions inside application-controlled callback as documented under assert.verifySteps().

The other thing I found in those MediaWiki examples, is the risk of false positives:

QUnit.test('async example', function (assert) {
  const done = assert.async();

  fetch('/api/status')
    .then(function (resp) {
      return resp.json();
    }).
    .then(function (data) {
      assert.true(data.isSuccess);  // Bad example: May not be reached!
    })
    .finally(done);
});

This is yet another example of assertions in application-controlled code, which can simply not execute at all, and allow the test to pass. This problem is avoided if the assertion is placed outside the Promise-chain, or if the chain is returned the QUnit.test, or rewritten with async-await such that the assertions are in the test function (this is the same under the hood as a Promise chain returned to QUnit.test because an async function returns a Promise).

QUnit.test('async example', function (assert) {
  return fetch('/api/status')
    .then(function (resp) {
      return resp.json();
    }).
    .then(function (data) {
      assert.true(data.isSuccess);  // OK. If the chain fails earlier, QUnit knows.
    });
});

QUnit.test('async example', async function (assert) {
  const statusData = null;

  await fetch('/api/status')
    .then(function (resp) {
      return resp.json();
    }).
    .then(function (data) {
      statusData = data;
    });

  assert.true(statusData.isSuccess);  // OK. Assertion is always reached.
});

QUnit.test('async example', async function (assert) {
  const resp = await fetch('/api/status');
  const data = await resp.json();
  assert.true(data.isSuccess);  // OK. If the chain fails earlier, QUnit knows.
});

Perhaps one of these risks is worth calling out. Can you think of a concise example where we'd trade timeouts for fast failures?

[lucaswerkmeister]

The other thing I found in those MediaWiki examples, is the risk of false positives:

QUnit.test('async example', function (assert) {
  const done = assert.async();

  fetch('/api/status')
    .then(function (resp) {
      return resp.json();
    }).
    .then(function (data) {
      assert.true(data.isSuccess);  // Bad example: May not be reached!
    })
    .finally(done);
});

I don’t think this is actually a false positive? If the promise fails, then it’s an unhandled promise rejection (.finally() doesn’t turn rejected promises into resolved ones), and QUnit will fail the test upon encountering that. (I actually had the same thought at first, and then I thought “I guess in theory QUnit could guard against that with an unhandled rejection handler”, and then I tried it out and discovered that it does exactly that already :D)

You could probably still have a false positive if you’re using jQuery Deferreds instead of native Promises – they’re fairly compatible, but I don’t think they run the same unhandled rejection handlers.

I’ll think a bit more about it later :)