qunitjs / qunit / 9847498493

90%

Build Type

push

github

Committed by Krinkle

Commit Message

Core: Fix crash when "bad thenable" is returned from global module hook

== Background

* We run QUnit.test functions via runTest() in src/test.js.
  This includes both test.callback.call() and resolvePromise(),
  and thus both are covered by the try-catch in test.run().

* We run (non-global) module hooks via callHook() in src/test.js,
  which similarly does both, and both are covered by try-catch
  in test.queueHook().

* We previously ran global module hooks with the resolvePromise()
  not covered by try-catch. This means a bad thenable returned from
  a global hook can cause a prettt confusing crash.
  The ProcessingQueue gets interrupted, a global error is reported
  (in the console only, not in the UI), the current test then
  eventually times out, and the ProcessingQueue is unable to
  continue because it is at non-zero config.depth, yet there are no
  more top-level tests in the queue, which means advance() crashes
  with advanceTestQueue() getting undefined from an empty array shift()
  and trying to call it as a function.

== Fix

Fix global module hooks to also effectively do a try-catch. However,
I'm doing this in a bit of a different way to improve code quality.

There is an old TODO in the code from me that planned to actually
move the resolvePromise() outside the try-catch for consistency with
tests and global hooks. This is a bad idea, as it would spread this
bug to module hooks. I had that idea because I didn't realize that:

1. test.run() has a higher-level try-catch so it't actually global
   hooks that is inconsistent, not module hooks.
2. There is nothing else saving us from a ProcessingQueue crash.

Yet, I still believe it is a good idea to move this outside the try-catch
because we should not tolerate errors from our internal resolvePromise
function. It is only the user-provided `promise.then` call inside
thqt function that we want to try-catch. A better way to achieve
this is to normalize the user-provided promise via Promise.r... (continued)

Run Details

16 of 16 new or added lines in 1 file covered. (100.0%)

1 existing line in 1 file now uncovered.

1822 of 2047 relevant lines covered (89.01%)

2354.31 hits per line