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

Acceptance Test Error in PhantomJS #138

Closed
ming-codes opened this issue Sep 19, 2017 · 11 comments
Closed

Acceptance Test Error in PhantomJS #138

ming-codes opened this issue Sep 19, 2017 · 11 comments

Comments

@ming-codes
Copy link
Contributor

ming-codes commented Sep 19, 2017

Attempted to remove a handler from an unknown element or an element with no handlers

I suspect this has to do with the setTimeout in {{vertical-collection}} not registered with Ember run loop. So the test failed to wait for the render cycle to complete. This make sense too as the test I have fails on every fifth build.

@pzuraq
Copy link
Contributor

pzuraq commented Sep 19, 2017

We purposefully use setTimeout and requestAnimationFrame to sidestep the Ember runloop here for performance reasons. requestAnimationFrame is not yet integrated with the runloop, and we need it to be able to interrupt the browser's render cycle to measure and change scroll position when needed. setTimeout is used to schedule an event to occur immediately after the requestAnimationFrame ends.

Both of these events are wrapped in manual calls to run, so they will flush any scheduled actions (see here and here)

Because VC relies on requestAnimationFrame and it is not yet integrated into the runloop, you will have to make your own test waiter to wait for all of the scheduled RAFs to flush. You can likely use registerWaiter, but we are actually using the new style of writing tests using async/await like in the examples in ember-native-dom-helpers and it's working very well. You can take a look at our wait function here, it's just a promise which resolves after flushing the RAF queue. It looks like the native-dom-helpers wait function can be customized too, so you could just do it there.

The other potential problem here is that we don't technically support PhantomJS at the moment, we've been focussing support on all the major modern browsers because Phantom doesn't have the same speed of development as the others, I think when we started it didn't even have RAF. It may work if you get the waiters setup properly, but it may not 😕 The upside is it's now possible to test with headless Chrome which is arguably better overall, since it'll probably reflect Chrome/other browser's behavior much better

@pzuraq
Copy link
Contributor

pzuraq commented Sep 19, 2017

@runspired I think this points to us needing to focus on testing documentation, and possibly add a test helper that can be easily integrated into tradition Ember tests and async/await style tests

@ming-codes
Copy link
Contributor Author

I believe async/await is also hooked into ember-test-helpers/wait and therefore need a registered waiter for it to work. Unfortunately, we're using ember-cli-pages-objects that doesn't use ember-native-dom-helpers. (There's an open ticket for it thought)

If I am to setup a waiter for this, what's the proper way to detect there's nothing left to wait? I tried with the scheduler, but there's always 1 job on there that's being scheduled.

@pzuraq
Copy link
Contributor

pzuraq commented Sep 19, 2017

It is and it isn't. Async/await just waits for promises, so you can have helper fuctions that wait for anything - but that usually means you want to use wait to really flush any runs. With VC it's a little weird since we are flushing synchronously in both cases.

Yeah, I wouldn't hook into the scheduler here, it's very much private API. I'll try to get something working using registerWaiter tomorrow

@ming-codes
Copy link
Contributor Author

I can understand how rAF can improve performance. But how is using setTimeout with manual run different from using Ember.run.later?

@pzuraq
Copy link
Contributor

pzuraq commented Sep 19, 2017

Under the hood Ember.run.later/Ember.run.next do a bit more work for scheduling, they don't just schedule a setTimeout, so there are some differences. I forget the exact reason we switched to using setTimeout directly here though, @runspired do you remember?

@pzuraq
Copy link
Contributor

pzuraq commented Sep 19, 2017

So I wrote this addon that adds a test waiter which completes after two RAFs, similar to the custom waitForRender function we're using in VC. It works, and it seems to work with both acceptance tests and ember-test-helpers, but it's not pretty.

The other strategy I'm considering at the moment is:

  1. Overwrite the acceptance test wait helper (if possible, I think you can do registerAsyncHelper and it'll overwrite the previous definition) with a function that also waits for RAF
  2. export the waitForRender function for use in async/await style and unit tests
  3. assign window.wait = waitForRender to override the wait function in ember-native-dom-helpers

Not sure which one will be more clean/future proof

@ming-codes
Copy link
Contributor Author

Here's another idea. If setTimeout really does improve performance, we can have two code paths. We can use Ember.run.later in the test build and setTimeout in a performance optimized production build. What do you think?

@pzuraq
Copy link
Contributor

pzuraq commented Sep 19, 2017

I'm not sure that setTimeout is the culprit here, the way the wait helper works it should be waiting for scheduled macro tasks to complete as is. requestAnimationFrame is more likely to be the issue. Have you tried the addon I linked above? If it doesn't work and you're sure it is setTimeout, can you post a reproduction so we can dig in?

@ming-codes
Copy link
Contributor Author

Sure, I'll give the addon a try tomorrow and report back.

@ming-codes
Copy link
Contributor Author

Yup, the test waiter addon works 👍

I'll close this issue.

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

No branches or pull requests

2 participants