Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUGFIX lts] Refactor / fix error handling scenarios. #15871

Merged
merged 4 commits into from Nov 28, 2017

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Nov 27, 2017

This PR has a few very small changes to the error handling system, but has a large impact to developer ergonomics.

The primary changes in this PR are:

Revert changes made in #14898

#14898 essentially made all error handling for things that are run-wrapped uncatchable, 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:
<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:

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:

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 of a step backwards, so it is being reverted.

As mentioned earlier, the underlying problem reported in #14864 is very real and must be addressed also. Therefore, in order to prevent Ember.onerror from swallowing errors during tests ember-qunit (and ember-mocha) will implement checks to ensure that Ember.onerror (if present) properly rethrows errors during testing. The following is psuedo code of what that might look like:

test('Ember.onerror is functioning properly', function(assert) {
  let thrown = new Error('onerror bubbling');
  if (!Ember.onerror) {
    assert.ok(true, 'no Ember.onerror is setup');
  } else {
    assert.throws(() => {
      Ember.onerror(thrown);
    }, /onerror bubbling/, 'Ember.onerror should rethrow when Ember.testing is true');
  }
});

test('window.onerror is functioning properly', function(assert) {
  assert.expect(1);

  let thrown = new Error('onerror bubbling');
  let originalQUnitOnError = QUnit.onError;
  QUnit.onError = function() {
    assert.ok(true, 'QUnit.onError is called');
  };

  try {
    let result = window.onerror(thrown.message);
  } finally {
    QUnit.onError = originalQUnitOnError;
  }
});

Remove the requirement of Ember.Test.Adapter's to implement the exception method.

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 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).

Over time Ember.Test.adapter.exception should be deprecated in favor of relying on Ember.onerror and/or window.onerror.

Simplify unhandled promise rejection bubbling.

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.

The majority of the changes are in new tests being added, I attempted to ensure that each of the following were under test:

  • Testing
    • With Ember.Test.adapter
      • 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
    • Without Ember.Test.adapter
      • 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

Each commit in this PR attempts to "stand on its own" and explain the reasoning for the changes they contain. As such, it may be easier to review commit by commit instead of all at once.

@rwjblue
Copy link
Member Author

rwjblue commented Nov 27, 2017

/cc @workmanw for review

@@ -30,14 +30,6 @@ export function setOnerror(handler) {
}

let dispatchOverride;
// dispatch error
export function dispatchError(error) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was previously only used by RSVP.on('error', ...) (after reverting #14898), and RSVP.on('error', ...) in this PR now does the logic of dispatchError directly (which makes it much clearer that this is RSVP error handling specific).

…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.
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.
…ing.

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.
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.
@rwjblue
Copy link
Member Author

rwjblue commented Nov 28, 2017

Rebased after landing #15874 to kick off another CI run using the latest Backburner version...

@rwjblue rwjblue merged commit c099fb0 into emberjs:master Nov 28, 2017
@rwjblue rwjblue deleted the revert-14898 branch November 28, 2017 21:14
@rwjblue
Copy link
Member Author

rwjblue commented Nov 28, 2017

Folks stumbling across this PR may be interested: @hjdivad and I have just published an updated version of ember-test-friendly-error-handler addon which makes creating an Ember.onerror that properly rethrows in development but still allows testing of the "production mode".

@rwjblue
Copy link
Member Author

rwjblue commented Dec 18, 2017

From the original PR description:

As mentioned earlier, the underlying problem reported in #14864 is very real and must be addressed also. Therefore, in order to prevent Ember.onerror from swallowing errors during tests ember-qunit (and ember-mocha) will implement checks to ensure that Ember.onerror (if present) properly rethrows errors during testing.

Finally have this implemented: emberjs/ember-qunit#304.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants