Implement FixLocation.provider field based on CLLocationSourceInformation #3728
Conversation
| if #available(iOS 15.0, *) { | ||
| let sourceInfo = CLLocationSourceInformation( | ||
| softwareSimulationState: true, | ||
| andExternalAccessoryState: false | ||
| ) |
There was a problem hiding this comment.
Despite this #available macro, iOS 14.5 tests are failing on CI because Xcode 12.5.1 ships with an older iOS development SDK that doesn’t recognize this type yet:
/Applications/Xcode-12.5.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/swift-frontend -frontend -c -primary-file /Users/distiller/project/Tests/MapboxCoreNavigationTests/CLLocationTests.swift /Users/distiller/project/Tests/MapboxCoreNavigationTests/NavigationEventsManagerTests.swift /Users/distiller/project/Tests/MapboxCoreNavigationTests/MapboxCoreNavigationTests.swift /Users/distiller/project/Tests/MapboxCoreNavigationTests/UnimplementedLoggingTests.swift /Users/distiller/project/Tests/MapboxCoreNavigationTests/DateTests.swift /Users/distiller/project/Tests/MapboxCoreNavigationTests/NavigationLocationManagerTests.swift /Users/distiller/project/Tests/MapboxCoreNavigationTests/ReplayLocationManagerTests.swift /Users/distiller/project/Tests/MapboxCoreNavigationTests/TilesetDescriptorFactoryTests.swift /Users/distiller/project/Tests/MapboxCoreNavigationTests/DistanceFormatterTests.swift /Users/distiller/project/Tests/MapboxCoreNavigationTests/RouteProgressTests.swift /Users/distiller/project/Tests/MapboxCoreNavigationTests/PassiveLocationManagerTests.swift /Users/distiller/project/Tests/MapboxCoreNavigationTests/LocationTests.swift /Users/distiller/project/Tests/MapboxCoreNavigationTests/BillingHandlerTests.swift /Users/distiller/project/Tests/MapboxCoreNavigationTests/StringTests.swift /Users/distiller/project/Tests/MapboxCoreNavigationTests/RouteControllerTests.swift /Users/distiller/project/Tests/MapboxCoreNavigationTests/OptionsTests.swift /Users/distiller/project/Tests/MapboxCoreNavigationTests/Extensions/RouteOptionsTests.swift /Users/distiller/project/Tests/MapboxCoreNavigationTests/BundleAdditionsTests.swift /Users/distiller/project/Tests/MapboxCoreNavigationTests/LeakTests.swift /Users/distiller/project/Tests/MapboxCoreNavigationTests/SimulatedLocationManagerTests.swift /Users/distiller/project/Tests/MapboxCoreNavigationTests/TunnelAuthorityTests.swift /Users/distiller/project/Tests/MapboxCoreNavigationTests/Extensions/RouteStateTests.swift /Users/distiller/project/Tests/MapboxCoreNavigationTests/NavigationServiceTests.swift /Users/distiller/project/Tests/MapboxCoreNavigationTests/NativeHandlersFactoryTests.swift -emit-module-path /Users/distiller/Library/Developer/Xcode/DerivedData/MapboxNavigation-gtrukiuwvhbmctfcsthejciepfao/Build/Intermediates.noindex/MapboxNavigation.build/Debug-iphonesimulator/MapboxCoreNavigationTests.build/Objects-normal/x86_64/CLLocationTests\~partial.swiftmodule -emit-module-doc-path /Users/distiller/Library/Developer/Xcode/DerivedData/MapboxNavigation-gtrukiuwvhbmctfcsthejciepfao/Build/Intermediates.noindex/MapboxNavigation.build/Debug-iphonesimulator/MapboxCoreNavigationTests.build/Objects-normal/x86_64/CLLocationTests\~partial.swiftdoc -emit-module-source-info-path /Users/distiller/Library/Developer/Xcode/DerivedData/MapboxNavigation-gtrukiuwvhbmctfcsthejciepfao/Build/Intermediates.noindex/MapboxNavigation.build/Debug-iphonesimulator/MapboxCoreNavigationTests.build/Objects-normal/x86_64/CLLocationTests\~partial.swiftsourceinfo -serialize-diagnostics-path /Users/distiller/Library/Developer/Xcode/DerivedData/MapboxNavigation-gtrukiuwvhbmctfcsthejciepfao/Build/Intermediates.noindex/MapboxNavigation.build/Debug-iphonesimulator/MapboxCoreNavigationTests.build/Objects-normal/x86_64/CLLocationTests.dia -emit-dependencies-path /Users/distiller/Library/Developer/Xcode/DerivedData/MapboxNavigation-gtrukiuwvhbmctfcsthejciepfao/Build/Intermediates.noindex/MapboxNavigation.build/Debug-iphonesimulator/MapboxCoreNavigationTests.build/Objects-normal/x86_64/CLLocationTests.d -emit-reference-dependencies-path /Users/distiller/Library/Developer/Xcode/DerivedData/MapboxNavigation-gtrukiuwvhbmctfcsthejciepfao/Build/Intermediates.noindex/MapboxNavigation.build/Debug-iphonesimulator/MapboxCoreNavigationTests.build/Objects-normal/x86_64/CLLocationTests.swiftdeps -target x86_64-apple-ios11.0-simulator -enable-objc-interop -sdk /Applications/Xcode-12.5.1.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator14.5.sdk -I /Users/distiller/Library/Developer/Xcode/DerivedData/MapboxNavigation-gtrukiuwvhbmctfcsthejciepfao/Build/Products/Debug-iphonesimulator -I /Applications/Xcode-12.5.1.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/usr/lib -F /Users/distiller/Library/Developer/Xcode/DerivedData/MapboxNavigation-gtrukiuwvhbmctfcsthejciepfao/Build/Products/Debug-iphonesimulator -F /Applications/Xcode-12.5.1.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/Library/Frameworks -F /Applications/Xcode-12.5.1.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator14.5.sdk/Developer/Library/Frameworks -enable-testing -g -module-cache-path /Users/distiller/Library/Developer/Xcode/DerivedData/ModuleCache.noindex -swift-version 5 -enforce-exclusivity\=checked -Onone -D DEBUG -serialize-debugging-options -serialize-debugging-options -Xcc -working-directory -Xcc /Users/distiller/project -enable-anonymous-context-mangled-names -Xcc -I/Users/distiller/Library/Developer/Xcode/DerivedData/MapboxNavigation-gtrukiuwvhbmctfcsthejciepfao/Build/Intermediates.noindex/MapboxNavigation.build/Debug-iphonesimulator/MapboxCoreNavigationTests.build/swift-overrides.hmap -Xcc -iquote -Xcc /Users/distiller/Library/Developer/Xcode/DerivedData/MapboxNavigation-gtrukiuwvhbmctfcsthejciepfao/Build/Intermediates.noindex/MapboxNavigation.build/Debug-iphonesimulator/MapboxCoreNavigationTests.build/MapboxCoreNavigationTests-generated-files.hmap -Xcc -I/Users/distiller/Library/Developer/Xcode/DerivedData/MapboxNavigation-gtrukiuwvhbmctfcsthejciepfao/Build/Intermediates.noindex/MapboxNavigation.build/Debug-iphonesimulator/MapboxCoreNavigationTests.build/MapboxCoreNavigationTests-own-target-headers.hmap -Xcc -I/Users/distiller/Library/Developer/Xcode/DerivedData/MapboxNavigation-gtrukiuwvhbmctfcsthejciepfao/Build/Intermediates.noindex/MapboxNavigation.build/Debug-iphonesimulator/MapboxCoreNavigationTests.build/MapboxCoreNavigationTests-all-non-framework-target-headers.hmap -Xcc -ivfsoverlay -Xcc /Users/distiller/Library/Developer/Xcode/DerivedData/MapboxNavigation-gtrukiuwvhbmctfcsthejciepfao/Build/Intermediates.noindex/MapboxNavigation.build/Debug-iphonesimulator/MapboxCoreNavigationTests.build/all-product-headers.yaml -Xcc -iquote -Xcc /Users/distiller/Library/Developer/Xcode/DerivedData/MapboxNavigation-gtrukiuwvhbmctfcsthejciepfao/Build/Intermediates.noindex/MapboxNavigation.build/Debug-iphonesimulator/MapboxCoreNavigationTests.build/MapboxCoreNavigationTests-project-headers.hmap -Xcc -I/Users/distiller/Library/Developer/Xcode/DerivedData/MapboxNavigation-gtrukiuwvhbmctfcsthejciepfao/Build/Products/Debug-iphonesimulator/include -Xcc -I/Users/distiller/Library/Developer/Xcode/DerivedData/MapboxNavigation-gtrukiuwvhbmctfcsthejciepfao/Build/Intermediates.noindex/MapboxNavigation.build/Debug-iphonesimulator/MapboxCoreNavigationTests.build/DerivedSources-normal/x86_64 -Xcc -I/Users/distiller/Library/Developer/Xcode/DerivedData/MapboxNavigation-gtrukiuwvhbmctfcsthejciepfao/Build/Intermediates.noindex/MapboxNavigation.build/Debug-iphonesimulator/MapboxCoreNavigationTests.build/DerivedSources/x86_64 -Xcc -I/Users/distiller/Library/Developer/Xcode/DerivedData/MapboxNavigation-gtrukiuwvhbmctfcsthejciepfao/Build/Intermediates.noindex/MapboxNavigation.build/Debug-iphonesimulator/MapboxCoreNavigationTests.build/DerivedSources -Xcc -DDEBUG\=1 -target-sdk-version 14.5 -import-objc-header /Users/distiller/project/Tests/MapboxCoreNavigationTests/MapboxCoreNavigationTests-Bridging-Header.h -pch-output-dir /Users/distiller/Library/Developer/Xcode/DerivedData/MapboxNavigation-gtrukiuwvhbmctfcsthejciepfao/Build/Intermediates.noindex/PrecompiledHeaders -pch-disable-validation -module-name MapboxCoreNavigationTests -o /Users/distiller/Library/Developer/Xcode/DerivedData/MapboxNavigation-gtrukiuwvhbmctfcsthejciepfao/Build/Intermediates.noindex/MapboxNavigation.build/Debug-iphonesimulator/MapboxCoreNavigationTests.build/Objects-normal/x86_64/CLLocationTests.o -index-store-path /Users/distiller/Library/Developer/Xcode/DerivedData/MapboxNavigation-gtrukiuwvhbmctfcsthejciepfao/Index/DataStore -index-system-modules
/Users/distiller/project/Tests/MapboxCoreNavigationTests/CLLocationTests.swift:97:30: error: cannot find 'CLLocationSourceInformation' in scope
let sourceInfo = CLLocationSourceInformation(
^~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/distiller/project/Tests/MapboxCoreNavigationTests/CLLocationTests.swift:110:47: error: extra argument 'sourceInfo' in call
sourceInfo: sourceInfo)
^~~~~~~~~~
#available is evaluated at runtime. Wrap this block in an #if compiler(…) block to prevent older Xcodes from seeing it when compiling.
.circleci/config.yml
Outdated
| name: "swift test; Xcode 13.2.1; iOS 15.2" | ||
| xcode: "13.2.1" | ||
| iOS: "15.2" | ||
| device: "iPhone 12 Pro Max" |
There was a problem hiding this comment.
Several snapshot (screenshot) tests fail on this build:
InstructionsBannerViewSnapshotTests.testAbbreviateInstructions()
InstructionsBannerViewSnapshotTests.testAbbreviateInstructionsIncludingDelimiter()
InstructionsBannerViewSnapshotTests.testAbbreviateWestFremontAvenue()
InstructionsBannerViewSnapshotTests.testExitShields()
InstructionsBannerViewSnapshotTests.testGenericShieldAndExitViewWithCustomDayStyle()
InstructionsBannerViewSnapshotTests.testGenericShields()
InstructionsBannerViewSnapshotTests.testLongDistance()
InstructionsBannerViewSnapshotTests.testMultilinePrimary()
InstructionsBannerViewSnapshotTests.testPrimaryShieldAndSecondary()
InstructionsBannerViewSnapshotTests.testSinglelinePrimaryAndSecondary()
InstructionsBannerViewSnapshotTests.testSweEngLongDistance()
InstructionsBannerViewSnapshotTests.testUkrainianLongDistance()
Font metrics may have changed subtly between minor versions of iOS. You can either SSH into the build machine or run the tests locally to compare the screenshots visually. If the output is acceptable, we can check in the new screenshots for iOS 15.2 specifically.
There was a problem hiding this comment.
If the output is acceptable, we can check in the new screenshots for iOS 15.2 specifically.
I.e. we can have separate screenshots for different SDK versions? How can we do that?
There was a problem hiding this comment.
Oh, it looks like we’ve only retained the ability to vary it by iPhone model rather than iOS version. That probably changed for v2.0.0 when we migrated to a different snapshot testing framework. The first step would be to diff the snapshots to see if they’re visually alike and if the difference is even caused by the OS upgrade versus some other kind of failure.
There was a problem hiding this comment.
I only can notice that this red line(not sure what is it) is a bit bolder:
There was a problem hiding this comment.
Btw does this snapshotting framework generate diff images? I remember I used something like this in the past.
There was a problem hiding this comment.
This is a very subtle difference. Ideally, we would decrease the sensitivity of the image comparison. We used to do this with the old snapshot testing frameworks before #3043, but currently we’d be blocked on an upstream fix like pointfreeco/swift-snapshot-testing#571.
For now, adding the OS version makes sense, but can we make it an optional fallback, so that we don’t have to update the snapshot tests every time we upgrade CI to a minor version? Either:
- Preflight a check if the unqualified file exists before calling
verifySnapshot(matching:as:named:record:snapshotDirectory:timeout:file:testName:line:). - Catch the error about a missing file and call this function with the modified file URL.
/cc @S2Ler
There was a problem hiding this comment.
@1ec5 well, but it is not clear how to record snapshots in that case. Imagine you run snapshot tests in record mode on iOS 15 and what would we do? Just overwrite files in "default" folder? And then manually move updated files to dedicated folder?
so that we don’t have to update the snapshot tests every time we upgrade CI to a minor version
Btw I remember from my past experience that such issues are usually caused only by major iOS version upgrade, should we distinguish only by major version then?
There was a problem hiding this comment.
Current screenshots are slightly outdated, for example, the red line width is now 0.33 pt after the recent changes, but on the screenshots that we use as a source of truth, the width is still 1pt. Looks like with other minor UI changes in iOS 15 it's now enough to break our tests, I'll take a look.
| // in some scenarios we store this information to history files, so to save space there, we use "short" names and 1/0 instead of true/false | ||
| let isSimulated = sourceInformation.isSimulatedBySoftware ? 1 : 0 | ||
| let isProducedByAccessory = sourceInformation.isProducedByAccessory ? 1 : 0 | ||
|
|
||
| provider = "sim:\(isSimulated),acc:\(isProducedByAccessory)" |
There was a problem hiding this comment.
Perhaps MapboxNavigationNative could be modified to accept something more structured like a dictionary instead of a freeform, user agent–like string, so that it can format and compress it as appropriate.
There was a problem hiding this comment.
I'm not sure we should add something iOS-specific - there is no analog of this structure on other platforms. Since this value will be used as kind of debug information I think it is okay to have it in this provider field in a free form. Btw initially I implemented it as a part of FixLocation.extras field, which allows to add any additional information to locations, but then decided that provider is exactly that field that was supposed to use for such use case(i.e. point to the source of location). WDYT?
There was a problem hiding this comment.
I meant a JSON-like dictionary of strings, similar to that extras field. But if you’re sure we won’t ever need additional structure beyond this format, then we can go with this more opaque string.
There was a problem hiding this comment.
I don't think we will need JSON here, I'd prefer to save bytes in history here rather than adding JSON overhead, because we likely need this data for debugging only.
1ec5
added
Core
|
No breaking changes detected in MapboxCoreNavigation |
|
No breaking changes detected in MapboxNavigation |
SiarheiFedartsou
force-pushed
the
sf-fix-location-provider
branch
from
a4b4cf7 to
d74cd94
Compare
|
No breaking changes detected in MapboxCoreNavigation |
|
No breaking changes detected in MapboxNavigation |
SiarheiFedartsou
force-pushed
the
sf-fix-location-provider
branch
2 times, most recently
from
bc4544a to
aa9056c
Compare
|
@1ec5 could you take a look please? |
Breaking Changes in MapboxCoreNavigationBreaking API Changes
|
|
No breaking changes detected in MapboxNavigation |
|
Should |
I'd prefer to preserve original values provided by system. More than it IMO we should remove these simulations because it may go in conflict with NNs simulation and we never know what it can lead to. |
We can discuss this change separately to avoid scope-creeping your PR. For what it’s worth, the same mechanism is also used for always simulating a route from beginning to end, useful for armchair testing and demos. Maybe we can make that |
We can, but this PR "as is" would allow us to already benefit from this additional field and this can be improved afterwards. |
SiarheiFedartsou
changed the title
SiarheiFedartsou
force-pushed
the
sf-fix-location-provider
branch
from
aa9056c to
3e1b4ff
Compare
|
No breaking changes detected in MapboxCoreNavigation |
|
No breaking changes detected in MapboxNavigation |
Description
Here we add encoding of
CLLocation.sourceInformationwhich was introduced in iOS 15 toFixLocation.providerfield and allows to better understand nature of the signal. It would provide us additional information which can be useful during debug.