Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Adds support for running Mapbox-gl-js expressions tests against the Objective-C expressions interface #11435

Closed
wants to merge 10 commits into from

Conversation

akitchen
Copy link
Contributor

This branch enables running the expressions tests from Mapbox-gl-js against the equivalent cocoa API

Note that this is currently only set up to support iOS, but macOS support shouldn't require much add'l effort.

"Do not merge" is checked as we have quite a few test failures relating to tail work and missing functionality, as well as a few crashing tests which have been blacklisted in the code

/cc @1ec5 @anandthakker @fabian-guerra

@akitchen akitchen added GL JS parity For feature parity with Mapbox GL JS iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS tests ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Mar 12, 2018
@akitchen akitchen added this to the ios-v4.0.0 milestone Mar 12, 2018
@akitchen akitchen changed the title Adds support for running expressions tests from Mapbox-gl-js Adds support for running Mapbox-gl-js expressions tests against the Objective-C expressions interface Mar 15, 2018
@akitchen
Copy link
Contributor Author

I think it would be helpful for @mapbox/maps-ios to start collecting a list of tests which fail, whether they crash, raise unexpected exceptions or fail expectations, so that we can catalog the list of differences between gl-js for the 4.0 release and beyond and prioritize any additional work we might not currently be aware of.

As the team is working on tickets related to expressions, please consider merging this branch into your branches and running these tests for additional feedback.

/cc @lilykaiser


NSSet *testsToSkip = [NSSet setWithObjects:
@"to-boolean",
@"concat/arity-1",
Copy link
Contributor

Choose a reason for hiding this comment

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

#11472 will make it possible to express a concatenation expression with an arity of 1 within an NSExpression. This is the case for concat but not, say, +, because concat takes a series of arguments, whereas mgl_join: would take a single argument containing all the string expressions to concatenate. By contrast, add:to: takes two arguments.

On the subject of arity, it’s worth pointing out that built-in NSExpression functions like + are strictly binary, such that 1 + 2 + 3 is interpreted as (1 + 2) + 3. Currently, ["+", 1, 2, 3] is converted to sum({1, 2, 3}), which I think is reasonable but not a literal round trip.

@friedbunny
Copy link
Contributor

friedbunny commented Mar 22, 2018

I’ve rebased this and cleaned it up. Tests now complete successfully with 17 passing and 196 skipped... which is a lot and makes me question how useful these will be in the long run.

I didn’t make an effort to fix any of these failures, some of which are intentional or don’t account for the design of the JS tests. Reasons for skipping tests (or families of tests) are included as comments.

@akitchen
Copy link
Contributor Author

I’ve rebased this and cleaned it up. Tests now complete successfully with 17 passing and 196 skipped... which is a lot and makes me question how useful these will be in the long run.

Thanks for doing this -- my original intention for the blacklist was to only contain crashers, and the rest we could catalog automatically somehow. I wonder if more effort should be spent scrutinizing the list of runtime failures. There's no rush to merge this, especially if it's not yet super useful.

@friedbunny
Copy link
Contributor

I wonder if more effort should be spent scrutinizing the list of runtime failures.

Definitely — hopefully this manual catalog will be useful in whittling down the list of failures, now that there’s a rough classification.

@1ec5
Copy link
Contributor

1ec5 commented Mar 30, 2018

#11472 introduced a catch-all MGL_FUNCTION() function that can represent any JSON expression. Any expression that doesn’t enjoy custom function support (whether via a conventional function or an aftermarket function) should roundtrip to the extent that mbgl::expression::Expression::serialize() does. Hopefully that means we can enable more of these tests.

@1ec5 1ec5 modified the milestones: ios-v4.0.0, ios-v4.0.1 Apr 18, 2018
@akitchen akitchen force-pushed the fb-expression-tests-from-js branch from e2c7069 to cc510e3 Compare April 27, 2018 15:42
@akitchen
Copy link
Contributor Author

I've rebased this on release-boba and made some adjustments to the list of failures.

This should make it easier for @mapbox/maps-ios to integrate these tests with other branches for ongoing evaluation.

@1ec5 1ec5 modified the milestones: ios-v4.0.1, ios-v4.1.0 May 8, 2018
@1ec5 1ec5 modified the milestones: ios-v4.1.0, ios-v4.2.0 Jun 20, 2018
@1ec5 1ec5 modified the milestones: ios-v4.2.0, ios-v4.3.0 Jul 18, 2018
@stale
Copy link

stale bot commented Oct 24, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the archived Archived because of inactivity label Oct 24, 2018
@stale
Copy link

stale bot commented Oct 27, 2018

This pull request has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions.

@stale stale bot closed this Oct 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived Archived because of inactivity ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold GL JS parity For feature parity with Mapbox GL JS iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants