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

Migrate expression tests #97

Closed
wants to merge 5 commits into from
Closed

Migrate expression tests #97

wants to merge 5 commits into from

Conversation

birkskyum
Copy link
Member

@birkskyum birkskyum commented Apr 6, 2023

The 358 json expression tests are used in GL Native as well, so it's best they live in this shared repo.

In order to run the integration tests from GL JS, the following geo classes are moved and exported from here:

MercatorCoordinate,
CanonicalTileID,
LngLat,
LngLatLike,

The expression test 'feature_filter' has some geo queries like 'within' so that explains the need for those.

I'm quite sure the .json belongs here, but there are two ways to go about the integration tests.

  1. We move the test for each implementations (JS, Native) here, so we can evolve them with the spec, like typescript impl in this case.

  2. We have just the JSON here, and let each implementation import those and run the test like how Native has done so far, but similar for JS. I we go with this, those geo classes might not belong here for good as it would be spec-only.

@birkskyum birkskyum marked this pull request as draft April 6, 2023 23:00
@birkskyum
Copy link
Member Author

@louwers, @ntadej do you have some thoughts on this topic ?

@HarelM
Copy link
Collaborator

HarelM commented Apr 7, 2023

I honestly don't know how I feel about moving these classes here...
I think the json files should be here as they define the tests. The code that does the evaluation also is here, so that doesn't leave a lot of options, doesn't it?

@birkskyum
Copy link
Member Author

birkskyum commented Apr 11, 2023

I guess it does limited the options quite a lot of we want to have the tests close to at least one implementation, but I'm leaning more and more towards only having the generic .json tests here, and have each implementation (JS/Native) be responsible for performing their respective integration tests.

@birkskyum
Copy link
Member Author

Currently, as a consequence of removing the maplibre-gl-js submodule, both the v8.json, and all the 300 expression test .json files are copied into the MapLibre Native repo. It would be great if GL JS and Native could share these.

@birkskyum birkskyum requested a review from louwers April 12, 2023 16:41
@ChrisLoer
Copy link
Contributor

@birkskyum With #146 just merged into main, I think the interpolate-hcl and interpolate-lab tests will need to be updated with new values, since the white point we use in those color spaces has changed to new values. They should be small shifts -- in the unit tests (e.g. interpolate.test.ts) we just accepted the new values, while visually inspecting the results with @kajkal 's comparison demo at https://kajkal.github.io/maplibre-gl-style-spec-color-demo/swipe.html

@birkskyum
Copy link
Member Author

Let's start by moving just the tests here, release them as artifacts, download and execute them near the respective renderers expression implementations. This is the approach Native takes and GL JS might as well do the same.

@birkskyum birkskyum closed this May 20, 2023
@HarelM
Copy link
Collaborator

HarelM commented Oct 19, 2023

@birkskyum can I delete the linked branch or is it still needed?

@birkskyum
Copy link
Member Author

feel free to delete it.

@HarelM HarelM deleted the migrate-expression-tests branch October 19, 2023 20:13
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