search

cibernox / ember-native-dom-helpers

Test helpers for your integration tests that fire native events
MIT License
186 stars 37 forks source link

Simplify `waitUntil` #130

Open rwjblue opened 6 years ago

rwjblue commented 6 years ago

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

rwjblue commented 6 years ago

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 commented 6 years ago

Failing this assertion: https://github.com/cibernox/ember-native-dom-helpers/blob/6300642eb033c12631fe83b6b22c70594f6dff4c/tests/integration/wait-until-test.js#L113

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 commented 6 years ago

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 commented 6 years ago

@simonihmig Did you do it for solving a problem?

simonihmig commented 6 years ago

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: https://github.com/cibernox/ember-native-dom-helpers/blob/6300642eb033c12631fe83b6b22c70594f6dff4c/tests/integration/wait-until-test.js#L115

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 commented 6 years ago

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 commented 6 years ago

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