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] Changed backburner's error handler to use `dispatchError` instead of `onError` #14898

Merged
merged 1 commit into from Feb 25, 2017

Conversation

Projects
None yet
@workmanw
Copy link
Contributor

commented Feb 1, 2017

This is so that backburner errors can be caught by the Test.adapter. Fixes #14864 (more details about the reason for this change in the issue).

@workmanw workmanw force-pushed the workmanw:backburner-dispatchError branch from 97a5273 to dd8b4b8 Feb 1, 2017

@rwjblue

This comment has been minimized.

Copy link
Member

commented Feb 1, 2017

Asking @krisselden for review, I believe he worked on the most recent refactors...

@rwjblue rwjblue requested a review from krisselden Feb 1, 2017

@workmanw workmanw force-pushed the workmanw:backburner-dispatchError branch 2 times, most recently from a69f885 to 20dbbf9 Feb 1, 2017

Wesley Workman
[BUGFIX] Changed backburner's error handler to use `dispatchError` in…
…stead of `onError`. This is so that backburner errors can be caught by the `Test.adapter`. Fixes #14864.

@workmanw workmanw force-pushed the workmanw:backburner-dispatchError branch from 20dbbf9 to 196442d Feb 1, 2017

@workmanw

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2017

@krisselden Any chance you can squeeze in a review? Hopefully this is a very minor change and I'd be happy to do any additional leg work to get this pushed through. I feel pretty strongly about this change because I recently had a regression go to production that slide past our tests due to the exception not being in a promise handler.

@stefanpenner

This comment has been minimized.

Copy link
Member

commented Feb 25, 2017

Pinging kris again

@krisselden krisselden merged commit d357d16 into emberjs:master Feb 25, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rwjblue

This comment has been minimized.

Copy link
Member

commented Feb 25, 2017

Thank you @krisselden!

@workmanw

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2017

Thanks!!! 👏

@stefanpenner

This comment has been minimized.

Copy link
Member

commented Feb 25, 2017

thanks @workmanw sorry for letting this linger. (these PR slip through the cracks too easily)

@workmanw

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2017

@stefanpenner No worries at all, really. I know how these things go. And with any change to ember-metal, I'd much prefer to wait until the proper parties have reviewed it. 👍

@Btown6983

This comment has been minimized.

Copy link

commented Feb 25, 2017

@jasonmit

This comment has been minimized.

Copy link
Member

commented Mar 9, 2017

Tests that were previously asserting errors were thrown are no longer working as expected:

Example of test:

expect(() => {
  this.render(hbs`{{x-foo}}`);
}).to.throw('bar must be defined');

Now the error is captured by mocha and test fails before assertion is made. I'm also hearing this is happening with the qunit adapter as well.

Anyone else experiencing this?

@workmanw

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2017

@miguelcobain Also experienced this with ember-paper. I've been thinking all day about what the best solutions is.

I'm confident in saying that this change was the right thing, even though it broke some addons / apps. The reason I say that is 2 parts, 1) in your code snippet, you're inside of an integration test which has a runloop going and you're expecting an assertion/exception to bubble through the runloop all the way to the top. I think this expectation is problematic because the runloop has it's own error handling, it just was not working the same in all cases which brings me to my second point. 2) If your assertion/exception took place inside of a promise handler, it would have been swallowed in the very same way. Example:

 assert.throws(() => {
    Ember.run(() => {
      Ember.RSVP.resolve(() => { throw 'bar must be defined'; });
    });
}, /bar must be defined/);

That said, I'm definitely very sorry that this caused breakage. Believe me, I know how much it stinks to upgrade your ember version and have broken tests. I did expect maybe a few apps would have broken tests that were legitimate uncaught failures that were now erroring. However I did not expect this side effect, nor know even know that people were testing assertions this way.


So how do we fix this? I would love to have some collaboration on this, but my belief is that the whole point of this change, and the test adapter, is to provide a mechanism to catch errors in RSVP and Backburner.

The idea I've been kicking around is to provides a helper mechanism that leverages replacing the test adapter with a temporary one for a period of time. Here is an example using QUint:

function expectEmberError(assert, callback, match) {
    let origTestAdapter = Ember.Test.adapter;
    let testError;
    let TestAdapter = QUnitAdapter.extend({
      exception(error) {
        error = testError;
      }
    });
 
   Ember.run(() => { Ember.Test.adapter = TestAdapter.create(); });
   callback();
   Ember.run(() => { Ember.Test.adapter.destroy(); });
   Ember.Test.adapter = origTestAdapter;

  assert.ok(error && error.message.match(matcher));
}

exceptEmberError(assert, () => {
    Ember.run(() => {
      Ember.RSVP.resolve(() => { throw 'bar must be defined'; });
    }, /bar must be defined/);
}, /bar must be defined/);

I also maintain https://github.com/workmanw/ember-qunit-assert-helpers and this feels perfect for that (and it would solve the issue with ember-paper, but doesn't help with mocha).

Also, one other thought is I wonder if the TestAdapter were to just rethrow the error (instead of using assert), if that would restore this behavior.

