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

Simplify waitUntil #130

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Simplify waitUntil #130

wants to merge 1 commit into from

Conversation

rwjblue
Copy link
Collaborator

@rwjblue rwjblue commented Dec 6, 2017

Was working on migrating this to @ember/test-helpers starting with the initial implementation here and simplified it a bit.

Was working on migrating this to `@ember/test-helpers` starting with the initial implementation here and simplified it a bit.
@rwjblue
Copy link
Collaborator Author

rwjblue commented Dec 6, 2017

We could also probably remove the setTimeout(tick, 10) in the root of the promise body (by replacing with tick() directly), but it didn't seem much better (and guaranteeing a new stack seemed good to me).

@rwjblue
Copy link
Collaborator Author

rwjblue commented Dec 6, 2017

Failing this assertion:

assert.ok(elapsedTime < 10, 'The Promise resolved immediately');

This would be fixed by invoking tick() at the end of the promise callback body, but TBH I think the assertion should likely be removed. The only reason that it passes now (reliably anyways) is that Ember.RSVP is configured to resolve in the run-loop so things don't always get a next tick...

@cibernox
Copy link
Owner

cibernox commented Dec 6, 2017

That immediate resolution was added in #97 to avoid the 10ms delay for checking something that might already be true. I don't have strong feelings, but I think that it makes sense to test the condition immediately before doing any waiting.

@cibernox
Copy link
Owner

cibernox commented Dec 6, 2017

@simonihmig Did you do it for solving a problem?

@simonihmig
Copy link
Contributor

Did you do it for solving a problem?

IIRC the main problem I was trying to solve in that mentioned PR was that before that it would immediately resolve the promise but still schedule another tick timeout, leading to a situation that this tick call could be run when the test was already finished and teared down (because of the immediately resolving promise), leading to an exception (no DOM available any more) afterwards.

So the main assertion that I think is important there is that the callback is called just once:

assert.equal(count, 1, 'Callback was called just once');

Wether the promise resolves immediately (when the condition is fulfilled) or after a first timeout is not important to me. I also think I would make sense to immediately resolve, but I have no strong feelings about that either.

@rwjblue
Copy link
Collaborator Author

rwjblue commented Dec 7, 2017

I currently have https://github.com/rwjblue/ember-test-helpers/blob/b86cd963d6ff383be634d2f76a49bdbbd184578c/addon-test-support/%40ember/test-helpers/wait-until.js#L9-L28 in the WIP PR over in ember-test-helpers that I have been working on.

At least in ember-test-helpers its pretty important that the helper is always async (having APIs that are sometimes sync and sometimes async is super error prone), in the current WIP (link above) I am running tick function immediately (basically setTimeout(tick, 0)) which results in minimal delay when the condition is true, but still guarantees things are async.

I'll continue iterating over in that PR (though I would absolutely love the continued help reviewing), and once things have settled implementation wise I'll make another crack at syncing the two libs back up...

@cibernox
Copy link
Owner

cibernox commented Dec 7, 2017

If asynchronously is important them I'm all for removing it.

let time = 0;
let tick = function() {
function tick() {
Copy link
Owner

Choose a reason for hiding this comment

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

If I'm right, you should schedule the first tick with a 0 ms delay to exit as soon as possible but still respond asynchronously, correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, confirm. We need to change the initial time value to -10 and change the first tick to use 0. That’s what I did in my branch in the test helpers.

Copy link
Owner

Choose a reason for hiding this comment

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

Can you port that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, absolutely. Was just trying to land it in test helpers then bring back any changes/tweaks from the implementations here once the PR in ember-test-helpers has landed.

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.

3 participants