Skip to content

Only run raster operations after image sources have loaded #4894

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

Merged
merged 7 commits into from
Feb 23, 2016
Merged

Only run raster operations after image sources have loaded #4894

merged 7 commits into from
Feb 23, 2016

Conversation

tschaub
Copy link
Member

@tschaub tschaub commented Feb 23, 2016

The raster tests were not properly cleaning up after themselves. Listeners for afteroperations were getting called a second time after we had called done() in the tests. This made it look like the wrong tests were failing.

This cleans up the raster tests by calling obj.dispose() on reused test objects and only listening for events once where we call done() in the listener.

There may still be an issue introduced in 98b823c with operations being run more often than expected. But we should create a specific test that demonstrates the issue if one can be found. The existing test failures were the result of sloppy test running.

update - this should now fix #4893.

@tschaub
Copy link
Member Author

tschaub commented Feb 23, 2016

In this branch, tests fail before 24b7b27 and pass after it. So there was really only one change needed. But the other changes made the problem clearer.

@ahocevar
Copy link
Member

This looks much clearer now. Thanks @tschaub, and please merge when Travis is happy.

@tschaub
Copy link
Member Author

tschaub commented Feb 23, 2016

Well, Chrome is still not happy. I lied about 24b7b27 making all tests pass. The first setOperation() test is still failing. This looks legit and could be the root of the problem.

@ahocevar
Copy link
Member

@tschaub, 9983586 fixes the issue. I haven't tested yet whether this has unwanted side effects, but the problem is that previously the renderer's getImage() method was called, which returned null if the source was not ready. Now, composeFrame() is called, and we do that without checking the return value from prepareFrame() to see whether there is something to render at all.

@tschaub tschaub changed the title Clean up raster tests Fix raster tests Feb 23, 2016
@tschaub
Copy link
Member Author

tschaub commented Feb 23, 2016

Thanks for the underlying fix @ahocevar. To summarize for others:

So, it is nice that we have a test that actually covers the problem. But it is a bummer that these tests are not run as part of our CI setup. Travis/Sauce integration can help - though it is a bit involved to get it working for 3rd party pull requests.

@tschaub tschaub changed the title Fix raster tests Only run raster operations after image sources have loaded Feb 23, 2016
tschaub added a commit that referenced this pull request Feb 23, 2016
Only run raster operations after image sources have loaded.
@tschaub tschaub merged commit 4b55175 into openlayers:master Feb 23, 2016
@tschaub tschaub deleted the raster-tests branch February 23, 2016 23:10
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.

ol.source.Raster tests are failing
2 participants