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

Expose Ember.Test.checkWaiters as public API #13603

Closed
mixonic opened this issue Jun 4, 2016 · 10 comments
Closed

Expose Ember.Test.checkWaiters as public API #13603

mixonic opened this issue Jun 4, 2016 · 10 comments

Comments

@mixonic
Copy link
Sponsor Member

mixonic commented Jun 4, 2016

#13440 landed a big refactor of Ember.Test. Among the changes was to remove Ember.Test.waiters 💁 from the exposed API. waiters leaked the internal implementation of the waiter queue, and was not explicitly flagged as @public.

In updating ember-test-helpers @rwjblue and I found that Ember.Test.waiters is used by that library to support wait() ✋ in integration tests. See: https://github.com/switchfly/ember-test-helpers/blob/6207a7b6af9214ef47a79f52d9b240bb95e6a728/lib/ember-test-helpers/wait.js#L46

Though we're going to ship a hacky intimate-API workaround for the 2.7 beta (master at this writing), it seems like checkWaiters should be made public API to support this use case. Just something that was missed when Ember.Test.waiters was removed. Thoughts @krisselden?

And as a point of order, no, I didn't write an RFC as I do not believe this change would represent "substantial" change to Ember. IMO we're simply recovering from the mistake of dropping waiters, which was not documented as public but was actually used. ✌️

@rwjblue
Copy link
Member

rwjblue commented Jun 4, 2016

I am in favor of exposing the new checkWaiters implementation at Ember.Test.checkWaiters, and then adding Ember.Test.waiters back as an accessor with a deprecation.

@workmanw
Copy link

In updating ember-test-helpers @rwjblue and I found that Ember.Test.waiters is used by that library to support wait() ✋ in integration tests.

Yeap. This definitely is responsible for the remaining test failures we're seeing with ember-2.7.0-beta.1.

@rwjblue
Copy link
Member

rwjblue commented Jun 14, 2016

@workmanw - How so? You were using Ember.Test.waiters directly (I would definitely suggest that this be left to the testing harness...)?

@workmanw
Copy link

@rwjblue No. And perhaps I misunderstood and it's a slightly different issue that needs filed. But in my integration test, when I call wait() it doesn't actually wait on the registered waiters anymore.

So on first look I thought it was related to ember-test-helpers (and it might still be). But this line here: https://github.com/switchfly/ember-test-helpers/blob/v0.5.23/lib/ember-test-helpers/wait.js#L46 . When debugging in my test suite, Ember.Test.waiters is undefined so it skips the check for waiters, even though some have definitely been registered.

I thought it was related to this issue ... I think I jumped to conclusion because of the regression tag and talk of waiting. Perhaps this is another issue. Perhaps related to the fact I'm using an older version of ember-test-helpers.

@rwjblue
Copy link
Member

rwjblue commented Jun 14, 2016

But in my integration test, when I call wait() it doesn't actually wait on the registered waiters anymore.

This is most likely due to emberjs/ember-test-helpers#161 (comment). Using ember-test-helpers < 0.5.26 with Ember 2.7.0-beta.1 will not be able to wait for Ember.Test.waiters (it was fixed with emberjs/ember-test-helpers@965b154). I'll try to get emberjs/ember-test-helpers#161 fixed quickly.

However, if we make Ember.Test.waiters a getter as @mixonic suggested, you should still be able to pass your tests using the same version of ember-test-helpers. So I'll likely work on that first...

@workmanw
Copy link

@rwjblue So looking more closely it does seem this is related to: #13440. Because the Ember.Testing.waiters is no longer exposed. It seems like you accounted for this change here: emberjs/ember-test-helpers@965b154, but that unfortunately creates a version incompatibility between ember-test-helpers: 0.5.23 and ember: 2.7.0-beta.1.

And I'm stuck on the older version of ember-test-helpers because of: emberjs/ember-test-helpers#161 . So that leaves me in a pickle I guess.

@rwjblue
Copy link
Member

rwjblue commented Jun 14, 2016

Yep. We'll get it squared away. I'll try to poke at this more first thing tomorrow morning.

@workmanw
Copy link

Happy to help do any of the leg work here. Let me know if there is something I can dive into.

@rwjblue
Copy link
Member

rwjblue commented Jun 19, 2016

Submitted #13716 to address this.

@workmanw
Copy link

@rwjblue Awesome. Thank you!

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

No branches or pull requests

4 participants