Add runtime-styling tests for 'smart setStyle' #170

anandthakker · 2016-11-16T02:45:52Z

For each existing runtime-styling test, adds a corresponding test using setStyle

anandthakker · 2016-11-16T02:48:07Z

The first commit is just the bulk copy of each runtime-styling/xxxx/ dir to runtime-styling/set-style-xxxx/, so probably slightly easier to read by just looking at each of the subsequent commits.

jfirebaugh · 2016-11-16T18:30:50Z

Looks good! Can you also add tests for:

Changes to layer.source, layer.source-layer, and layer.type
Changes to glyphs and sprite
Changes to sources that trigger layer add/readd
Layer reordering

anandthakker · 2016-11-16T18:33:29Z

👍 ack--yeah, good call.

anandthakker · 2016-11-16T19:04:12Z

Changes to glyphs

This one might be a weird -- I think what this will entail is putting a set of font data that is different from glyphs/Open Sans Semibold,Arial Unicode MS Bold somewhere like glyphs-alternate/Open Sans Semibold,Arial Unicode MS Bold, so that we can do a setStyle that changes local://glyphs/{fontstack}/{range}.pbf to local://glyphs-alternate/{fontstack}/{range}.pbf and actually produces a different visual result. I'm not at all familiar w/ the glyphs stuff -- does this sound about right?

anandthakker · 2016-11-16T19:08:11Z

Oh, actually -- maybe it would be simpler (and less contrived?) to add glyphs-atlernate/Some Other Font, and then, in the setStyle operation, update glyphs: and change the style to use Some Other Font

jfirebaugh · 2016-11-16T19:18:43Z

Would a hack like local://glyphs/alternate/../{fontstack}/{range}.pbf work?

anandthakker · 2016-11-16T19:21:12Z

@jfirebaugh It would probably work as far as testing that setStyle doesn't blow up, but my concern is that, since this URL would be pointing to the same font files, the expected output after the setStyle operation would be identical to what's shown before it -- so the test would not be able to distinguish between setStyle incorrectly being a noop vs. actually doing the work.

lucaswoj · 2016-11-17T00:14:01Z

You should be able to change the url from local://glyphs/Open Sans Semibold,Arial Unicode MS Bold/{range}.pbf to local://glyphs/NotoCJK/{range}.pbf

anandthakker · 2016-11-17T00:15:26Z

Ohh, nice--that would be simpler.

jfirebaugh · 2016-11-17T00:18:02Z

https://github.com/mapbox/mapbox-gl-style-spec/blob/mb-pages/lib/validate/validate_glyphs_url.js#L14

local://glyphs/Open Sans Semibold,Arial Unicode MS Bold/{range}.pbf?ignore={fontstack} would work though. 😁

anandthakker · 2016-11-17T01:20:36Z

Update:

Added regression test for Removing a symbol layer and immediately readding it as a circle layer causes TypeError in draw_circle mapbox-gl-js#3633 (Normally this would belong in its own PR, but since set-style-layer-change-source-type is being added here, and is equivalent, I figured I might as well sneak the regression test in.
Skipped tests for native, confirming locally (via npm link) that the suite passes.
Added the following tests that @jfirebaugh pointed out above (and that I really should have completed in the first place before submitting for review 😬 )
- Changes to layer.source, layer.source-layer, and layer.type: set-style-layer-change-{source,source-type,source-layer}
- Changes to glyphs and sprite: set-style-{sprite,glyphs}
- Changes to sources that trigger layer add/readd set-style-source-update
- Layer reordering set-style-layer-reorder

anandthakker · 2016-11-17T02:26:31Z

Ugh, weirdness. The text-font/chinese test fails on my machine due to three of the icons being very slightly offset. Happens on gl-js master and also on the smart-set-style branch.

Not a big deal if CircleCI is still happy, except that this probably means that the expected image for the set-style-glyphs test (which is based on this one and which I generated with UPDATE=1 on my laptop) is gonna be wrong as well

anandthakker · 2016-11-17T02:26:40Z

anandthakker · 2016-11-17T02:30:56Z

K, dealt with the above by just removing the icons. They aren't relevant to this test anyway.

anandthakker · 2016-11-17T02:49:17Z

render-tests/runtime-styling/set-style-layer-change-source-layer/style.json

+              "id": "fill",
+              "type": "fill",
+              "source": "vector",
+              "source-layer": "building"


Reason that expected.png looks right -- original style uses source-layer: 'landuse'.

Here's what the geojson dumps look like for landuse and building, respectively:

(the rendered image is right around the traffic circle)

anandthakker · 2016-11-17T02:50:09Z

render-tests/runtime-styling/set-style-layer-change-source-type/style.json

+                "id": "circle",
+                "type": "circle",
+                "source": "geojson"
+              }


Reason that expected.png looks right: the original circle layer uses an sdf icon colored red.

anandthakker · 2016-11-17T02:51:16Z

render-tests/runtime-styling/set-style-layer-change-source/style.json

+              {
+                "id": "circle",
+                "type": "circle",
+                "source": "geojson"


Reason that expected.png looks right: the original circle layer points at wrong-geojson, whose coordinates are off-center.

anandthakker · 2016-11-17T02:55:00Z

render-tests/runtime-styling/set-style-source-update/style.json

+                    0
+                  ]
+                }
+              }


Reason that expected.png looks right: in the original style, the point is off-center.

anandthakker · 2016-11-17T02:58:26Z

render-tests/runtime-styling/set-style-layer-reorder/style.json

+                "id": "circle",
+                "type": "circle",
+                "source": "geojson"
+              }


Reason that expected.png looks right: in the original style, the smaller, red circle (circle-2) comes after the black one, so that it appears in the center.

(Note -- in this commit, this test is wrong because the source also changes; fixed in e7e13da.)

anandthakker · 2016-11-17T02:59:12Z

render-tests/runtime-styling/set-style-sprite/style.json

+                  "symbol-placement": "point",
+                  "icon-allow-overlap": true,
+                  "icon-ignore-placement": true,
+                  "icon-image": "restaurant-12"


Reason that expected.png looks right: the original style uses generic_icon from the emerald sprite set.

anandthakker · 2016-11-17T02:59:51Z

render-tests/runtime-styling/set-style-glyphs/style.json

+                  "text-field": "{name}",
+                  "text-font": [
+                    "NotoCJK"
+                  ],


Reason that expected.png looks right: the original style uses text-field: 'HELLO', rendered with Open Sans / Arial.

anandthakker · 2016-11-17T03:01:52Z

In response to a sudden bout of paranoia--and also hopefully to make the review a little easier--I added code comments in each of the new tests indicating why the expected.png makes sense. (A couple of them are 'outdated' because I was going commit-by-commit.)

I think this should now be ready for review.

anandthakker · 2016-11-20T17:05:19Z

@mollymerp didn't want to lose track of your Slack question re: reusing expected images for runtime-styling/set-style-xxx and runtime-styling/xxx tests. Can't see the logs, but to paraphrase my answer:

The bulk of the set-style-xxx tests are directly copied from the equivalent, pre-existing runtime-styling. In those cases, I only changed style.json to use setStyle instead of whatever individual operation was being tested (addLayer, setLayoutProperty, etc.). The expected.png images are identical.
A few tests are totally new, because they test setStyle-specific things like reordering layers, changing glyphs, etc. For those, I tried to leave a note in the diff explaining why the expected image makes sense.

So, there are lots of duplicated images, but I figured that, eventually, the non-set-style versions of the tests will just be removed entirely... so maybe the dupe-ing is okay, temporarily?

Anand Thakker added 5 commits November 15, 2016 17:16

Make set-style-xxx copy of each runtime-styling test

3154032

Translate mutator ops to setStyle ops

5bad0ac

Localize URLs in setStyle argument

8504642

Replace null value with omission

8681200

Ignore set-style runtime styling tests on native

ddba3f9

