-
Notifications
You must be signed in to change notification settings - Fork 780
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
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.resolve(). Our reference implementation in lib/ shows that it indeed try-catches both the scheduling of then() callback, and the contents within. So the way I'm fixing this is: * Leave the global hooks code completely unchanged. * Make test.run() and test.queueHook() more like queueGlobalHook, by moving the resolvePromise outside the try-catch. * Fix resolvePromise() to use Promise.resolve() which try-catches just the user-provided "promise.then", nothing else is wrapped in try-catch.
- Loading branch information
Showing
3 changed files
with
194 additions
and
68 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| QUnit.test('throw early', async function (_assert) { | ||
| throw new Error('boo'); | ||
| }); | ||
|
|
||
| QUnit.test('throw late', async function (assert) { | ||
| await Promise.resolve(''); | ||
| assert.true(true); | ||
| throw new Error('boo'); | ||
| }); | ||
|
|
||
| // NOTE: This is not about testing a rejected Promise or throwing async function. | ||
| // In those cases, `ret.then(, cb)` will not throw, but inform you via cb(err). | ||
| // Instead, this is testing a bad Thenable implementation, where then() itself | ||
| // throws an error. This is not possible with native Promise, but is possible with | ||
| // custom Promise-compatible libraries and | ||
| QUnit.test('test with bad thenable', function (assert) { | ||
| assert.true(true); | ||
| return { | ||
| then: function () { | ||
| throw new Error('boo'); | ||
| } | ||
| }; | ||
| }); | ||
|
|
||
| QUnit.hooks.beforeEach(function (assert) { | ||
| if (assert.test.testName !== 'example') { return; } | ||
| return { | ||
| then: function () { | ||
| throw new Error('global brocoli'); | ||
| } | ||
| }; | ||
| }); | ||
| QUnit.hooks.afterEach(function (assert) { | ||
| if (assert.test.testName !== 'example') { return; } | ||
| return { | ||
| then: function () { | ||
| throw new Error('global artichoke'); | ||
| } | ||
| }; | ||
| }); | ||
|
|
||
| QUnit.module('hooks with bad thenable', function (hooks) { | ||
| hooks.beforeEach(function () { | ||
| return { | ||
| then: function () { | ||
| throw new Error('banana'); | ||
| } | ||
| }; | ||
| }); | ||
| hooks.afterEach(function () { | ||
| return { | ||
| then: function () { | ||
| throw new Error('apple'); | ||
| } | ||
| }; | ||
| }); | ||
|
|
||
| QUnit.test('example', function (assert) { | ||
| assert.true(true); | ||
| }); | ||
| }); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| # command: ["qunit", "async-test-throw.js"] | ||
|
|
||
| TAP version 13 | ||
| not ok 1 throw early | ||
| --- | ||
| message: "Promise rejected during \"throw early\": boo" | ||
| severity: failed | ||
| actual : null | ||
| expected: undefined | ||
| stack: | | ||
| Error: boo | ||
| at /qunit/test/cli/fixtures/async-test-throw.js:2:9 | ||
| ... | ||
| not ok 2 throw late | ||
| --- | ||
| message: "Promise rejected during \"throw late\": boo" | ||
| severity: failed | ||
| actual : null | ||
| expected: undefined | ||
| stack: | | ||
| Error: boo | ||
| at /qunit/test/cli/fixtures/async-test-throw.js:8:9 | ||
| ... | ||
| not ok 3 test with bad thenable | ||
| --- | ||
| message: "Promise rejected during \"test with bad thenable\": boo" | ||
| severity: failed | ||
| actual : null | ||
| expected: undefined | ||
| stack: | | ||
| Error: boo | ||
| at /qunit/test/cli/fixtures/async-test-throw.js:20:13 | ||
| ... | ||
| not ok 4 hooks with bad thenable > example | ||
| --- | ||
| message: "Promise rejected before \"example\": global brocoli" | ||
| severity: failed | ||
| actual : null | ||
| expected: undefined | ||
| stack: | | ||
| Error: global brocoli | ||
| at /qunit/test/cli/fixtures/async-test-throw.js:29:13 | ||
| ... | ||
| --- | ||
| message: "Promise rejected before \"example\": banana" | ||
| severity: failed | ||
| actual : null | ||
| expected: undefined | ||
| stack: | | ||
| Error: banana | ||
| at /qunit/test/cli/fixtures/async-test-throw.js:46:15 | ||
| ... | ||
| --- | ||
| message: "Promise rejected after \"example\": apple" | ||
| severity: failed | ||
| actual : null | ||
| expected: undefined | ||
| stack: | | ||
| Error: apple | ||
| at /qunit/test/cli/fixtures/async-test-throw.js:53:15 | ||
| ... | ||
| --- | ||
| message: "Promise rejected after \"example\": global artichoke" | ||
| severity: failed | ||
| actual : null | ||
| expected: undefined | ||
| stack: | | ||
| Error: global artichoke | ||
| at /qunit/test/cli/fixtures/async-test-throw.js:37:13 | ||
| ... | ||
| 1..4 | ||
| # pass 0 | ||
| # skip 0 | ||
| # todo 0 | ||
| # fail 4 | ||
|
|
||
| # exit code: 1 |