Thoughts? Would you mind trying that last suggestion? You should be able to easily test it by just adding the following to your setup:

import MochaTestAdapter from 'ember-mocha-adapter';

Ember.Test.adapter = MochaTestAdapter.extend({
  exception(error) {
    throw error;
  }
}).create();
@jasonmit

This comment has been minimized.

Copy link
Member

commented Mar 9, 2017

@workmanw a variation of what you posted seems to do the trick. I opened an issue against ember-mocha so it's also on their radar.

Also, no need to apologize this is the cost of moving things forward sometimes :)

@toranb

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2017

@jasonmit this also broke my test suite for the following

test('each computed is truly readonly', function(assert) {
  assert.expect(1);
  this.render(hbs`{{count-list}}`);
  try {
    this.$('.btn-alter').trigger('click');
  } catch (e) {
    assert.ok(e.message.indexOf('Cannot set read-only property') > -1);
  }
});
@toranb

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2017

@workmanw I was able to update this to work with qunit using assert.throws

test('each computed is truly readonly', function(assert) {
  assert.expect(1);
  this.render(hbs`{{count-list}}`);
  Ember.run(() => {
    assert.throws(() => {
      this.$('.btn-alter').trigger('click');
    }, (e) => {
      return e.message.indexOf('Cannot set read-only property') > -1;
    });
  });
});

The full diff shows the qunit adapter hacking required to make this work (not usually something ember core is proud to see me showing off but it does solve the immediate issue)

https://github.com/ember-redux/ember-redux/pull/102/files

@toranb

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2017

@workmanw I spoke 2 soon after all :) was able to dump the hacks I had previously and the diff is just a swap of the try/catch for assert throws (wrapped inside ember.run)

@jasonmit

This comment has been minimized.

Copy link
Member

commented Mar 10, 2017

@toranb that sounds much cleaner :) Thanks for circling back

@jasonmit

This comment has been minimized.

Copy link
Member

commented Mar 11, 2017

@toranb I unfortunately wasn't able to get this working, but in my failing test I was throwing "on render" and not an action. Perhaps this can be handled by patching this.render?

For now, I'll keep the expectError utility method until we land fixes in all the right places..

@cibernox

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2017

I am on a similar situation. Tests in ember-power-select started failing with the update from 2.11.2 to 2.11.3.

The test itself is hard to fix because the error happens immediatly when the select is rendered without the mandatory onchange action.

Test: https://github.com/cibernox/ember-power-select/blob/master/tests/integration/components/power-select/assertions-and-deprecations-test.js#L9-L19

Assertion: https://github.com/cibernox/ember-power-select/blob/master/addon/components/power-select.js#L99-L109

I could use some guidance, because everything I've tried (wrap things un Ember.run, use try/catch...) didn't work.

@workmanw

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2017

Sorry for the delay on my part. So I was able to make the adapter helper I previously discussed work. Here is a complete QUnit test that pass with Ember-2.11.3.

// tests/integration/component-assert-test.js
import { test, moduleForComponent } from 'ember-qunit';
import hbs from 'htmlbars-inline-precompile';
import { QUnitAdapter } from 'ember-qunit';
import Ember from 'ember';

function expectEmberAssert(qAssert, callback, matcher) {
  let origTestAdapter = Ember.Test.adapter, testError;
  let TestAdapter = QUnitAdapter.extend({
    exception(error) {
      testError = error;
    }
  });

  Ember.run(() => { Ember.Test.adapter = TestAdapter.create(); });
  callback();
  Ember.run(() => {
    Ember.Test.adapter.destroy();
    Ember.Test.adapter = origTestAdapter;
  });

  qAssert.ok(testError && testError.message && testError.message.match(matcher));
}

moduleForComponent('x-assert-test', 'Ember.assert in integration test', {
  integration: true,
  beforeEach() {
    this.register('component:x-assert-test', Ember.Component.extend({
      init() {
        this._super();
        Ember.assert('x-assert-test will always assert');
      }
    }));
  }
});

test('Check for assert', function(assert) {
  expectEmberAssert(assert, () => {
    this.render(hbs`{{x-assert-test}}`);
  }, /x-assert-test will always assert/);
});

So the impact for the tests are minor, just have to use a new utility for asserting.

As I previously mentioned I've been working on ember-qunit-assert-helpers to provide qunit aware asserts. I will carve out some time today to add this helper over there and cut a new version.

workmanw pushed a commit to workmanw/ember-qunit-assert-helpers that referenced this pull request Mar 12, 2017

Wesley Workman
Improved the implementation of `expectAssertion` to handle assertions…
… thrown inside of a runloop during an integration test. Related to: emberjs/ember.js#14898 .

workmanw added a commit to workmanw/ember-qunit-assert-helpers that referenced this pull request Mar 12, 2017

Improved the implementation of `expectAssertion` to handle assertions…
… thrown inside of a runloop during an integration test. Related to: emberjs/ember.js#14898 .
@workmanw

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2017

