Deprecate QUnit.load

[leobalter]

Tip

Migration guide has been published at: https://api.qunitjs.com/QUnit/load/

---

This is not even documented, but it's used everywhere.

We need to make it an unnecessary call and then warn if it's called.

Subtasks

• Understand the practical use cases for this undocumented method.
• Create a documentation page for QUnit.load and add examples of how to accomplish these use cases instead.
• In QUnit 2.x, deprecate the method with a warning and link to these examples.

[trentmwillis]

I agree with this in general, because I get tripped up on load and start all the time, but it does open up one question: what happens with QUnit.config.autostart? Will that be revamped to just call QUnit.start?

[leobalter]

We need to discuss this, but I believe calling start() on every run is not a bad idea compared to the magic of autostarting in a browser. The responsibility to catch the document.ready or the window.load events are not for a test runner.

[trentmwillis]

FWIW, the reporter is actually what currently handles the autostart on load behavior. We could just update this to call QUnit.start instead. I think that makes for a pretty clear path forward.

[leobalter]

I thought about this, but this might break suites using config.autostart and user defined start

[leobalter]

I guess we have this fixed. Checking.

[trentmwillis]

I've begun working on this. I'd like to deprecate it so that we can remove it in 3.0.

However, the interplay between QUnit.config.autostart, QUnit.load(), and QUnit.start() is difficult to unwind. There's a much simpler end-state, but getting there in a way that we don't break existing setups is going to be tricky.

[smcclure15]

I took a pass at untangling the start/load logic:

master...smcclure15:untangle-load-logic

Most of that endeavor was for my own understanding, getting my head around what the valid sequences were among these APIs. I hope by sharing that it can benefit others as well.

It's a dense change in the end, but I tried to get each of the 3 commits fairly clean:

1, it flattens some of the if/else logic in start. Fewer nested checks.
2, I got rid of some redundant state given prior validations up to that point.
3, I created a state machine to modularize responsibility. Maybe the "beauty is in the eye of the beholder" for this design pattern, but I prefer short, encapsulated implementations over chains of if/else's and re-entry checks

I believe everything is still intact - all tests pass and I bashed it quite it bit. I do worry about edge cases in this, and wouldn't PR it until I hit it really hard.

I think what this cleaner (I hope!) logic is telling me is that our recommendations for what to do when QUnit.load is deprecated is for users to use QUnit.start exactly once OR set config.autostart = true. I think that could be it.
The load mechanism is very forgiving, as it's kind of a backdoor to "start if you haven't started yet", which is boarded up nicely with QUnit.start already.

This is all independent of #1502, which is just a matter of issuing a warning and making the reporter use a non-warning version. Once the QUnit.load frontdoor is gone, then it's just implementation detail for us to refactor (with would-be incompatibilities).

Open to your thoughts and discussion

[Krinkle]

@smcclure15 Thanks for that analysis. I agree with the objective and conclusion, one either uses the default autostart, or call start exactly once.

My worry is that abiding by that may be non-trivial for end-users to figure out within their current systems. I'd like us to cover a good chunk of what QUnit.load is used for today and document here (and later in the QUnit 3 migration guide) in an easy-to-digest form what we prescribe going forward for some of those higher-level stories.

It's been a while I looked at this, so maybe its obvious and mostly covered here already. Just wanted to clarify that goal.

[smcclure15]

The following analysis starts with a trivial passing test:

QUnit.test( "example", assert => {
    assert.true( true );
} );

and adds various combinations of calls to QUnit.load, QUnit.start, and QUnit.config.autostart to study behavior.

Node environment with QUnit CLI

Synchronous run execution

Adding QUnit.config.autostart = false; does not impact the run at all; it still passes:

QUnit.config.autostart = false;
QUnit.test( "example", assert => {
    assert.true( true );
} );

Adding one (or more!) QUnit.load calls (before or after!) doesn't change anything either (still passes):

QUnit.config.autostart = false;
QUnit.load();
QUnit.load();
QUnit.load();
QUnit.test( "example", assert => {
    assert.true( true );
} );
QUnit.load();
QUnit.load();
QUnit.load();

But using QUnit.load with_out_ the autostart = false will error:

// QUnit.config.autostart = false;
QUnit.load();
QUnit.test( "example", assert => {
    assert.true( true );
} );

% qunit demo.js
TAP version 13
ok 1 example
not ok 2 global failure
  ---
  message: "Called start() while test already started running"

Similarly, using any QUnit.start calls will give this same error:

QUnit.test( "example", assert => {
    assert.true( true );
} );
QUnit.start();

% qunit demo.js
TAP version 13
ok 1 example
not ok 2 global failure
  ---
  message: "Called start() while test already started running"

TAKEAWAY: Don't use autostart = false, QUnit.load, or QUnit.start. They were either ignored or already erroring.

Asynchronous run execution

This actually fails with both QUnit.load and QUnit.start:

QUnit.config.autostart = false;
setTimeout(() => {
    QUnit.test( "example", assert => {
        assert.true( true );
    });
    QUnit.start();
}, 100)

% qunit demo.js
TAP version 13
not ok 1 global failure
  ---
  message: "No tests were run."
  severity: failed
  actual  : {}
  expected: undefined
  stack: Error: No tests were run.

and has a trailing exception from within the framework 100ms later:

/<SNIP>/dist/qunit.js:2312
          if (module.hooks[handler].length) {
                                    ^

TypeError: Cannot read property 'length' of undefined

This has no impact on deprecating QUnit.load, and this might be a separable issue to fix (re: autostart and start).

Writing your own CLI with js-reporters

Users can leverage the js-reporters project and build their own CLI runner, via something like the following:

const JsReporters = require("js-reporters");
const runner = JsReporters.autoRegister();
JsReporters.TapReporter.init(runner);

// load tests
require(test);

// kick off test execution
QUnit.load();

If the test file that is required is the "trivial" test case, replacing QUnit.load with QUnit.start is a straightforward (and forward-compatible) fix.

If the test that is required uses QUnit.start or QUnit.load within its content, the "outer" QUnit.load in the above runner wouldn't be impacted, since subsequent QUnit.load calls don't do any harm.
When converting the "outer" call to QUnit.start, this makes the system more strict, and errors if the test uses either QUnit.start or QUnit.load within it.
The fix is to remove any QUnit.load or QUnit.start calls within the test content, and leave that run-execution management to the "runner" layer to make just one call to QUnit.start.

Browser environment

Synchronous run execution

Using one or more QUnit.load calls doesn't change any behavior, and can be removed:

QUnit.load();
QUnit.load();
QUnit.load();
QUnit.test( "example", assert => {
    assert.true( true );
});
QUnit.load();
QUnit.load();
QUnit.load();

Asynchronous run execution

Unlike the Node example that failed with no tests run, the following successfully passes:

QUnit.config.autostart = false;
setTimeout(() => {
    QUnit.test( "example", assert => {
        assert.true( true );
    });
    QUnit.start();
}, 100)

Here, the QUnit.start call is (and has been) required to begin running.