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

Ember 2.15 - Integration tests with rejected promise #15569

Closed
amk221 opened this issue Aug 4, 2017 · 18 comments
Closed

Ember 2.15 - Integration tests with rejected promise #15569

amk221 opened this issue Aug 4, 2017 · 18 comments

Comments

@amk221
Copy link
Contributor

@amk221 amk221 commented Aug 4, 2017

Using 2.14.1, I can test both success and failure scenarios work when resolving promises.

Switching to v2.15.0-beta.1 however results in this error:

screen shot 2017-08-04 at 14 18 49

I was unable to replicate in a Twiddle, so I have created a demo repo instead.

The relevant files are:
my-component.js
my-component-test.js

Unsure, but might be related to #15490

@amk221 amk221 changed the title Integration tests with rejected promise Ember 2.15 beta - Integration tests with rejected promise Aug 11, 2017
@pixelhandler

This comment has been minimized.

Copy link
Contributor

@pixelhandler pixelhandler commented Aug 25, 2017

I'm curious why there are 2 assertions run, also it seems the error isn't rendered in the component.

Also, what is this ~ usage for?

{{~#if result~}}
  result: {{result}}
{{~else if error~}}
  error: {{error}}
{{~else~}}
  please wait
{{~/if~}}

I don't recall seeing the ~ before.

@amk221

This comment has been minimized.

Copy link
Contributor Author

@amk221 amk221 commented Aug 28, 2017

The tidle remove the whitespace but is not relevant to the test case.

Yes, that is my point - in previous versions I could test the error was rendered
After upgrading I can't.

@amk221

This comment has been minimized.

Copy link
Contributor Author

@amk221 amk221 commented Sep 7, 2017

Testing successful and unsuccessful outcomes for rendering a component that handles promises is such a common requirement. I'm very surprised nobody else has chimed in.

@amk221 amk221 changed the title Ember 2.15 beta - Integration tests with rejected promise Ember 2.15 - Integration tests with rejected promise Sep 20, 2017
@lindyhopchris

This comment has been minimized.

Copy link

@lindyhopchris lindyhopchris commented Oct 2, 2017

@amk221 I'm seeing this too in my app. I'm doing the following in a component:

  doSignIn: task(function * (credentials) {
    try {
      this.set('error', null);
      yield this.get('onSignIn')(credentials);
    } catch (err) {
      this.set('error', getWithDefault(err, 'message', 'Could not sign you in.'));
    } finally {
      this.set('password', null);
    }
  }).drop()

A test involving throwing an error from the onSignIn callback is passing on 2.14 but not in 2.15

@amk221

This comment has been minimized.

Copy link
Contributor Author

@amk221 amk221 commented Oct 2, 2017

@lindyhopchris Glad I'm not the only one experiencing this (but sorry you are too!)
Some feedback from the core team would be greatly appreciated. It's been over a month now...

@rwjblue

This comment has been minimized.

Copy link
Member

@rwjblue rwjblue commented Oct 2, 2017

I missed that there was a demo repo in your intro (sorry about that), I'll try to poke at this later today...

@pixelhandler

This comment has been minimized.

Copy link
Contributor

@pixelhandler pixelhandler commented Nov 3, 2017

@amk221 any luck with Ember 2.16.x ?

@amk221

This comment has been minimized.

Copy link
Contributor Author

@amk221 amk221 commented Nov 3, 2017

Nope, not with 2.16.0

@amk221

This comment has been minimized.

Copy link
Contributor Author

@amk221 amk221 commented Nov 4, 2017

I updated the test repo to 2.16.2 https://github.com/amk221/-ember-rsvp-test-failure

@rwjblue

This comment has been minimized.

Copy link
Member

@rwjblue rwjblue commented Nov 4, 2017

We probably need to bisect this to figure out where the regression was introduced. Nothing jumps out at me from the changelog...

@Leooo

This comment has been minimized.

Copy link
Contributor

@Leooo Leooo commented Nov 6, 2017

Isn't it #15694 ?

@rwjblue

This comment has been minimized.

Copy link
Member

@rwjblue rwjblue commented Nov 6, 2017

I don't think so, #15694 is about rejecting without a reason. In this case, the rejection has a provided reason...

@amk221

This comment has been minimized.

Copy link
Contributor Author

@amk221 amk221 commented Nov 21, 2017

I built the test repo with 2.14.1 and 2.15.0-beta.1 and diff'd the output, but was unable to find anything obvious

@rwjblue

This comment has been minimized.

Copy link
Member

@rwjblue rwjblue commented Nov 28, 2017

I believe this will be addressed by BackburnerJS/backburner.js#277 but need to test the reproduction to confirm.

@rwjblue

This comment has been minimized.

Copy link
Member

@rwjblue rwjblue commented Nov 28, 2017

OK, I have finally run this issue to ground. It's a doozy...

The underlying principle in tests is that rejected promises which are not handled within the same run loop trigger the unhandled rejection handler (which by default calls assert.ok(false, ....) and fails the current test). Generally speaking, this is very desireable ergonomics wise.

Given this test (which passes on Ember <= 2.14 and fails on 2.15+):

test('error', function(assert) {
  assert.expect(1);

  this.set('promise', RSVP.reject('bar'));

  this.render(hbs`{{my-component promise=promise}}`);

  return wait().then(() => {
    assert.equal(this.$().text(), 'error: bar');
  });
});

We can see that the following things are happening:

  1. create a rejected promise (RSVP.reject('bar')) which queues up the unhandled rejection assertion
  2. invoke this.set (which is essentially Ember.run(Ember.set, this, rejectedPromise))
  3. invoke this.render (which is ultimately going to instantiate the component with the provided promise, and call promise.then(handleSuccess, handleFailure))

The primary difference between the Ember versions is how the unhandled rejection assertion is delivered.

In Ember <= 2.14 the assertion is queued up to be ran after a flush of the run loop on the next tick (e.g. setTimeout(flushAndAssert, 0)). Since both 2) and 3) above are synchronous, this meant that under Ember <= 2.14, the promise rejection was handled by the time the unhandled rejection assertion is ran, and therefore no assertion is made.

