Allow assert.async callback to "wrap" actual async callback and handle errors?

[platinumazure]

It would be really excellent if there was an easy way to have inadvertent exceptions from async callbacks be noted in an assertion and the test runner restarted, much like what is already done in the synchronous and promise pathways. (As it is, the test runner chokes completely in these cases because QUnit.start(), or equivalent, is not called.)

• Synchronous pathway: https://github.com/jquery/qunit/blob/1.18.0/src/test.js#L106-L120
• Promise pathway: https://github.com/jquery/qunit/blob/1.18.0/src/test.js#L326-L341

What I would like to see is something similar for when an asynchronous callback throws an error and the async callback doesn't have anything QUnit on its call stack.

I think this could be implemented with a generic callback-wrapping function that basically has a try/catch in it and handles errors in a similar way as the two sections of code I've outlined above. I'll write up a pull request in the next few days to outline what I'm getting at, but here's a very rough API:

// In Assert.prototype
async: function () {
    // do the usual setup of async
    var done = function () {
        // do the usual cleanup of async
    };
    done.wrapError = function (asyncCallback) {
        return function wrapError () {
            try {
                asyncCallback.apply(null, arguments);
            } catch (e) {
                // Get the current test object using QUnit.config.current or similar
                // Call QUnit.pushFailure() if we have a test object
                done();     // Equivalent to QUnit.start()
            }
        };
    };
    return done;
}

Usage:

QUnit.test("Wrapping an error callback", function (assert) {
    var done = assert.async();
    setTimeout(done.wrapError(function () {
        throw new Error();
        // Previously would halt the test runner, but now would allow tests to continue running
    }), 50);
});

