[node] Added render test suite implementation that recycles the map object by brunoabinader · Pull Request #9689 · mapbox/mapbox-gl-native

brunoabinader · 2017-08-02T15:42:11Z

This PR:

Splits test-suite target using npm-run-all's run-s script to run individual scripts in sequence.
Adds --recycle-map option to the render test that recycles the map object among test runs.

brunoabinader · 2017-08-02T16:46:44Z

* failed background-pattern @2x - ~~~this means we should be able to update the sprite loader pixel ratio e.g. by recreating a sprite loader object inside Style::Impl.~~~
We cannot update the pixel ratio from a map object once it is created - this means we might need to have different instances of the map object for different pixel ratio values.

kkaefer · 2017-08-03T10:52:14Z

@@ -0,0 +1,118 @@
+'use strict';


Instead of duplicating this file, can you please add the changes to the original suite_implementation and add a command line argument to toggle between the two variants? It looks like the majority of the code is the same.

Depends on mapbox/mapbox-gl-js#5091.

kkaefer · 2017-08-03T10:52:41Z

  "dependencies": {
+    "nan": "^2.4.0",
    "node-pre-gyp": "^0.6.28",
-    "nan": "^2.4.0"


Nit: Reduce diff size

Oh, I think yarn automatically sorts packages in alphabetical order when inserting new dependencies.

brunoabinader · 2017-08-04T10:56:35Z

Results are in!

	Render test (current)	Render test (--recycle-map)
real	1m54.190s	1m0.672s
user	2m12.828s	1m11.892s
sys	0m2.296s	0m1.472s

So far the main obstacles were:

Once a Map object is created with a given pixel ratio, it is currently not possible to modify its value. Thus, we need a map of Map objects, one per pixel ratio.
Not every render test calls for setStyle - in those cases, left overs in the Style::Impl e.g. added images can cause rendering artifacts on the next test run. Thus, we need to somehow clear the style e.g. by setting an empty JSON as in map.load("{}");. Even though we're eventually loading a new style on each test run, it seems the state cleanup made in Style::Impl::parse is not enough.

Caveats:

Because both render tests runs outputs the results to the same HTML file (in render-tests/index.html), only the last render test run is stored as artifact in CircleCI. We might want to either rename the recycling map render test to something else, have two separate HTML files for each run, or select one of the two test runs as base.

What's left:

Node v4 does not properly lookup for inherited dependencies e.g. mapbox-gl-styles is a dependency for @mapbox/mapbox-gl-test-suite (included in package.json), but it is not being installed as should (this works fine in Node v6)
Crash in style parsing in Node v4 (due to the lack of the core style file above):

* passed background-visibility visible
[Error: Failed to parse style: 0 - Invalid value.]
ERROR (ParseStyle): Failed to parse style: 0 - Invalid value.

brunoabinader · 2017-08-04T12:18:47Z

Status:

line-dasharray/literal--line-width-composite-function: [core] Update ZoomHistory.lastInteger{Zoom,Time} if different from floor zoom #9760
line-dasharray/literal--line-width-property-function: [core] Update ZoomHistory.lastInteger{Zoom,Time} if different from floor zoom #9760
line-dasharray/literal--line-width-zoom-function: [core] Update ZoomHistory.lastInteger{Zoom,Time} if different from floor zoom #9760
runtime-styling/image-add-alpha: [core] Ensure all image dependencies are met in GeometryTileWorker::onImagesAvailable #9739
text-pitch-scaling/line-half: Render tests for text-pitch-{alignment,scaling} are false positives in GL native #9732
symbol-avoid-edges/rotated: Ensure GeometryTile::pending state is only false in the last placement attempt #9842
symbol-avoid-edges/rotated-false: Ensure GeometryTile::pending state is only false in the last placement attempt #9842
symbol-avoid-edges/true: Ensure GeometryTile::pending state is only false in the last placement attempt #9842
geojson/external-*: Asynchronism between style and render style causing rendering failures in still mode #9868

brunoabinader · 2017-08-09T13:34:08Z

Render tests now pass locally on both macOS and Linux. This indicates the test failures mentioned in #9689 (comment) could be caused by a race condition when fetching data from local resources.

brunoabinader · 2017-08-25T21:47:13Z

Last known open bug found by this PR is described in #9868 - which concerns some occasional render test failures in geojson/external-* tests.

jfirebaugh · 2017-08-25T23:21:22Z

    "test": "tape platform/node/test/js/**/*.test.js",
    "test-memory": "node --expose-gc platform/node/test/memory.test.js",
-    "test-suite": "node platform/node/test/render.test.js && node platform/node/test/query.test.js"
+    "test-suite": "run-s test-render \"test-render -- --recycle-map\" test-query",


This will overwrite the artifacts from the first test-render run with those from the test-render --recycle-map run. Might be confusing/make it hard to debug.

Also, looks like we've roughly doubled the performance of render tests, but are giving that right back by running them twice. Should we parallelize this somehow? For instance we could run one variant in each of the two node 6 builds.

Let's make it so then that tests results run via --recycle-map will be saved in index-recycle-map.html then.

I'm also 👍 in having e.g. node6-debug running tests with --recycle-map while node6-release runs them as usual.

This is now done in GL JS side via mapbox/mapbox-gl-js#5195. I'll update this PR accordingly.

lbud · 2017-08-28T20:34:39Z

@brunoabinader I merged #9489 so the icon-anchor ignores can be removed now 👍

brunoabinader · 2017-08-28T22:22:09Z

Thanks @lbud! PR is updated now.

brunoabinader · 2017-08-30T13:39:50Z

@kkaefer @jfirebaugh can you please re-review?

brunoabinader · 2017-08-30T15:05:29Z

By the way, though I've added the option to shuffle the render test sequence, I'm going to enable it on CI on a further PR - which also includes implementing --seed.

jfirebaugh

LGTM other than one question.

jfirebaugh · 2017-08-30T19:26:27Z

#9704 is closed -- is this ignore still necessary?

You're right - this is no longer necessary 👍 removing

Based on a previous change proposal.

…debug

brunoabinader added Node.js node-mapbox-gl-native tests labels Aug 2, 2017

brunoabinader requested a review from kkaefer August 2, 2017 15:42

kkaefer previously requested changes Aug 3, 2017

View reviewed changes

brunoabinader force-pushed the node-test-render-recycle-map branch 4 times, most recently from 8e994ec to 2058749 Compare August 4, 2017 10:16

brunoabinader mentioned this pull request Aug 4, 2017

Support fractional minzoom values #9704

Closed

brunoabinader force-pushed the node-test-render-recycle-map branch from 2058749 to ac43003 Compare August 4, 2017 10:18

brunoabinader force-pushed the node-test-render-recycle-map branch from ac43003 to e2ce61e Compare August 4, 2017 11:10

brunoabinader force-pushed the node-test-render-recycle-map branch 4 times, most recently from afe594d to 902f38e Compare August 8, 2017 10:48

brunoabinader mentioned this pull request Aug 8, 2017

Render tests for text-pitch-{alignment,scaling} are false positives in GL native #9732

Closed

brunoabinader force-pushed the node-test-render-recycle-map branch from e779634 to 34f2206 Compare August 9, 2017 08:04

brunoabinader mentioned this pull request Aug 9, 2017

[core] Ensure all image dependencies are met in GeometryTileWorker::onImagesAvailable #9739

Merged

brunoabinader force-pushed the node-test-render-recycle-map branch 2 times, most recently from f2934da to 2044420 Compare August 10, 2017 10:34

This was referenced Aug 10, 2017

[core] Added Style::getDefaultCamera() #9749

Merged

[core] Update ZoomHistory.lastInteger{Zoom,Time} if different from floor zoom #9760

Merged

brunoabinader force-pushed the node-test-render-recycle-map branch 3 times, most recently from 50a479c to fe47ec5 Compare August 14, 2017 15:25

brunoabinader force-pushed the node-test-render-recycle-map branch from fe47ec5 to da842cc Compare August 22, 2017 12:02

jfirebaugh reviewed Aug 25, 2017

View reviewed changes

brunoabinader force-pushed the node-test-render-recycle-map branch from 7c4a443 to 648e73c Compare August 28, 2017 15:44

brunoabinader mentioned this pull request Aug 28, 2017

Add icon-anchor property #9849

Merged

brunoabinader force-pushed the node-test-render-recycle-map branch from 648e73c to 130c241 Compare August 28, 2017 17:57

brunoabinader force-pushed the node-test-render-recycle-map branch from 130c241 to c3df47f Compare August 28, 2017 22:20

brunoabinader force-pushed the node-test-render-recycle-map branch 2 times, most recently from bf23aa9 to ab16876 Compare August 29, 2017 12:11

brunoabinader mentioned this pull request Aug 29, 2017

Still mode optimizations #9884

Merged

brunoabinader force-pushed the node-test-render-recycle-map branch 3 times, most recently from f7969cb to 3bd440a Compare August 30, 2017 13:07

brunoabinader force-pushed the node-test-render-recycle-map branch from 3bd440a to f3b7553 Compare August 30, 2017 13:44

jfirebaugh approved these changes Aug 30, 2017

View reviewed changes

brunoabinader added 7 commits August 31, 2017 09:45

[node] Split 'test-suite' run using 'run-s' script

035bb51

[node] Cleanup NodeMap::Render

b488053

[node] Set style default camera upon each test suite run

b2f5b00

[test] Interpret render tests params: --recycleMap and --shuffle

9390e81

[test] One map per pixel ratio in render.test.js --recycle-map

3a8854b

[test] Unignore render-tests/circle-pitch-alignment/*

4deb559

[test] Enable render tests recycling the map object in node6-clang39-…

ff6acdc

…debug

brunoabinader force-pushed the node-test-render-recycle-map branch from f3b7553 to ff6acdc Compare August 31, 2017 06:47

brunoabinader merged commit 0694d22 into master Aug 31, 2017

brunoabinader deleted the node-test-render-recycle-map branch August 31, 2017 07:17

friedbunny mentioned this pull request Sep 8, 2017

egl-node6-clang39 builds are failing on the v3.6.x release branch #9940

Closed

Conversation

brunoabinader commented Aug 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brunoabinader commented Aug 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brunoabinader commented Aug 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brunoabinader commented Aug 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brunoabinader commented Aug 9, 2017

Uh oh!

brunoabinader commented Aug 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lbud commented Aug 28, 2017

Uh oh!

brunoabinader commented Aug 28, 2017

Uh oh!

brunoabinader commented Aug 30, 2017

Uh oh!

brunoabinader commented Aug 30, 2017

Uh oh!

jfirebaugh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

brunoabinader commented Aug 2, 2017 •

edited

Loading

brunoabinader commented Aug 2, 2017 •

edited

Loading

brunoabinader commented Aug 4, 2017 •

edited

Loading

brunoabinader commented Aug 4, 2017 •

edited

Loading