In Ember 2.15+ (versions which include BackburnerJS/backburner.js#203), the assertion is queued up to be ran after the flush of the next run loop. This means that when the run loop from 2) is ran, the assertion is triggered and therefore the test fails.

Unfortunately, I believe the change in behavior is desirable and should remain.


There are a few different ways to successfully test this interaction. I've made separate PR's to the demo repo to show each of them:

  1. Pass in a pending promise, then either resolve or reject as needed: amk221/-ember-rsvp-test-failure#1
  2. Chain off of the rejected promise to reset its "unhandled rejection" state and avoid the assertion: amk221/-ember-rsvp-test-failure#2
  3. Monkey patch Ember.Test.adapter.exception to swallow the unhandled rejection assertion: amk221/-ember-rsvp-test-failure#3
@rwjblue

This comment has been minimized.

Copy link
Member

@rwjblue rwjblue commented Nov 28, 2017

I believe the new behavior is more correct (though I agree with @amk221 that the change in behavior was surprising), so I am going to close this... (happy to reopen if I've made a mistake above though)

@lindyhopchris

This comment has been minimized.

Copy link

@lindyhopchris lindyhopchris commented Nov 29, 2017

@rwjblue thanks for the detailed explanation, I like the look of option 1 so will give that a go. I'm more than happy that this is closed as I'm all in favour of being shown a neater way of testing the scenario! Thanks for your help.

@mani-mishra

This comment has been minimized.

Copy link
Contributor

@mani-mishra mani-mishra commented Jan 30, 2018

We too experienced a version of this in our tests.
We use sinon.js for stubbing functions calls.

So a test like below, which was passing before 2.15,

test('it shows correct error message', function(assert) {
   const sandbox = sinon.sandbox.create();
  // stubs some async call which is made in component
   const stub = sandbox.stub(Model, 'function').returns(RSVP.reject('some error')); // Line 2
   this.render(hbs`{{my-component}}`);
  return wait().then(() => {
    assert.equal('do some assertion');
  });
});

It fails in latest ember (tried in 2.18), where the RSVP.reject propagates up the chain and breaks the test.

Apart from @rwjblue suggestion above, changing the line 2 to use sinon rejects helper also works. Sinon rejects/resolves methods return native JS promise and not RSVP.

const stub = sandbox.stub((Model, 'function').rejects(); // Line 2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.