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 release] ensure “pause on exception” pauses in the right place #15599

Closed
wants to merge 1 commit into from

Conversation

stefanpenner
Copy link
Member

It is quite common (like in ember-data’s tests) to not have a global onError handler in tests.
But because of the previous state of code, we would still end up paused in the default dispatchError rather then where the actual error was thrown.

This addresses the issue, but ensuring onErrorTarget.onerror returns undefined if no onError has been set

before:

screen shot 2017-08-25 at 6 15 44 am

After:

screen shot 2017-08-25 at 6 16 01 am

@stefanpenner stefanpenner force-pushed the improve-test-debugging branch 2 times, most recently from dc120d4 to 1fc7137 Compare August 25, 2017 13:37
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

I think we also need a test for this. Something like:

import { 
  onErrorTarget,
  getDispatchOverride,
  setDispatchOverride,
  getOnerror,
  setOnerror
 } from '../error_handler';

const DISPATCH_OVERRIDE = getDispatchOverride();
const ONERROR = getOnerror();

module('error_handler', {
  afterEach() {
    setOnerror(ONERROR);
    setDispatchOverride(DISPATCH_OVERRIDE);
  }
});

test('by default there is no onerror', function(assert) {
  assert.equal(onErrorTarget.onerror, undefined);
});

test('when dispatch override is registered', function(assert) {
  assert.expect(2);
  let override = () => assert.ok(true, 'dispatch called');
  setDispatchOverride(overide);
  assert.equal(onErrorTarget.onerror, override);
});

test('when onerror is registered', function(assert) {
  assert.expect(2);
  let override = () => assert.ok(true, 'onerror called');
  setOnerror(overide);
  assert.equal(onErrorTarget.onerror, override);
});

@@ -13,6 +13,12 @@ let getStack = error => {
return stack;
};

export const onErrorTarget = {
get onerror() {
return dispatchError || getOnerror();
Copy link
Member

@rwjblue rwjblue Aug 25, 2017

Choose a reason for hiding this comment

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

Maybe I'm missing something, but I don't see how this works since dispatchError is always defined just below here?

I would think you want this to actually be:

export const onErrorTarget = {
  get onerror() {
    return getDispatchOverride() || getOnerror();
  }
}

@stefanpenner
Copy link
Member Author

yup, test looks good. Adding it

@rwjblue
Copy link
Member

rwjblue commented Aug 25, 2017

@stefanpenner - We should consider backporting this to 2.12 LTS. Thoughts?

It is quite common (like in ember-data’s tests) to not have a global onError handler in tests.
But because of the previous state of code, we would still end up paused in the default dispatchError rather then where the actual error was thrown.

This addresses the issue, but ensuring onErrorTarget.onerror returns undefined if no onError has been set
@stefanpenner
Copy link
Member Author

There appears to be a BB issue preventing this from working correctly.

@stefanpenner
Copy link
Member Author

I think -> BackburnerJS/backburner.js#262 fixes the BB issue

@stefanpenner
Copy link
Member Author

stefanpenner commented Aug 25, 2017

ya that BB fix address this (linked locally)

@stefanpenner
Copy link
Member Author

oh GH you so silly... #15600

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.

2 participants