One could easily envision a similar API which would just invoke the done() callback on success or error, allowing consumers to avoid invoking done() in the callback itself. (I don't think there's a huge loss of readability either, since done is still mentioned in the function wrap invocation. It's just a matter of choosing an intelligent function name to complement or replace wrapError.)

Any thoughts?

[JamesMGreene]

Thanks for the input, @platinumazure! (And on a personal note: Hi, Kevin! Hope all is well. 👋)

My thoughts:
Synchronous flows and async Promises both have 1-2 guaranteed and consistent resolution/exit points, which makes testing them pretty easy. The unfortunate burden of asynchronous testing [without Promises] is that there is NOT a limited number of exit points. The scenario you demonstrated for the theoretical API change works wonderfully is your async call stack is only 1 level deep but falls flat if there is any async chaining, continuation passing, etc. involved.

For example:

var syncCallbackInvokedAsynchronously = function() {
  // This will be caught. YAY!  =D
  throw new Error();
};

QUnit.test("Working wrapping an error callback - USEFUL!", function (assert) {
  var done = assert.async();
  setTimeout(
    done.wrapError(syncCallbackInvokedAsynchronously),
    50
  );
});


var asyncCallbackInvokedAsynchronously = function() {
  setTimeout(
    function() {
      // This will not be caught... BOO-HISS!  =(
      throw new Error();
    },
    50
  );
};

QUnit.test("Non-working wrapping an error callback - USELESS!", function (assert) {
  var done = assert.async();
  setTimeout(
    done.wrapError(asyncCallbackInvokedAsynchronously),
    50
  );
});

The only way I know of to track that type of flow is to duck-punch all of the core asynchronous functions (setTimeout/clearTimeout, setInterval/clearInterval, setImmediate/clearImmediate, requestAnimationFrame/cancelAnimationFrame, addEventListener/removeEventListener, XMLHttpRequest, etc.) so that you can implement long-tail/async stack tracing (a la Q, {Track:js}, Chrome dev tools, etc.).

To date, QUnit has not yet gone down the path of duck-punching any core Web Platform functions in such a manner, so we will definitely need to discuss further.

[platinumazure]

I definitely agree that QUnit should not go down the path of stubbing out
setTimeout and relatives. That's not what I'm suggesting and that's not a
use case I'm trying to support here.

My thoughts are as follows:

1. It is relatively common for test cases to involve only one level of
   asynchronous calling, just in the test function. My proposal solves this
   common case very well.

2. As for multiple levels of async calls, 98+% of the time those can be
   broken down into two types:

   a. The extra async is introduced in a test. In this case, the solution
   to that is the same as the solution to multiple asyncs that are not nested:
   Use multiple assert.async calls in the test, and ensure each one is
   called in due time (including possibly by the proposed new API). The
   non-nested case is documented in the documentation for assert.async on
   the website.

   b. The extra async is introduced in the code under test. In this case,
   the answer remains the same as before this proposal: Use a mock/stub
   library like e.g., sinon.js to handle that, and understand that that is
   beyond the scope of QUnit.

It's probably worth clarifying that the only issue I'm really trying to
solve is, when part of a test (as in, code in function scope of a test) is
asynchronous and QUnit simply can't track it due to the call stack not
including anything in QUnit.test (etc.), resulting in an unexpected test
runner halt. That violates the principle of least surprise to me and I
think it's worth looking into. As for any async problems that come from
code under test being asynchronous, like I said, that should be handled via
stubbing/mocking techniques or by refactoring the code to make it more
testable.

Am I not understanding your point fully? If I'm missing something, please
let me know.
On Apr 29, 2015 9:33 PM, "James M. Greene" notifications@github.com wrote:

Thanks for the input, @platinumazure https://github.com/platinumazure!
(And on a personal note: Hi, Kevin! Hope all is well. [image: 👋])

My thoughts:
Synchronous flows and async Promises both have 1-2 guaranteed and
consistent resolution/exit points, which makes testing them pretty easy.
The unfortunate burden of asynchronous testing [without Promises] is that
there is NOT a limited number of exit points. The scenario you
demonstrated for the theoretical API change works wonderfully is your async
call stack is only 1 level deep but falls flat if there is any async
chaining, continuation passing, etc. involved.

The only way I know of to track that type of flow is to duck-punch all of
the core asynchronous functions (setTimeout/clearTimeout, setInterval/
clearInterval, setImmediate/clearImmediate, requestAnimationFrame/
cancelAnimationFrame, addEventListener/removeEventListener, XMLHttpRequest,
etc.) so that you can implement long-tail/async stack tracing (a la Q
https://github.com/kriskowal/q, {Track:js}
https://docs.trackjs.com/JavaScript_Api_Reference/trackJs.configure/callback/callback.bindStack,
Chrome dev tools
http://www.html5rocks.com/en/tutorials/developertools/async-call-stack/,
etc.).

To date, QUnit has not yet gone down the path of duck-punching any core
Web Platform functions in such a manner, so we will definitely need to
discuss further.

—
Reply to this email directly or view it on GitHub
#816 (comment).

[platinumazure]

And now I'm on the website, your code sample renders... My basic points are unchanged: the second case is converted either by multiple assert.async() calls (if the nested async is defined in the test) or by a mock/stub library otherwise.

I just want to make the common stuff, within a test, easy (I.e., setTimeout or require embedded in a test itself). I don't want to turn QUnit into a stub library.

[scottgonzalez]

I'm -1 on this.

[platinumazure]

@scottgonzalez Would love to hear why.

[scottgonzalez]

It's awkward and potentially misleading.

[platinumazure]

@scottgonzalez Well, I can understand that the proposed API needs some work. Are you in favor of the concept, at least (that is, shoring up an inconsistency in exception handling between synchronous, promise, and asynchronous test code)?

[scottgonzalez]

I'm not in favor of the concept.

[jzaefferer]

It's probably worth clarifying that the only issue I'm really trying to solve is, when part of a test (as in, code in function scope of a test) is asynchronous and QUnit simply can't track it due to the call stack not including anything in QUnit.test (etc.), resulting in an unexpected test runner halt. That violates the principle of least surprise to me and I think it's worth looking into. As for any async problems that come from code under test being asynchronous, like I said, that should be handled via stubbing/mocking techniques or by refactoring the code to make it more testable.

That's something we should address, but not with the suggested solution.

[platinumazure]

That's something we should address, but not with the suggested solution.

I've no doubt there is a better approach out there somewhere. If you have an idea how it should be done, let me know what you're thinking and I'm happy to write up a pull request to get things started. Thanks!

[jzaefferer]

For a start, could you do a PR that modifies one of the existing test page and makes the runner halt? Preferably with a scenario that is likely to occur in practice.

[platinumazure]

Sure. Should the target branch be anything besides master?

[leobalter]

master is fine.

[jzaefferer]

@platinumazure I guess this is also something that fell out of sight? Would be great if you could still put together the failure case we discussed above.

[platinumazure]

Yeah, I've been trying to think of one besides my RequireJS case with no
luck. Still noodling on this one.
On Oct 16, 2015 3:40 PM, "Jörn Zaefferer" notifications@github.com wrote:

@platinumazure https://github.com/platinumazure I guess this is also
something that fell out of sight? Would be great if you could still put
together the failure case we discussed above.

—
Reply to this email directly or view it on GitHub
#816 (comment).

[jzaefferer]

Okay, thanks. Let us know either way.

[leobalter]

@platinumazure what's the current status on this one? We did some changes on our RequireJS since last year and that might be slightly better to handle now.

[platinumazure]

@leobalter No meaningful updates at this time. This has become less of an issue for us since we were able to use require.in error to trap most errors. I still think this could be useful in general but I haven't had the time to fulfill your (reasonable) requirements around proving that this is a real issue.

If you like, you can close this and I can reopen when I have something to add to the discussion. Sound good?

[platinumazure]

Closing, as global error handlers can now invoke QUnit.onError and (when #1108 lands) resume the test runner.