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

Commits on Nov 28, 2017

  1. [BUGFIX lts] Revert "[BUGFIX] Changed backburner's error handler to u…

    …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 committed Nov 28, 2017
    Copy the full SHA
    8360825 View commit details
    Browse the repository at this point in the history
  2. [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.
    rwjblue committed Nov 28, 2017
    Copy the full SHA
    1d37ba6 View commit details
    Browse the repository at this point in the history
  3. [BUGFIX lts] Allow Ember.Test.adapter to opt out of exception handl…

    …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.
    rwjblue committed Nov 28, 2017
    Copy the full SHA
    80f7d6b View commit details
    Browse the repository at this point in the history
  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.
    rwjblue committed Nov 28, 2017
    Copy the full SHA
    0f3f49c View commit details
    Browse the repository at this point in the history