-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Conversation
Replaced the style layer integration tests with unit tests. Test conversion of style values to property values and vice versa, not just round-tripping. Test the initial state and null-resetting of each null-resettable property. Test NSValue additions for style attribute enumerations. Test properties common to all style layer classes. Test MGLStyle’s source and layer collections. Eviscerated implementations of unavailable style layer properties corresponding to style specification properties that were renamed. Implemented corresponding getters to prevent ivars from being autosynthesized for these unavailable properties. Added a missing bridging header to the iOS test project.
@1ec5, thanks for your PR! By analyzing this pull request, we identified @boundsj, @jfirebaugh and @incanus to be potential reviewers. |
I put this issue on the v3.4.0 milestone because it doesn’t touch any shipping code (other than some dead code the compiler makes us put in) but makes us more confident about the shipping code (for example, leading me to find #7704). These tests can just as easily go into v3.4.1, however. |
macOS SDK test coverage is at 29.16% overall, up from 27%: Relevant files compared to #7548 (comment):
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great!!! In the interest of time, I focused on background and fill layer in my review. My only high level comment is that the MGLRuntimeStylingHelper
function factory used to make the tests a bit more readable. Now you have to understand more about the mbgl implementation to make sense of the constant and property tests. I think that is fine but we might consider streamlining this in future refactors.
MGLRuntimeStylingHelper did make the tests more readable. However, I decided to replace it with more codegen so that |
|
||
@interface NSValue (MGLStyleLayerTestAdditions) | ||
|
||
+ (instancetype)valueWithMGLVector:(CGVector)vector; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
macOS lacks +[NSValue valueWithCGVector:]
and NSValue.cgVectorValue
for some reason. It’s possible to call +[NSValue value:withObjCType:]
and -[NSValue getValue:]
instead, but that approach requires more than one statement at a time, so it doesn’t fit neatly into the assertion statements.
I’m considering upstreaming this category to the SDK proper, using +[NSValue valueWithMGLCoordinate:]
and NSValue.mglCoordinateValue
as a precedent.
Replaced the style layer integration tests with unit tests. Until #7664, we weren’t really testing integration in these style layer test classes, anyways, since the added layers were getting blown away as soon as the style finished loading.
Test conversion of style values to property values and vice versa, not just round-tripping. Test the initial state and null-resetting of each null-resettable property. Test NSValue additions for style attribute enumerations. Test properties common to all style layer classes. Test MGLStyle’s source and layer collections.
Eviscerated implementations of unavailable style layer properties corresponding to style specification properties that were renamed. (This improves test coverage; this code can never run anyways.) Implemented corresponding getters to prevent ivars from being autosynthesized for these unavailable properties.
/cc @boundsj @friedbunny @frederoni