From 8360825e8415e03836ba0c0f907a533ed5f8a666 Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Wed, 22 Nov 2017 11:03:21 -0500 Subject: [PATCH 1/4] [BUGFIX lts] Revert "[BUGFIX] Changed backburner's error handler to use `dispatchError` instead of `onError`. This is so that backburner errors can be caught by the `Test.adapter`. Fixes #14864." This reverts commit 196442dc17b2511495b68087b1c5e817c70d62da which essentially made all error handling for things that are run-wrapped async, dramatically impacting development ergonomics. The originally reported issue is a _very real_ problem that we need to guard against. To reproduce that issue, the following conditions must exist: * The application must implement `Ember.onerror` in a way that does not rethrow errors. * Throw an error during anything that uses `run.join`. The example scenario had a template like this: ```hbs ``` To fix this error swallowing behavior, the commit being reverted made all errors hit within the run loop use `dispatchError`, which (during tests) has the default implementation of invoking `QUnit`'s `assert.ok(false)`. Unfortunately, this meant that it is now impossible to use a standard `try` / `catch` to catch errors thrown within anything "run-wrapped". For example, these patterns were no longer possible after the commit in question: ```js try { Ember.run(() => { throw new Error('This error should be catchable'); }); } catch(e) { // this will never be hit during tests... } ``` This ultimately breaks a large number of test suites that rely (rightfully so!) on being able to do things like: ```js module('service:foo-bar', function(hooks) { setupTest(hooks); hooks.beforeEach(() => { this.owner.register('service:whatever', Ember.Service.extend({ someMethod(argumentHere) { Ember.assert('Some random argument validation here', !argumentHere); } }); }); test('throws when argumentHere is missing', function(assert) { let subject = this.owner.lookup('service:foo-bar'); assert.throws(() => { run(() => subject.someMethod()); }, /random argument validation/); }); }); ``` The ergonomics of breaking standard JS `try` / `catch` is too much, and therefore the original commit is being reverted. --- packages/ember-metal/lib/error_handler.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ember-metal/lib/error_handler.js b/packages/ember-metal/lib/error_handler.js index 2ebdb29bae9..72ce2c47412 100644 --- a/packages/ember-metal/lib/error_handler.js +++ b/packages/ember-metal/lib/error_handler.js @@ -16,7 +16,7 @@ let getStack = error => { let onerror; export const onErrorTarget = { get onerror() { - return dispatchOverride || onerror; + return onerror; } }; From 1d37ba6b761de84c7765f3fc213225ae1de44cb7 Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Mon, 27 Nov 2017 10:09:12 -0500 Subject: [PATCH 2/4] [BUGFIX lts] Add tests for various permutations of error handler setup. Known permutations: * Testing * With `Ember.onerror` * Sync Backburner (`run` and `run.join`) * Async Backburner (`run.later`, `run.next`) * RSVP * Without `Ember.onerror` * Sync Backburner (`run` and `run.join`) * Async Backburner (`run.later`, `run.next`) * RSVP * Development / Production * With `Ember.onerror` * Sync Backburner (`run` and `run.join`) * Async Backburner (`run.later`, `run.next`) * RSVP * Without `Ember.onerror` * Sync Backburner (`run` and `run.join`) * Async Backburner (`run.later`, `run.next`) * RSVP This commit adds tests for all scenarios. --- packages/ember-testing/tests/adapters_test.js | 19 +- packages/ember/tests/error_handler_test.js | 431 +++++++++++++++++- 2 files changed, 421 insertions(+), 29 deletions(-) diff --git a/packages/ember-testing/tests/adapters_test.js b/packages/ember-testing/tests/adapters_test.js index 8647ee33bdd..69fcc29a721 100644 --- a/packages/ember-testing/tests/adapters_test.js +++ b/packages/ember-testing/tests/adapters_test.js @@ -4,12 +4,13 @@ import Adapter from '../adapters/adapter'; import QUnitAdapter from '../adapters/qunit'; import { Application as EmberApplication } from 'ember-application'; -var App, originalAdapter, originalQUnit; +var App, originalAdapter, originalQUnit, originalWindowOnerror; QUnit.module('ember-testing Adapters', { setup() { originalAdapter = Test.adapter; originalQUnit = window.QUnit; + originalWindowOnerror = window.onerror; }, teardown() { if (App) { @@ -20,6 +21,7 @@ QUnit.module('ember-testing Adapters', { Test.adapter = originalAdapter; window.QUnit = originalQUnit; + window.onerror = originalWindowOnerror; } }); @@ -71,17 +73,22 @@ QUnit.test('Adapter is used by default (if QUnit is not available)', function() ok(!(Test.adapter instanceof QUnitAdapter)); }); -QUnit.test('With Ember.Test.adapter set, errors in Ember.run are caught', function () { +QUnit.test('With Ember.Test.adapter set, errors in synchronous Ember.run are bubbled out', function (assert) { let thrown = new Error('Boom!'); - let caught; + let caughtInAdapter, caughtInCatch; Test.adapter = QUnitAdapter.create({ exception(error) { - caught = error; + caughtInAdapter = error; } }); - run(() => { throw thrown; }); + try { + run(() => { throw thrown; }); + } catch(e) { + caughtInCatch = e; + } - deepEqual(caught, thrown); + assert.equal(caughtInAdapter, undefined, 'test adapter should never receive synchronous errors'); + assert.equal(caughtInCatch, thrown, 'a "normal" try/catch should catch errors in sync run'); }); diff --git a/packages/ember/tests/error_handler_test.js b/packages/ember/tests/error_handler_test.js index e71a0961a6c..0501a3a1c12 100644 --- a/packages/ember/tests/error_handler_test.js +++ b/packages/ember/tests/error_handler_test.js @@ -6,88 +6,473 @@ const ONERROR = Ember.onerror; const ADAPTER = Ember.Test && Ember.Test.adapter; const TESTING = Ember.testing; +let WINDOW_ONERROR; + QUnit.module('error_handler', { + setup() { + // capturing this outside of module scope to ensure we grab + // the test frameworks own window.onerror to reset it + WINDOW_ONERROR = window.onerror; + }, + teardown() { Ember.onerror = ONERROR; Ember.testing = TESTING; + window.onerror = WINDOW_ONERROR; if (Ember.Test) { Ember.Test.adapter = ADAPTER; } } }); -function runThatThrows(message) { +function runThatThrowsSync(message = 'Error for testing error handling') { return run(() => { throw new Error(message); }); } -test('by default there is no onerror', function(assert) { - Ember.onerror = undefined; - assert.throws(runThatThrows, Error); - assert.equal(Ember.onerror, undefined); +function runThatThrowsAsync(message = 'Error for testing error handling') { + return run.next(() => { + throw new Error(message); + }); +} + +test('by default there is no onerror - sync run', function(assert) { + assert.strictEqual(Ember.onerror, undefined, 'precond - there should be no Ember.onerror set by default'); + assert.throws(runThatThrowsSync, Error, 'errors thrown sync are catchable'); }); -test('when Ember.onerror is registered', function(assert) { +test('when Ember.onerror (which rethrows) is registered - sync run', function(assert) { assert.expect(2); Ember.onerror = function(error) { assert.ok(true, 'onerror called'); throw error; }; - assert.throws(runThatThrows, Error); - // Ember.onerror = ONERROR; + assert.throws(runThatThrowsSync, Error, 'error is thrown'); +}); + +test('when Ember.onerror (which does not rethrow) is registered - sync run', function(assert) { + assert.expect(2); + Ember.onerror = function(error) { + assert.ok(true, 'onerror called'); + }; + runThatThrowsSync(); + assert.ok(true, 'no error was thrown, Ember.onerror can intercept errors'); }); if (DEBUG) { - test('when Ember.Test.adapter is registered', function(assert) { - assert.expect(2); + test('when Ember.Test.adapter is registered and error is thrown - sync run', function(assert) { + assert.expect(1); Ember.Test.adapter = { exception(error) { - assert.ok(true, 'adapter called'); - throw error; + assert.notOk(true, 'adapter is not called for errors thrown in sync run loops'); } }; - assert.throws(runThatThrows, Error); + assert.throws(runThatThrowsSync, Error); }); - test('when both Ember.onerror Ember.Test.adapter are registered', function(assert) { + test('when both Ember.onerror (which rethrows) and Ember.Test.adapter are registered - sync run', function(assert) { assert.expect(2); - // takes precedence Ember.Test.adapter = { exception(error) { - assert.ok(true, 'adapter called'); - throw error; + assert.notOk(true, 'adapter is not called for errors thrown in sync run loops'); } }; Ember.onerror = function(error) { - assert.ok(false, 'onerror was NEVER called'); + assert.ok(true, 'onerror is called for sync errors even if Ember.Test.adapter is setup'); throw error; }; - assert.throws(runThatThrows, Error); + assert.throws(runThatThrowsSync, Error, 'error is thrown'); + }); + + test('when both Ember.onerror (which does not rethrow) and Ember.Test.adapter are registered - sync run', function(assert) { + assert.expect(2); + + Ember.Test.adapter = { + exception(error) { + assert.notOk(true, 'adapter is not called for errors thrown in sync run loops'); + } + }; + + Ember.onerror = function(error) { + assert.ok(true, 'onerror is called for sync errors even if Ember.Test.adapter is setup'); + }; + + runThatThrowsSync(); + assert.ok(true, 'no error was thrown, Ember.onerror can intercept errors'); + }); + + QUnit.test('when Ember.Test.adapter is registered and error is thrown - async run', function(assert) { + assert.expect(3); + let done = assert.async(); + + let caughtInAdapter, caughtInCatch, caughtByWindowOnerror; + Ember.Test.adapter = { + exception(error) { + caughtInAdapter = error; + } + }; + + window.onerror = function(message) { + caughtByWindowOnerror = message; + // prevent "bubbling" and therefore failing the test + return true; + }; + + try { + runThatThrowsAsync(); + } catch(e) { + caughtInCatch = e; + } + + setTimeout(() => { + assert.equal(caughtInAdapter, undefined, 'test adapter should never catch errors in run loops'); + assert.equal(caughtInCatch, undefined, 'a "normal" try/catch should never catch errors in an async run'); + + assert.pushResult({ + result: /Error for testing error handling/.test(caughtByWindowOnerror), + actual: caughtByWindowOnerror, + expected: 'to include `Error for testing error handling`', + message: 'error should bubble out to window.onerror, and therefore fail tests (due to QUnit implementing window.onerror)' + }); + + done(); + }, 20); + }); + + test('when both Ember.onerror and Ember.Test.adapter are registered - async run', function(assert) { + assert.expect(1); + let done = assert.async(); + + Ember.Test.adapter = { + exception(error) { + assert.notOk(true, 'Adapter.exception is not called for errors thrown in run.next'); + } + }; + + Ember.onerror = function(error) { + assert.ok(true, 'onerror is invoked for errors thrown in run.next/run.later'); + }; + + runThatThrowsAsync(); + setTimeout(done, 10); }); } -QUnit.test('Ember.run does not swallow exceptions by default (Ember.testing = true)', function() { +QUnit.test('does not swallow exceptions by default (Ember.testing = true, no Ember.onerror) - sync run', function(assert) { Ember.testing = true; + + let error = new Error('the error'); + assert.throws(() => { + Ember.run(() => { + throw error; + }); + }, error); +}); + +QUnit.test('does not swallow exceptions by default (Ember.testing = false, no Ember.onerror) - sync run', function(assert) { + Ember.testing = false; let error = new Error('the error'); - throws(() => { + assert.throws(() => { Ember.run(() => { throw error; }); }, error); }); -QUnit.test('Ember.run does not swallow exceptions by default (Ember.testing = false)', function() { +QUnit.test('does not swallow exceptions (Ember.testing = false, Ember.onerror which rethrows) - sync run', function(assert) { + assert.expect(2); Ember.testing = false; + + Ember.onerror = function(error) { + assert.ok(true, 'Ember.onerror was called'); + throw error; + }; + let error = new Error('the error'); - throws(() => { + assert.throws(() => { Ember.run(() => { throw error; }); }, error); }); + +QUnit.test('Ember.onerror can intercept errors (aka swallow) by not rethrowing (Ember.testing = false) - sync run', function(assert) { + assert.expect(1); + Ember.testing = false; + + Ember.onerror = function(error) { + assert.ok(true, 'Ember.onerror was called'); + }; + + let error = new Error('the error'); + try { + Ember.run(() => { + throw error; + }); + } catch(e) { + assert.notOk(true, 'Ember.onerror that does not rethrow is intentionally swallowing errors, try / catch wrapping does not see error'); + } +}); + + +QUnit.test('does not swallow exceptions by default (Ember.testing = true, no Ember.onerror) - async run', function(assert) { + let done = assert.async(); + let caughtByWindowOnerror; + + Ember.testing = true; + + window.onerror = function(message) { + caughtByWindowOnerror = message; + // prevent "bubbling" and therefore failing the test + return true; + }; + + Ember.run.later(() => { + throw new Error('the error'); + }, 10); + + setTimeout(() => { + assert.pushResult({ + result: /the error/.test(caughtByWindowOnerror), + actual: caughtByWindowOnerror, + expected: 'to include `the error`', + message: 'error should bubble out to window.onerror, and therefore fail tests (due to QUnit implementing window.onerror)' + }); + + done(); + }, 20); +}); + +QUnit.test('does not swallow exceptions by default (Ember.testing = false, no Ember.onerror) - async run', function(assert) { + let done = assert.async(); + let caughtByWindowOnerror; + + Ember.testing = false; + + window.onerror = function(message) { + caughtByWindowOnerror = message; + // prevent "bubbling" and therefore failing the test + return true; + }; + + Ember.run.later(() => { + throw new Error('the error'); + }, 10); + + setTimeout(() => { + assert.pushResult({ + result: /the error/.test(caughtByWindowOnerror), + actual: caughtByWindowOnerror, + expected: 'to include `the error`', + message: 'error should bubble out to window.onerror, and therefore fail tests (due to QUnit implementing window.onerror)' + }); + + done(); + }, 20); +}); + +QUnit.test('Ember.onerror can intercept errors (aka swallow) by not rethrowing (Ember.testing = false) - async run', function(assert) { + let done = assert.async(); + + Ember.testing = false; + + window.onerror = function(message) { + assert.notOk(true, 'window.onerror is never invoked when Ember.onerror intentionally swallows errors'); + // prevent "bubbling" and therefore failing the test + return true; + }; + + let thrown = new Error('the error'); + Ember.onerror = function(error) { + assert.strictEqual(error, thrown, 'Ember.onerror is called with the error'); + }; + + Ember.run.later(() => { + throw thrown; + }, 10); + + setTimeout(done, 20); +}); + +function generateRSVPErrorHandlingTests(message, generatePromise, timeout = 10) { + test(`${message} when Ember.onerror which does not rethrow is present - rsvp`, function(assert) { + assert.expect(1); + + let thrown = new Error('the error'); + Ember.onerror = function(error) { + assert.strictEqual(error, thrown, 'Ember.onerror is called for errors thrown in RSVP promises'); + }; + + generatePromise(thrown); + + // RSVP.Promise's are configured to settle within the run loop, this + // ensures that run loop has completed + return new Ember.RSVP.Promise((resolve) => setTimeout(resolve, timeout)); + }); + + test(`${message} when Ember.onerror which does rethrow is present - rsvp`, function(assert) { + assert.expect(2); + + let thrown = new Error('the error'); + Ember.onerror = function(error) { + assert.strictEqual(error, thrown, 'Ember.onerror is called for errors thrown in RSVP promises'); + throw error; + }; + + window.onerror = function(message) { + assert.pushResult({ + result: /the error/.test(message), + actual: message, + expected: 'to include `the error`', + message: 'error should bubble out to window.onerror, and therefore fail tests (due to QUnit implementing window.onerror)' + }); + + // prevent "bubbling" and therefore failing the test + return true; + }; + + generatePromise(thrown); + + // RSVP.Promise's are configured to settle within the run loop, this + // ensures that run loop has completed + return new Ember.RSVP.Promise((resolve) => setTimeout(resolve, timeout)); + }); + + test(`${message} when Ember.onerror which does not rethrow is present (Ember.testing = false) - rsvp`, function(assert) { + assert.expect(1); + + Ember.testing = false; + let thrown = new Error('the error'); + Ember.onerror = function(error) { + assert.strictEqual(error, thrown, 'Ember.onerror is called for errors thrown in RSVP promises'); + }; + + generatePromise(thrown); + + // RSVP.Promise's are configured to settle within the run loop, this + // ensures that run loop has completed + return new Ember.RSVP.Promise((resolve) => setTimeout(resolve, timeout)); + }); + + test(`${message} when Ember.onerror which does rethrow is present (Ember.testing = false) - rsvp`, function(assert) { + // this should be: `assert.expect(2);`, but currently non-testing + // Ember.onerror is called twice for errors in RSVP promises: + // + // 1) the default RSVP error handler calls `dispatchError` (which then calls + // `Ember.onerror`) + // 2) the run wrapping that is done by the RSVP + Backburner + // integration calls `dispatchError` (which then calls `Ember.onerror`) + assert.expect(3); + + Ember.testing = false; + let thrown = new Error('the error'); + Ember.onerror = function(error) { + assert.strictEqual(error, thrown, 'Ember.onerror is called for errors thrown in RSVP promises'); + throw error; + }; + + window.onerror = function(message) { + assert.pushResult({ + result: /the error/.test(message), + actual: message, + expected: 'to include `the error`', + message: 'error should bubble out to window.onerror, and therefore fail tests (due to QUnit implementing window.onerror)' + }); + + // prevent "bubbling" and therefore failing the test + return true; + }; + + generatePromise(thrown); + + // RSVP.Promise's are configured to settle within the run loop, this + // ensures that run loop has completed + return new Ember.RSVP.Promise((resolve) => setTimeout(resolve, timeout)); + }); + + if (DEBUG) { + test(`${message} when Ember.Test.adapter is present - rsvp`, function(assert) { + assert.expect(1); + + let thrown = new Error('the error'); + Ember.Test.adapter = Ember.Test.QUnitAdapter.create({ + exception(error) { + assert.strictEqual(error, thrown, 'Adapter.exception is called for errors thrown in RSVP promises'); + } + }); + + generatePromise(thrown); + + // RSVP.Promise's are configured to settle within the run loop, this + // ensures that run loop has completed + return new Ember.RSVP.Promise((resolve) => setTimeout(resolve, timeout)); + }); + + test(`${message} when both Ember.onerror and Ember.Test.adapter are present - rsvp`, function(assert) { + assert.expect(1); + + let thrown = new Error('the error'); + Ember.Test.adapter = Ember.Test.QUnitAdapter.create({ + exception(error) { + assert.strictEqual(error, thrown, 'Adapter.exception is called for errors thrown in RSVP promises'); + } + }); + + Ember.onerror = function(error) { + assert.notOk(true, 'Ember.onerror is not called if Test.adapter does not rethrow'); + }; + + generatePromise(thrown); + + // RSVP.Promise's are configured to settle within the run loop, this + // ensures that run loop has completed + return new Ember.RSVP.Promise((resolve) => setTimeout(resolve, timeout)); + }); + + test(`${message} when both Ember.onerror and Ember.Test.adapter are present - rsvp`, function(assert) { + assert.expect(2); + + let thrown = new Error('the error'); + Ember.Test.adapter = Ember.Test.QUnitAdapter.create({ + exception(error) { + assert.strictEqual(error, thrown, 'Adapter.exception is called for errors thrown in RSVP promises'); + throw error; + } + }); + + Ember.onerror = function(error) { + assert.strictEqual(error, thrown, 'Ember.onerror is called for errors thrown in RSVP promises if Test.adapter rethrows'); + }; + + generatePromise(thrown); + + // RSVP.Promise's are configured to settle within the run loop, this + // ensures that run loop has completed + return new Ember.RSVP.Promise((resolve) => setTimeout(resolve, timeout)); + }); + } +} + +generateRSVPErrorHandlingTests('errors in promise constructor', (error) => { + new Ember.RSVP.Promise(() => { + throw error; + }); +}); + +generateRSVPErrorHandlingTests('errors in promise .then callback', (error) => { + Ember.RSVP.resolve().then(() => { + throw error; + }); +}); + +generateRSVPErrorHandlingTests('errors in async promise .then callback', (error) => { + new Ember.RSVP.Promise((resolve) => setTimeout(resolve, 10)).then(() => { + throw error; + }); +}, 20); From 80f7d6b46b434777216f247967596548cd06abae Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Mon, 27 Nov 2017 14:19:14 -0500 Subject: [PATCH 3/4] [BUGFIX lts] Allow Ember.Test.adapter to opt out of `exception` handling. In the majority of testing frameworks (e.g. Mocha, QUnit, Jasmine) a global error handler is attached to `window.onerror`. When an uncaught exception is thrown, this global handler is invoked and the test suite will properly fail. Requiring that the provided test adapter provide an exception method is strictly worse in nearly all cases than allowing the error to be bubbled upwards and caught/handled by either `Ember.onerror` (if present) or `window.onerror` (generally from the test framework). This commit makes `Ember.Test.adapter.exception` optional. When not present, errors during testing will be handled by existing "normal" means. --- packages/ember-testing/lib/test/adapter.js | 2 +- packages/ember/tests/error_handler_test.js | 51 ++++++++++++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/packages/ember-testing/lib/test/adapter.js b/packages/ember-testing/lib/test/adapter.js index 6bbe1df7a35..ca431790ea9 100644 --- a/packages/ember-testing/lib/test/adapter.js +++ b/packages/ember-testing/lib/test/adapter.js @@ -8,7 +8,7 @@ export function getAdapter() { export function setAdapter(value) { adapter = value; - if (value) { + if (value && typeof value.exception === 'function') { setDispatchOverride(adapterDispatch); } else { setDispatchOverride(null); diff --git a/packages/ember/tests/error_handler_test.js b/packages/ember/tests/error_handler_test.js index 0501a3a1c12..8a7898f1e88 100644 --- a/packages/ember/tests/error_handler_test.js +++ b/packages/ember/tests/error_handler_test.js @@ -397,6 +397,57 @@ function generateRSVPErrorHandlingTests(message, generatePromise, timeout = 10) }); if (DEBUG) { + test(`${message} when Ember.Test.adapter without \`exception\` method is present - rsvp`, function(assert) { + assert.expect(1); + + let thrown = new Error('the error'); + Ember.Test.adapter = Ember.Test.QUnitAdapter.create({ + exception: undefined + }); + + window.onerror = function(message) { + assert.pushResult({ + result: /the error/.test(message), + actual: message, + expected: 'to include `the error`', + message: 'error should bubble out to window.onerror, and therefore fail tests (due to QUnit implementing window.onerror)' + }); + + // prevent "bubbling" and therefore failing the test + return true; + }; + + generatePromise(thrown); + + // RSVP.Promise's are configured to settle within the run loop, this + // ensures that run loop has completed + return new Ember.RSVP.Promise((resolve) => setTimeout(resolve, timeout)); + }); + + test(`${message} when both Ember.onerror and Ember.Test.adapter without \`exception\` method are present - rsvp`, function(assert) { + assert.expect(1); + + let thrown = new Error('the error'); + Ember.Test.adapter = Ember.Test.QUnitAdapter.create({ + exception: undefined + }); + + Ember.onerror = function(error) { + assert.pushResult({ + result: /the error/.test(error.message), + actual: error.message, + expected: 'to include `the error`', + message: 'error should bubble out to window.onerror, and therefore fail tests (due to QUnit implementing window.onerror)' + }); + }; + + generatePromise(thrown); + + // RSVP.Promise's are configured to settle within the run loop, this + // ensures that run loop has completed + return new Ember.RSVP.Promise((resolve) => setTimeout(resolve, timeout)); + }); + test(`${message} when Ember.Test.adapter is present - rsvp`, function(assert) { assert.expect(1); From 0f3f49cd6c27ee4d4d41033f16a438c2b69d9220 Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Mon, 27 Nov 2017 14:49:39 -0500 Subject: [PATCH 4/4] [BUGFIX beta] Avoid double throwing unhandled promise rejections. When used in Ember, `RSVP` is configured to settle within Backburner's configured `actions` queue. Prior to this change, any unhandled promise rejections would: * `Ember.testing === true` * When `Ember.Test.adapter` was registered the `Test.adapter`'s `exception` method would be called, and the rejection would be logged to the console. * When `Ember.Test.adapter` was not registered, the `defaultDispatch` implementation would re-throw the error, and since RSVP settles in the run loop this means that the re-thrown error would be caught by the currently flushing Backburner queue's `invokeWithOnError` which sends any errors to `Ember.onerror` (if present). If `Ember.onerror` was not present, the exception would bubble up the "normal" unhandled exception system (and ultimately to `window.onerror`). * `Ember.testing === false` * When `Ember.onerror` is present, it would be invoked with the rejection reason. * When `Ember.onerror` is not present, the rejection reason would be logged to the console. After this change: * When `Ember.Test.adapter` is present, its `exception` method is invoked with the rejection. * Otherwise the rejection reason is rethrown. The benefits of this are: * It is now possible to debug rejected promises via "normal" JS exception debugging (e.g. break on uncaught exception). * There are many fewer decision points, making it much easier to grok what is going on. --- packages/ember-metal/lib/error_handler.js | 31 ------------------- packages/ember-metal/lib/index.js | 1 - packages/ember-runtime/lib/ext/rsvp.js | 9 ++++-- packages/ember-runtime/tests/ext/rsvp_test.js | 4 +-- packages/ember/tests/error_handler_test.js | 9 +----- 5 files changed, 9 insertions(+), 45 deletions(-) diff --git a/packages/ember-metal/lib/error_handler.js b/packages/ember-metal/lib/error_handler.js index 72ce2c47412..6921fda95f4 100644 --- a/packages/ember-metal/lib/error_handler.js +++ b/packages/ember-metal/lib/error_handler.js @@ -1,18 +1,6 @@ import Logger from 'ember-console'; import { isTesting } from 'ember-debug'; -// To maintain stacktrace consistency across browsers -let getStack = error => { - let stack = error.stack; - let message = error.message; - - if (stack && stack.indexOf(message) === -1) { - stack = `${message}\n${stack}`; - } - - return stack; -}; - let onerror; export const onErrorTarget = { get onerror() { @@ -30,14 +18,6 @@ export function setOnerror(handler) { } let dispatchOverride; -// dispatch error -export function dispatchError(error) { - if (dispatchOverride) { - dispatchOverride(error); - } else { - defaultDispatch(error); - } -} // allows testing adapter to override dispatch export function getDispatchOverride() { @@ -46,14 +26,3 @@ export function getDispatchOverride() { export function setDispatchOverride(handler) { dispatchOverride = handler; } - -function defaultDispatch(error) { - if (isTesting()) { - throw error; - } - if (onerror) { - onerror(error); - } else { - Logger.error(getStack(error)); - } -} diff --git a/packages/ember-metal/lib/index.js b/packages/ember-metal/lib/index.js index 01288ef1d06..1ed903f78e5 100644 --- a/packages/ember-metal/lib/index.js +++ b/packages/ember-metal/lib/index.js @@ -19,7 +19,6 @@ export { export { getOnerror, setOnerror, - dispatchError, setDispatchOverride, getDispatchOverride } from './error_handler'; diff --git a/packages/ember-runtime/lib/ext/rsvp.js b/packages/ember-runtime/lib/ext/rsvp.js index ca6537e616d..42cabd313e2 100644 --- a/packages/ember-runtime/lib/ext/rsvp.js +++ b/packages/ember-runtime/lib/ext/rsvp.js @@ -1,7 +1,7 @@ import * as RSVP from 'rsvp'; import { run, - dispatchError + getDispatchOverride } from 'ember-metal'; import { assert } from 'ember-debug'; @@ -21,7 +21,12 @@ RSVP.on('error', onerrorDefault); export function onerrorDefault(reason) { let error = errorFor(reason); if (error) { - dispatchError(error); + let overrideDispatch = getDispatchOverride(); + if (overrideDispatch) { + overrideDispatch(error); + } else { + throw error; + } } } diff --git a/packages/ember-runtime/tests/ext/rsvp_test.js b/packages/ember-runtime/tests/ext/rsvp_test.js index bc264418327..804b6e3da01 100644 --- a/packages/ember-runtime/tests/ext/rsvp_test.js +++ b/packages/ember-runtime/tests/ext/rsvp_test.js @@ -46,12 +46,10 @@ QUnit.test('Can reject with non-Error object', function(assert) { try { run(RSVP, 'reject', 'foo'); } catch (e) { - ok(false, 'should not throw'); + equal(e, 'foo', 'should throw with rejection message'); } finally { setTesting(wasEmberTesting); } - - ok(true); }); QUnit.test('Can reject with no arguments', function(assert) { diff --git a/packages/ember/tests/error_handler_test.js b/packages/ember/tests/error_handler_test.js index 8a7898f1e88..34caf115def 100644 --- a/packages/ember/tests/error_handler_test.js +++ b/packages/ember/tests/error_handler_test.js @@ -361,14 +361,7 @@ function generateRSVPErrorHandlingTests(message, generatePromise, timeout = 10) }); test(`${message} when Ember.onerror which does rethrow is present (Ember.testing = false) - rsvp`, function(assert) { - // this should be: `assert.expect(2);`, but currently non-testing - // Ember.onerror is called twice for errors in RSVP promises: - // - // 1) the default RSVP error handler calls `dispatchError` (which then calls - // `Ember.onerror`) - // 2) the run wrapping that is done by the RSVP + Backburner - // integration calls `dispatchError` (which then calls `Ember.onerror`) - assert.expect(3); + assert.expect(2); Ember.testing = false; let thrown = new Error('the error');