Why does backburner call Ember.onError instead of dispatchError?
#14864
Comments
workmanw
pushed a commit
to workmanw/ember.js
that referenced
this issue
Feb 1, 2017
…stead of `onError`. This is so that back burner errors can be caught my the `Test.adapter`. Fixes emberjs#14864.
workmanw
pushed a commit
to workmanw/ember.js
that referenced
this issue
Feb 1, 2017
…stead of `onError`. This is so that backburner errors can be caught my the `Test.adapter`. Fixes emberjs#14864.
workmanw
pushed a commit
to workmanw/ember.js
that referenced
this issue
Feb 1, 2017
…stead of `onError`. This is so that backburner errors can be caught by the `Test.adapter`. Fixes emberjs#14864.
workmanw
pushed a commit
to workmanw/ember.js
that referenced
this issue
Feb 1, 2017
…stead of `onError`. This is so that backburner errors can be caught by the `Test.adapter`. Fixes emberjs#14864.
workmanw
pushed a commit
to workmanw/ember.js
that referenced
this issue
Feb 1, 2017
…stead of `onError`. This is so that backburner errors can be caught by the `Test.adapter`. Fixes emberjs#14864.
workmanw
pushed a commit
to workmanw/ember.js
that referenced
this issue
Feb 1, 2017
…stead of `onError`. This is so that backburner errors can be caught by the `Test.adapter`. Fixes emberjs#14864.
rwjblue
added a commit
to rwjblue/ember.js
that referenced
this issue
Nov 27, 2017
…rror` instead of `onError`. This is so that backburner errors can be caught by the `Test.adapter`. Fixes emberjs#14864." This reverts commit 196442d.
rwjblue
added a commit
to rwjblue/ember.js
that referenced
this issue
Nov 27, 2017
…se `dispatchError` instead of `onError`. This is so that backburner errors can be caught by the `Test.adapter`. Fixes emberjs#14864." This reverts commit 196442d 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 <button {{action 'throwsAnError'}}>Click me!</button> ``` 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.
rwjblue
added a commit
to rwjblue/ember.js
that referenced
this issue
Nov 28, 2017
…se `dispatchError` instead of `onError`. This is so that backburner errors can be caught by the `Test.adapter`. Fixes emberjs#14864." This reverts commit 196442d 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 <button {{action 'throwsAnError'}}>Click me!</button> ``` 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.
rwjblue
added a commit
that referenced
this issue
Nov 29, 2017
…se `dispatchError` instead of `onError`. This is so that backburner errors can be caught by the `Test.adapter`. Fixes #14864." This reverts commit 196442d 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 <button {{action 'throwsAnError'}}>Click me!</button> ``` 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. (cherry picked from commit 8360825)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Recently we discovered a bug in our application that our tests should have caught. The error was actually a crash inside of the ember router that occurred when an action was sent to the router while it was transition from the
application_loadingroute to another router. The details of this are not important as it was our mistake (I could probably provide a fiddle if desired).Despite this error, the tests continued on and actually passed. This left me with a bad feeling because our acceptance test use their own QUnitAdapter to ensure than errors cause a test failure. So I decided to dig in to see what was going on.
Here is a normal stack from an exception during the test:
Here is the stack that failed silently:
The difference can be found by contrasting RSVP's error handling to in Backburner's error handling.
You can see that RSVP's error handling logic will call dispatchError which will defer to a
TestAdapterif set to do so. Unfortunately Backburner just callsEmber.onErrordirectly.I don't really possess a whole lot of knowledge about ember-metal, but just looking at this I can't see any obvious reason why Backburner's error handling couldn't call
dispatchErrorinstead of directly callingEmber.onError.Any thoughts? I'd sleep much better knowing that these error were being routed to my test suite.
The text was updated successfully, but these errors were encountered: