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

Fix issue #4859: allow loading RTL text plugin before first style load. #4870

Merged
merged 3 commits into from
Jun 26, 2017

Conversation

ChrisLoer
Copy link
Contributor

@ChrisLoer ChrisLoer commented Jun 22, 2017

Fixes issue #4859: this was a regression I introduced in 171db61. Unfortunately I still haven't figured out a good way to exercise this code in our test suite, since it depends on running in the browser. :/

/cc @anandthakker @mollymerp

Copy link
Contributor

@mollymerp mollymerp left a comment

Choose a reason for hiding this comment

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

Hmmm would it be possible to unit test to ensure that the callback is called with argument of type Object with those properties?

@ChrisLoer
Copy link
Contributor Author

@mollymerp I had to hack things up a bit to do the stubbing (I'm not very experienced with Sinon), but I extracted the hard-to-test createBlobURL and put in a test that verifies a bogus pluginBlobURL gets passed down to the workers. Do you think that makes sense?

 - Test that loading a style after loading the plugin works
 - Test that pluginBlobURL makes it all the way to the workers
@mollymerp
Copy link
Contributor

Thanks for adding that test!
Would adding a calledWith check to make sure the correct arguments are passed to the test here https://github.com/mapbox/mapbox-gl-js/blob/master/test/unit/style/style.test.js#L59 have caught this specific regression? Sorry if this is a dumb question – I'm not too familiar with the RTL plugin 😬

@ChrisLoer
Copy link
Contributor Author

@mollymerp A calledWith check on the original test (https://github.com/mapbox/mapbox-gl-js/blob/master/test/unit/style/style.test.js#L59) wouldn't have worked originally because it didn't have a stubbed out createBlobURL (one confusing thing here is that the the URL we ultimately care about is the "blob URL", not the "plugin URL" that gets passed in to setRTLTextPlugin). I'll try adding a setRTLTextPlugin call to the test now that we've got the stubbing mechanism.

…gin instead of directly firing change signal.
@ChrisLoer
Copy link
Contributor Author

I had to add a clearRTLTextPlugin method for the tests to allow both tests to modify the plugin... global singletons don't play well with tests. :/

Copy link
Contributor

@mollymerp mollymerp left a comment

Choose a reason for hiding this comment

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

Thanks and sorry for the testing troubles!

@ChrisLoer ChrisLoer merged commit d2f27d6 into master Jun 26, 2017
@ChrisLoer ChrisLoer deleted the cloer_4859 branch June 26, 2017 18:48
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.

3 participants