Okay @miguelcobain @toranb @cibernox . As promised, I added this functionality to ember-qunit-assert-helpers and released a new version 0.1.3. If you add that addon to your package, it "should be" as simple as changing assert.throws to assert.expectAssertion.

assert.expectAssertion(() => {
  this.render(hbs`
    {{#power-select options=countries selected=selected as |opt|}}{{opt}}{{/power-select}}
  `);
}, /requires an `onchange` function/);
@toranb

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2017

@cibernox I'd take a look at the work @workmanw did w/ that qunit helper assuming you have the bandwidth. Without that it may be possible to get it working with something like the below (but this isn't anything I would advice people do generally as it requires monkey patching the TestAdapter/ and Logger)

let application, originalLoggerError, originalTestAdapterException;

module('testing some error that is thrown up ... good times', {
    beforeEach() {
        application = startApp(); //if acceptance testing
        originalLoggerError = Ember.Logger.error;
        originalTestAdapterException = Ember.Test.adapter.exception;
        Ember.Logger.error = function() {};
        Ember.Test.adapter.exception = function() {};
    },
    afterEach() {
        Ember.Logger.error = originalLoggerError;
        Ember.Test.adapter.exception = originalTestAdapterException;
        Ember.run(application, 'destroy');
    }
});
@workmanw

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2017

Also, FWIW, @rwjblue proposed adding an additional API to ember-qunit that makes this process more sane, without replacing the adapter. I'm going to pursue that and submit a PR this week. See: workmanw/ember-qunit-assert-helpers#6 (comment)

@cibernox

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2017

@workmanw I'll then wait a couple days to see how this evolves. Thanks you all for the prompt response.

@sandersky

This comment has been minimized.

Copy link

commented Mar 13, 2017

I don't want to argue whether or not this fix should go in, but I personally think Ember 2.11.4 should be released to revert this change. Adding breaking changes in patch releases isn't very kosher when it comes to semver.

@juwara0 juwara0 referenced this pull request Mar 13, 2017

Merged

Add updates to get code coverage working #57

1 of 4 tasks complete

bantic added a commit to bustle/ember-mobiledoc-editor that referenced this pull request Mar 13, 2017

Use TestAdapter to ensure render errors are thrown
Uses a customized test adapter for some tests that will rethrow errors
that occur during the render cycle. This fixes a few tests that were
false-negativing. Notes on the change to Ember are in emberjs/ember.js#14898

Fixes #128

bantic added a commit to bustle/ember-mobiledoc-editor that referenced this pull request Mar 13, 2017

Use TestAdapter to ensure render errors are thrown (#129)
Uses a customized test adapter for some tests that will rethrow errors
that occur during the render cycle. This fixes a few tests that were
false-negativing. Notes on the change to Ember are in emberjs/ember.js#14898

Fixes #128

@kevinansfield kevinansfield referenced this pull request Mar 14, 2017

Merged

Dependency updates #581

san650 added a commit to san650/ember-cli-page-object that referenced this pull request Mar 31, 2017

Do not use assert.throws when testing actions in integration tests
ember@2.11.3 introduces a (breaking?) change in how exceptions are
propagated in backburner which causes some test that were previously
working, to fail.

See emberjs/ember.js#14898 (comment)

Also fix some descriptions in blueprint tests
@lolmaus

This comment has been minimized.

Copy link

commented Jul 19, 2017

We're using Ember 2.11.3 which includes this fix, yet we're still running into #12791.

Please tell me if this PR is supposed to resolve #12791.

Use case: we have a store.findRecord().catch() in a route. The catch makes 404 errors silent and transparent for end user. But the test still fails due to a 404 from Mirage.

@stefanpenner

This comment has been minimized.

Copy link
Member

commented Sep 1, 2017

This PR appears to break the following invariant:

Ember.run(() => {
  throw new Error('error');
});

// we get here, but we should not. `Ember.run` now eats the error

and I think #15600 addresses.

@workmanw

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2017

Going to follow up on #15600.

@runspired

This comment has been minimized.

Copy link
Contributor

commented Nov 13, 2017

So I know I'm late to this party and I'm about to stand on a soap-box but I really think this was the wrong idea all around. While this change does help catch a class of errors that were otherwise silently ignored, it also breaks every expectation of "how the system works" and takes us very far from writing "just Javascript".

For example, I write this code:

class CustomError extends Error {}
...
throw new CustomError('we are error prone!');

With this change, in order to test my error case, I now must

  1. understand what the run-loop is
  2. determine whether my error will be thrown within a run-loop in the test
  3. understand that if I am in a run-loop, I will receive a totally different error thrown from an internal Ember.assert instead of the actual Error that I wrote and threw.
  4. hack my way around carefully annotating what I did and why if I want to assert the actual error class in tests
  5. give up in despair if I both need a run-loop and access to the error class

In addition, breaking the ability to use try ... catch and assert.throws in tests is a not insignificant change.

@rwjblue

This comment has been minimized.

Copy link
Member

commented Nov 27, 2017

For those interested, I submitted #15871 which is a partial revert of this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.