Core: Fix late onerror handling

[Krinkle]

Background

Previously, QUnit.onError and QUnit.onUnhandledRejection could report global errors by synthesizing a new test, even after a run has ended.

This is problematic when an errors ocurrs after all modules (and their hooks) have finished, and the overall test run has ended.

The most immediate problem is that hooks having finished already, means it is illegal for a new test to start since "after" has already run. To protect against such illegal calls, the hooks object is emptied internally, and this new test causes an internal error:

TypeError: Cannot read property 'length' of undefined

This is not underlying problem though, but rather our internal safeguard working as intended. The higher-level problem is that there is no appropiate way to report a late error as a test since the run has already ended. The QUnit.done() callbacks have run, and the runEnd event has been emitted.

Approach

Instead of trying to report (late) errors as a test, only print them to console.warn(), which goes to stderr in Node.js. For the CLI, also remember that uncaught errors were found and use that to make sure we don't change exitCode back to zero (e.g. in case we have an uncaught error after the last test but before our runEnd callback is called).

Changes

• Generalise QUnit.onUnhandledRejection and re-use it for window.onerror (browser), and uncaught exceptions (CLI).

• Fix broken use of QUnit.onError in process.on( "uncaughtException" ).
  This was passing the wrong parameters. Use the new onUncaughtException method instead.

• Clarify that QUnit.onError is only for window.onerror. For now, keep its strange non-standard signature as-is (with the custom object parameter), but document this and its return value.

• Remove the unused "..args" from QUnit.onError. This was only ever passed from one of our unit tests to give one extra argument (a string of "actual"), which then ended up passed as "actual" parameter to pushFailure(). We never used this in the actual onError binding, so remove this odd variadic construct for now.

• Change ProcessingQueue#done, which is in charge of reporting the "No tests were run" error, to no longer rely on the way that QUnit.onError previously queued a late test.

  The first part of this function may run twice (same as before, once after an empty test run, and one more time after the synthetic test has finished and the queue is empty again). Change this so that we no longer assign finished = true in that first part. This means we will still support queueing of this one late test. But, since the quueue is empty, we do need to call advance() manually as otherwise it'd never get processed.

  Previously, finished = true was assigned first, which meant that QUnit.onError was adding a test under that condition. But this worked anyway because Test#queue internally had manual advancing exactly for this use case, which is also where we now emit a deprecation warning (to become an error in QUnit 3). Note that using this for anything other than the "No tests run" error was already unreliable since generally runEnd would have been emitted already. The "No tests run" test was exactly done from the one sweet spot where it was (and remains) safe because that threw an error and thus prevented runEnd from being emitted.

Fixes #1377.
Ref #1322.
Ref #1446.

[Krinkle]

@smcclure15 I've split this in two commits for easier review. I ran the first one in a separate branch against CI first to make sure it passes on its own, however, GitHub won't do or show that here unfortunately.

[smcclure15]

This is awesome! Thanks for tackling this task at the higher level it was needing. Code and messaging feels much cleaner now in this state.

Left a few questions/nits, but otherwise looks great

[smcclure15]

FWIW I've found

assert.true(x.includes(y), x)

to be helpful (to add the "actual" as a diagnostic to inspect on failure)

[Krinkle]

Ack. I saw that pattern in a few places. I also saw the pushResult({ result: str.includes(), actual: str }) pattern in a few other places in this file which I think renders in a slightly less confusing way. We could pick one of those - or define a local assert.includes util to start with - and use that consistently for all similar needs in this file. (Also happy to take a patch for that after this lands 🙂 ).

[Krinkle]

(ref)

[steveszc]

@Krinkle Just came across this change as it is breaking some functionality in https://github.com/steveszc/ember-cli-memory-leak-detector

My use-case is that I need to inject a test after all other tests have finished running (once the tests finish I capture a heap snapshot, search it for memory leaks created during the test run, and create a new test that either passes or fails depending on whether memory leaks were found).

I previously relied on the (apparently unintentional) ability to create a test after the test suite has finished. I understand that I have a rather unconventional use-case but it would great if I can continue reporting memory leaks as a failed test within Qunit as it provides a great DX.

Is there a way to do this moving forward?
Is there another venue/forum where I can bring this up for discussion? Happy to create a new issue if there is no easy work around.

[Krinkle]

@steveszc Thanks. I've pulled your comment into a new issue at #1663.

In short: Yes, I believe there should be (and is) a way to do this moving forward. If those aren't a good fit for some reason, then I'll proritise getting the needed accomodation in-place for the next minor release. I'll elaborate at #1663.