mapbox / mapbox-gl-test-suite

Rendering tests for Mapbox GL
6 stars 12 forks source link

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

Closed anandthakker closed 7 years ago

anandthakker commented 7 years ago

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

anandthakker commented 7 years ago

Ready for 👀 @jfirebaugh @lucaswoj

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 commented 7 years ago

Looks good! Can you also add tests for:

anandthakker commented 7 years ago

👍 ack--yeah, good call.

anandthakker commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

anandthakker commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

Ohh, nice--that would be simpler.

jfirebaugh commented 7 years ago

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 commented 7 years ago

Update:

anandthakker commented 7 years ago

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 commented 7 years ago

fail

anandthakker commented 7 years ago

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

anandthakker commented 7 years ago

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 commented 7 years ago

@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:

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?