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

Enable map recycling option when running render tests in GL JS #5092

Closed
wants to merge 3 commits into from

Conversation

brunoabinader
Copy link
Member

Enables map recycling option when running render tests in GL JS.

Depends on #5091 to be merged first.

This PR:

  • Causes the load event to be fired again when setting a style after a previous set style.
  • Enables map recycling in GL JS render tests via options.recycleMap.

@brunoabinader brunoabinader changed the title Test suite recyclemap js impl Enable map recycling option when running render tests in GL JS Aug 3, 2017
@jfirebaugh
Copy link
Contributor

Changing the semantics of the load event would be a significant disruption for existing code; let's find another way to accomplish this.

@brunoabinader
Copy link
Member Author

Changing the semantics of the load event would be a significant disruption for existing code; let's find another way to accomplish this.

@anandthakker suggested we could listen to the 'data' event instead, and check the event payload for e.dataType === 'style'.

@anandthakker
Copy link
Contributor

^ Yep, that should work in the {diff: true} case, but note that if you do map.setStyle(newStyle) without diff: true, that event would only be fired if the diff-and-patch operation fails and we have to fall back to wholesale replacement of the style.

package.json Outdated
"test-suite-clean": "find test/integration/*-tests -mindepth 2 -type d -not \\( -exec test -e \"{}/style.json\" \\; \\) -print | xargs -t rm -r",
"test-unit": "tap --reporter dot --no-coverage test/unit",
"test-render": "node --max-old-space-size=2048 test/render.test.js",
"test-render-recycle-map": "node --max-old-space-size=2048 test/render.test.js --recycle-map",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think yarn test-render --recycle-map should work, which would make this unnecessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

This works, but I had to append -- before the script arguments:
yarn test-render -- --recycle-map

circle.yml Outdated
@@ -38,6 +38,12 @@ workflows:
filters:
tags:
only: /.*/
- test-render-recyrcle-map:
Copy link
Contributor

Choose a reason for hiding this comment

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

recyrcle-map looks like a typo

@springmeyer
Copy link
Contributor

Is this ready to merge?

@brunoabinader
Copy link
Member Author

Is this ready to merge?

This is stale - requires rebase + logic update to avoid changing load event semantics, as suggested by @jfirebaugh.

@jfirebaugh
Copy link
Contributor

Closing here but without prejudice towards updating with the needed changes and resubmitting -- would be nice to have this.

@jfirebaugh jfirebaugh closed this Jun 26, 2018
@jfirebaugh jfirebaugh deleted the test-suite-recyclemap-js-impl branch June 26, 2018 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants