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

Un-ignore test-suite runtime-styling tests #2847

Merged
merged 10 commits into from
Jul 13, 2016
Merged

Conversation

lucaswoj
Copy link
Contributor

@lucaswoj lucaswoj commented Jul 8, 2016

fixes #2797

cc @jfirebaugh

ref mapbox/mapbox-gl-test-suite#123

Todo

  • figure out if it is ok to not destroy the gl context in test-suite
  • fix [Error: Invalid file signature] log messages due to lack of JPEG support
  • fix CI failures due to wait operations returning too early
  • fix CI segfaults

@lucaswoj
Copy link
Contributor Author

lucaswoj commented Jul 8, 2016

Remaining test suite failures appear to be related to the implementation of wait. There are some cases in which wait will return despite some resources being pending. Proper resolution may require finishing #1715

For reference, this patch "fixes" the test suite

diff --git a/test/suite_implementation.js b/test/suite_implementation.js
index ef42c28..56474dd 100644
--- a/test/suite_implementation.js
+++ b/test/suite_implementation.js
@@ -90,7 +90,9 @@ function applyOperations(map, operations, callback) {
     } else if (operation[0] === 'wait') {
         var wait = function() {
             if (map.loaded()) {
-                applyOperations(map, operations.slice(1), callback);
+                setTimeout(function() {
+                    applyOperations(map, operations.slice(1), callback);
+                }, 500);
             } else {
                 map.once('render', wait);
             }

@lucaswoj
Copy link
Contributor Author

This is ready to go on 🍏

How does this look @mollymerp @jfirebaugh @mourner?

@@ -83,7 +83,7 @@ Buffer.prototype.setVertexAttribPointers = function(gl, program) {
* @param gl The WebGL context
*/
Buffer.prototype.destroy = function(gl) {
if (this.buffer) {
if (gl && this.buffer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

When would gl be falsy here? Instead of adding a guard clause, can we eliminate that case?

Copy link
Contributor Author

@lucaswoj lucaswoj Jul 12, 2016

Choose a reason for hiding this comment

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

A falsy value for gl indicates that we have manually destroyed the GL context (see this code)

Tracking this information prevents "invalid GL context" errors in the runtime styling tests when asynchronous operations are executed after the context has been destroyed.

I will investigate some alternate designs today.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented a (hopefully) cleaner solution in ff07754

@lucaswoj
Copy link
Contributor Author

Updated and ready for 👀

@jfirebaugh
Copy link
Contributor

🚢

@lucaswoj
Copy link
Contributor Author

lucaswoj commented Jul 13, 2016

Before shipping this PR, I am investigating an increased rate of intermittent test-suite segfaults that only occur on CI and seem to be a result of this PR.

@lucaswoj
Copy link
Contributor Author

The CI segfaults are annoying but should not block this release. I will create a separate issue to track their resolution.

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.

Fix crashing runtime-styling test-suite tests
2 participants