anandthakker assigned jfirebaugh Nov 16, 2016

anandthakker mentioned this pull request Nov 16, 2016

Smart setStyle (part 1) mapbox/mapbox-gl-js#3621

Merged

15 tasks

Add more set-style tests

9b15ef8

Anand Thakker added 4 commits November 16, 2016 16:26

Fix native ignores

e41eec7

Move regression test to regressions dir

9365b56

Add test for setStyle / change sprite URL

f14843b

Add test for setStyle / change glyphs URL

5501ff4

anandthakker mentioned this pull request Nov 17, 2016

Change [implementation]: false to skipped: { [implementation]: 'reason' } #177

Merged

Remove unnecessary icons from set-style-glyphs test

9e98ccf

anandthakker commented Nov 17, 2016

View reviewed changes

Don't change source in layer-ordering test

e7e13da

anandthakker commented Nov 17, 2016

View reviewed changes

jfirebaugh merged commit cfa20f8 into master Nov 22, 2016

jfirebaugh deleted the set-style-tests branch November 22, 2016 00:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add runtime-styling tests for 'smart setStyle' #170

Add runtime-styling tests for 'smart setStyle' #170

anandthakker commented Nov 16, 2016

anandthakker commented Nov 16, 2016

jfirebaugh commented Nov 16, 2016 •

edited

Loading

anandthakker commented Nov 16, 2016

anandthakker commented Nov 16, 2016

anandthakker commented Nov 16, 2016

jfirebaugh commented Nov 16, 2016

anandthakker commented Nov 16, 2016 •

edited

Loading

lucaswoj commented Nov 17, 2016

anandthakker commented Nov 17, 2016

jfirebaugh commented Nov 17, 2016

anandthakker commented Nov 17, 2016

anandthakker commented Nov 17, 2016 •

edited

Loading

anandthakker commented Nov 17, 2016

anandthakker commented Nov 17, 2016

anandthakker Nov 17, 2016

anandthakker Nov 17, 2016

anandthakker Nov 17, 2016

anandthakker Nov 17, 2016

anandthakker Nov 17, 2016

anandthakker Nov 17, 2016

anandthakker Nov 17, 2016

anandthakker commented Nov 17, 2016 •

edited

Loading

anandthakker commented Nov 20, 2016

Add runtime-styling tests for 'smart setStyle' #170

Add runtime-styling tests for 'smart setStyle' #170

Conversation

anandthakker commented Nov 16, 2016

anandthakker commented Nov 16, 2016

jfirebaugh commented Nov 16, 2016 • edited Loading

anandthakker commented Nov 16, 2016

anandthakker commented Nov 16, 2016

anandthakker commented Nov 16, 2016

jfirebaugh commented Nov 16, 2016

anandthakker commented Nov 16, 2016 • edited Loading

lucaswoj commented Nov 17, 2016

anandthakker commented Nov 17, 2016

jfirebaugh commented Nov 17, 2016

anandthakker commented Nov 17, 2016

anandthakker commented Nov 17, 2016 • edited Loading

anandthakker commented Nov 17, 2016

anandthakker commented Nov 17, 2016

anandthakker Nov 17, 2016

Choose a reason for hiding this comment

anandthakker Nov 17, 2016

Choose a reason for hiding this comment

anandthakker Nov 17, 2016

Choose a reason for hiding this comment

anandthakker Nov 17, 2016

Choose a reason for hiding this comment

anandthakker Nov 17, 2016

Choose a reason for hiding this comment

anandthakker Nov 17, 2016

Choose a reason for hiding this comment

anandthakker Nov 17, 2016

Choose a reason for hiding this comment

anandthakker commented Nov 17, 2016 • edited Loading

anandthakker commented Nov 20, 2016

jfirebaugh commented Nov 16, 2016 •

edited

Loading

anandthakker commented Nov 16, 2016 •

edited

Loading

anandthakker commented Nov 17, 2016 •

edited

Loading

anandthakker commented Nov 17, 2016 •

edited

Loading