How should global error handlers resume the test runner in 2.0?

[platinumazure]

We're using QUnit and RequireJS. We've defined a require.onError handler as follows:

define([], function () {
    require.onError = function (e) {
        if (QUnit && QUnit.config) {
            if (QUnit.config.current) {
                var currentTest = QUnit.config.current;

                currentTest.pushFailure("Died on test #" + (currentTest.assertions.length + 1) + " " +
                    currentTest.stack + ": " + (e.message || e));
            }

            // Restart the tests if they're blocking
            if (QUnit.config.blocking) {
                QUnit.start();
            }
        }
    };
});

This is heavily inspired by how the global error handler used to handle these issues. However, QUnit.start() itself now throws an error (in 2.0.0-rc1), so I need a new way to resume the test runner. What is the best way to do that?

[platinumazure]

For what it's worth, I would be okay with a new API, maybe something like Test#pushFailureAndResume(), which couples the semaphore decrement with the error being pushed. That way people can only use it to add a global failure, rather than skirting the assert.async() mechanism.

[leobalter]

that's not async control, that's an error control we didn't have QUnit 1.x designed for it. It just worked in a way allowing the leakage from inside and outside a test context.

On the previous version, QUnit.start could be used to control any context of any test from anywhere, even from another test.

This resume method is a feature we never had properly designed on QUnit and we need to understand what is cause the blocking state to better handle it. I guess the promise-able tests might help finding a good path, but I would like to leave this issue open to discuss the best approach before shipping anything we can't remove until another major version.

[platinumazure]

Fine by me.

For what it's worth, promise-able tests won't work in the RequireJS case because RequireJS does not return a promise, and its callback functions are executed asynchronously (here meaning not on the same call stack). My require.onError function is similarly unable to support the injection of a local test context in order to somehow return a promise.

Please let me know if I can provide further information to aid in this discussion.

[platinumazure]

Oh, you said you wanted to know what causes the blocking state. That's an easy one:

QUnit.test("This is a test", function (assert) {
    var done = assert.async();     // Causes config.blocking to be set to true, as it should

    require(["some/dependency"], function (dependency) {
        // This callback is invoked asynchronously. Do stuff as normal.

        assert.ok(dependency);

        done();
    });
});

If the above snippet threw (within the require callback), it will not be caught by either the synchronous try/catch or the promise logic.

This is very similar to my other issue, #816. However, I'm currently able to deal with that problem using the require.onError code from my first post; with QUnit 2.0.0-rc1 out, this issue is becoming more urgent for me, because I have no way of restarting the test runner on error.

[leobalter]

Thanks for providing more info.

On this specific example, I would say the best approach is to not rely on anything from the tests that is outside it's context, if that becomes repetitive you can abstract the require calls to include an error callback, as in:

require(["some/dependency"], function (dependency) {
    // This callback is invoked asynchronously. Do stuff as normal.

    assert.ok(dependency);

    done();
},
function(err) {
    ...
    done();
});

[platinumazure]

Hmm... We have one of these on every test in our codebase (thousands of tests). That could be painful. Are we sure there isn't a DRYer way to do this?

[leobalter]

this abstraction could be something like:

function t_require(done, deps, ...callbacks) {
  return require(deps, callback[0], function(...args) {
    var res;
    if (callback[1]) {
      res = callback[1].apply(this, args);
    }
    done();
    return res;
  });
}

that's not the best looking thing, but I it should work while we don't have anything else to cover this on QUnit.

[platinumazure]

Oh, okay, I got it. You are giving me suggestions for what I can do while we're figuring out the design. Thanks! Sorry for misunderstanding.

Just to be absolutely sure: Are you interested (not immediately, perhaps, but in the future) in the notion of having QUnit support these global handlers in some way? Or does this strike you as not meriting a place on the roadmap?

[leobalter]

Are you interested (not immediately, perhaps, but in the future) in the notion of having QUnit support these global handlers in some way?

I am! I'm just afraid I don't have the proper bandwidth to do it as soon as I would like.

I believe this deprecation cleanup on 2.0 opens the path for a better async handling and that's totally welcoming for the project.

This issue is great to account for on any work we do on this improvement.

[platinumazure]

Awesome. If it will help to have some proof-of-concept pull requests as APIs are proposed, I am happy to help.

[leobalter]

That would be great!

[leobalter]

I guess the #947 might help solving this as well.

cc @trentmwillis

[gibson042]

