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

Refactor NSExpression unit tests #421

Open
birkskyum opened this issue Sep 3, 2022 · 10 comments
Open

Refactor NSExpression unit tests #421

birkskyum opened this issue Sep 3, 2022 · 10 comments
Labels

Comments

@birkskyum
Copy link
Member

birkskyum commented Sep 3, 2022

Remove all XCTExpectFailure from macos/ios/darwin test suites introduced in #411 by refactoring the tests to the new format. Related to #331

@birkskyum
Copy link
Member Author

birkskyum commented Sep 3, 2022

@birkskyum
Copy link
Member Author

birkskyum commented Sep 4, 2022

@roblabs , do you know how to write

[NSExpression expressionWithFormat:@"MGL_FUNCTION('image', 'Icon Image')"];

With the new syntax?

@roblabs
Copy link
Collaborator

roblabs commented Sep 5, 2022

@birkskyum — the expression MGL_FUNCTION is curious and still needs attention.

While I was researching the source for #331 & PR #411, I found documentation for the expression in "Predicates and Expressions.md":

https://maplibre.org/maplibre-gl-native/ios/api/predicates-and-expressions.html#code-mgl_function-code


But the Objective-C source mentions that it is not implemented.

https://github.com/maplibre/maplibre-gl-native/blob/main/platform/ios/platform/darwin/src/NSExpression+MGLAdditions.mm#L256,L259

- (id)MGL_FUNCTION:(id)firstArgument, ... {
    [NSException raise:NSInvalidArgumentException
                format:@"Mapbox GL function expressions lack underlying Objective-C implementations."];
    return nil;
}

In general, I found that writing the expressions in Swift first was easier to sort out than in Objective-C.

@louwers
Copy link
Collaborator

louwers commented Feb 17, 2023

The MapLibre GL JS update is blocked by this because the test files are generated. #816

@birkskyum
Copy link
Member Author

I found that some of these tests only can be enabled again by porting them to swift. The swift syntax is more flexible and we have cases that aren't supported by obj-c anymore.

@birkskyum
Copy link
Member Author

Because of that, I don't plan to look further into refactoring the obj-c code as it seemed futile.

@louwers
Copy link
Collaborator

louwers commented Feb 17, 2023

OK thanks for the update.

@1ec5
Copy link
Contributor

1ec5 commented Feb 17, 2023

But the Objective-C source mentions that it is not implemented.

This just tells you that it’s an abstract class.

MGL_FUNCTION is a generic aftermarket expression that developers can use for any style-spec function that isn’t specifically implemented yet as an aftermarket expression.

/ref mapbox/mapbox-gl-native#11472

@1ec5
Copy link
Contributor

1ec5 commented Feb 17, 2023

The swift syntax is more flexible and we have cases that aren't supported by obj-c anymore.

There isn’t a separate Swift syntax. You may be referring to the “MGL” convenience initializers on NSExpression, which are also available in Objective-C. These initializers are more type-safe but not as flexible as anything that uses an expression function.

The tests were originally intended to catch the situation where the longstanding trick to installing an aftermarket function ceased to be supported on iOS or macOS. If that has finally happened, then some code can switch to the initializers as in the examples in #411, but more dynamic code needs a different solution.

As noted in #331 (comment), the initializers may not work in some cases, because they’re converted to the aftermarket functions under the hood anyways.

@louwers
Copy link
Collaborator

louwers commented Feb 22, 2023

The unit tests are generated so should not be edited manually.

Instead the template that generates them should be fixed: platform/ios/platform/darwin/test/MGLStyleLayerTests.mm.ejs platform/ios/platform/darwin/test/MGLBackgroundStyleLayerTests.mm

@louwers louwers removed the ios-15.5 label Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants