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

Add expression tests #476

Merged
merged 6 commits into from
Jan 7, 2024
Merged

Add expression tests #476

merged 6 commits into from
Jan 7, 2024

Conversation

HarelM
Copy link
Collaborator

@HarelM HarelM commented Jan 6, 2024

Launch Checklist

Resolves #95

This adds the expression tests to this repo, as the code related to their functionality is also here.
I've also made some minor modification to the test execution code to be more readable.
After this is merged, I'll remove the tests and their files from the maplibre-gl-js repo.

The issue that prompted it is the following:

As when I tried to fix it I found out the code and the tests are residing in different repositories.

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Link to related issues.
  • Write tests for all new functionality.
  • Add an entry to CHANGELOG.md under the ## main section.

@codecov-commenter
Copy link

codecov-commenter commented Jan 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7a2b256) 78.06% compared to head (929975f) 92.09%.

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #476       +/-   ##
===========================================
+ Coverage   78.06%   92.09%   +14.03%     
===========================================
  Files         101      100        -1     
  Lines        4149     4176       +27     
  Branches     1185     1197       +12     
===========================================
+ Hits         3239     3846      +607     
+ Misses        910      330      -580     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@HarelM HarelM requested review from birkskyum and louwers January 6, 2024 21:43
@HarelM
Copy link
Collaborator Author

HarelM commented Jan 6, 2024

When reviewing, the only interesting files are the non json files, mainly:
test/integration/style-spec/validate_spec.test.ts
test/integration/expression/expression.test.ts
package.json and the files added to the lib folder in expression.
I've also removed some files that were only used in the tests.

@louwers
Copy link
Collaborator

louwers commented Jan 6, 2024

I didn't know the MapLibre GL JS expression parser lives in this repo.

As when I tried to fix it I found out the code and the tests are residing in different repositories.

This sounds bad, and the solution to move the tests here sounds like a good fix, but maybe we should also think about the architecture supporting parity between GL JS and Native a bit more. For example, should the expression related code from MapLibre Native also move to this repo? I don't think that is desirable.

A monorepo would be a pretty clean solution, but GitHub does not support it well (the issues and PRs are not separated enough and a single repo has only a single landing page). We are running into that with Native, where developers with vastly different backgrounds are put off by the C++ monster lurking beneath the Android / Qt / iOS platform interfaces that they are familiar with.

Just thinking out loud, but maybe manually copying these tests over to the Native repo (as we do right now) is an OK solution for now... It is better than moving C++ code out of the Native repo in here or having to open two PRs for a single fix.

@HarelM
Copy link
Collaborator Author

HarelM commented Jan 6, 2024

This is a big question which we have yet to find a good solution for.
The main reason that there is code here is that there's a npm package that this repo serves that is related to the style spec, that allows validating it. It makes sense to have it in a different repo as it guards from using stuff from the other repo.
The expression tests definition is cross platform I believe, not sure how to solve the sharing part of it though, and also there might be issues with parity when considering the expected results.
In any case, I don't have a good solution, we can talk about it in the next TSC maybe...

@birkskyum
Copy link
Member

regarding parity between gl js / native, it's still not super clear for me what to do with the .ts code, but moving the json expression tests as done in this PR seems like a natural step in the right direction - and a bit closer to having native also consuming them from here instead of holding a copy.

@HarelM HarelM merged commit 86c4898 into main Jan 7, 2024
6 checks passed
@HarelM HarelM deleted the add-expression-tests branch January 7, 2024 07:36
@HarelM
Copy link
Collaborator Author

HarelM commented Jan 7, 2024

Also @louwers the coverage here is now 92%, I'm totally kicking your butt :-P

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.

Move Expression Tests to this repo
4 participants