@platinumazure: Given that your current strategy already dives into QUnit internals, you can create something analogous by spying on assert.async. Note that this is also subject to breakage at any time, but it should be an easy swap for now: https://jsfiddle.net/8kgpzwut/ .

[platinumazure]

@gibson042 My apologies, I thought I had thanked you earlier. Thanks for taking the time to put together a fiddle proving the concept- really helps.

[platinumazure]

@trentmwillis Thanks for merging #1020, that looks promising.

I'm wondering if it might be worth exposing two versions of recover-- one to only get rid of one hold, if possible, and the other to get rid of all holds and forcibly restart the test? (As I say this, though, I'm wondering how the hell to know which hold to restart.)

Example use case:

QUnit.test("Two asyncs, one of which throws", function (assert) {
    var done1 = assert.async(), done2 = assert.async();

    setTimeout(function () {
        throw new Error();
        // done1();
    });

    setTimeout(function () {
        assert.ok(true);
        done2();
    });
});

window.onError = function someGlobalExceptionHandler() {
    // remove just one hold from the test somehow? is this even possible?
};

Alternatively, I'm wondering if recover() should purge all assertions made so far and add an assertion failure saying the global error handler was invoked and everything is fubar.

Apologies if I'm going over ground that has already been covered elsewhere (and please do feel free to link me to any existing discussion so I can get up to speed).

[trentmwillis]

@platinumazure yes, that is the issue I was pondering as well, and why I hadn't moved forward with a suggestion. Essentially we want QUnit.recover() to restart in the case of a failure, but we don't want it be abused and used to restart things that were still just waiting for some async task.

[platinumazure]

@trentmwillis Thanks for the prompt reply. Yes, I agree that we can't afford to let it be abused.

One of the APIs I was going to suggest originally was QUnit.pushFailureAndRestart( errorMessage ), which would kill all async holds but also require an error message (which is then logged as an assertion failure), so that it becomes impossible to just get out of jail free. Thoughts?

[gibson042]

@platinumazure I agree. As a purely internal operation, our diligence ensures that every recovery is preceded by a pushFailure, but as a public operation we should move that part into the function.

[platinumazure]

My status on this:

• I'm willing to propose some APIs any time, but I'm waiting for ES6 and possible consumption of ESLint first. So this will take a backseat for a little bit, unless someone else on the core team believes this should take higher priority.

[platinumazure]

Now that ESLint is in, I'm hoping to get back to this in the next week or two.

Any thoughts on an API name/contract? I had proposed QUnit.pushFailureAndRestart( errorMessage ), but we also have QUnit.recover(). If we have a preference for one of those (or a completely different API name/contract), I can incorporate that into a PR.

[JamesMGreene]

Shouldn't we perhaps just make it so that the global error handler resumes execution if an unhandled error occurs while a test is in progress?

Also, doesn't your test timeout after ~5 seconds and move on to the next test anyway?

[platinumazure]

Okay, it took me a while to come back to this but I think @JamesMGreene has the right idea here.

I'm working on #1099 to make the global error handling logic more directly accessible for other runtimes or plugins. That plus an ability to resume the test runner from the global error handler (possibly behind a config option) should hopefully work for these use cases.

[leobalter]

This is not only fair and reasonable, it's good to have this feature. Thanks for leading this work, @platinumazure.

[trentmwillis]

@platinumazure can this be closed now that #1099 has landed? I'm assuming yes, but wanted to double check

[platinumazure]

Hi @trentmwillis, maybe not yet:

I'm working on #1099 to make the global error handling logic more directly accessible for other runtimes or plugins. That plus an ability to resume the test runner from the global error handler (possibly behind a config option) should hopefully work for these use cases.

Emphasis mine. I'm happy to close this issue and create a new one for clarity, if needed, but the problem described by this issue is not fully solved.

Okay if I close this myself later today?

[trentmwillis]

Okay, no worries. We can leave this one open for continuity.

[Krinkle]

From what I can tell, problems with async errors are now localised to individual tests and naturally resume both execution of other tests in the same module as well as other modules. Confirmed on latest stable (2.11) for both CLI and HTML:

I'm closing this for now as such, but I recognise that @platinumazure did ask for an explicit recovery method. If that's still needed, please file a new task for that. I'm hoping to avoid it as it smells like a workaround, and I'd prefer to make it "just" work by default. But, if plugins needs this capability to catch-resume in a way that core should not have awareness of, then I am still open to reconsidering that, by all means!

[platinumazure]

Thanks @Krinkle. I don't have a need for a recovery method at this point-- if that changes, I'll file a new issue.