Refactor project and add support for SPM #250
Conversation
frederoni
force-pushed
the
fred/spm
branch
8 times, most recently
from
9a1e7e9
to
1b2e366
Compare
Codecov Report
@@ Coverage Diff @@
## master #250 +/- ##
==========================================
- Coverage 88.45% 88.27% -0.19%
==========================================
Files 79 78 -1
Lines 5448 5414 -34
==========================================
- Hits 4819 4779 -40
- Misses 629 635 +6 |
frederoni
force-pushed
the
fred/spm
branch
2 times, most recently
from
895d400
to
0e769a5
Compare
| @@ -42,3 +46,10 @@ jobs: | |||
| command: | | |||
| if [ "<< parameters.xcode >>" = "12.0.0" ]; then export XCODE_XCCONFIG_FILE=~/project/xcode-12-fix.xcconfig; fi | |||
| Tests/Integration/test_carthage.sh | |||
| - when: | |||
| condition: | |||
| equal: [ "11.7.0", << parameters.xcode >> ] | |||
There was a problem hiding this comment.
Is there any problem with XCode 12 code coverage?
There was a problem hiding this comment.
Not that I know of but since Xcode 12 is still in beta and we run the same tests as with Xcode 11, I figured merging multiple reports would be excessive at this point.
| @@ -21,7 +21,7 @@ Pod::Spec.new do |s| | |||
|
|
|||
| # ――― Platform Specifics ――――――――――――――――――――――――――――――――――――――――――――――――――――――― # | |||
|
|
|||
| s.ios.deployment_target = "8.0" | |||
| s.ios.deployment_target = "9.0" | |||
There was a problem hiding this comment.
I wonder if we should have bump of minimum deployment target in the separate PR/commit?
Package.swift
Outdated
| let package = Package( | ||
| name: "MapboxMobileEvents", | ||
| platforms: [ | ||
| .iOS(.v10) |
There was a problem hiding this comment.
Does it matter which version of iOS we set here?
There was a problem hiding this comment.
Yes, good catch. Lowered to v9.
| @@ -95,6 +95,7 @@ - (void) test004_ShortUpdateInterval { | |||
| XCTAssertNil(configError); | |||
| } | |||
|
|
|||
| #if !SWIFT_PACKAGE | |||
There was a problem hiding this comment.
What is the problem with that test? Worth adding a comment?
There was a problem hiding this comment.
Support for resources was added in Swift 5.3 but we currently only require 5.2. 5.2 is included in Xcode 11 and 5.3 in 12.
| @@ -83,7 +83,8 @@ -(void)test002_checkCOMHashCount { | |||
| NSArray *comHashes = NSUserDefaults.mme_configuration.mme_certificatePinningConfig[@"events.mapbox.com"]; | |||
| XCTAssert(comHashes.count == 4); | |||
| } | |||
|
|
|||
|
|
|||
| #if !SWIFT_PACKAGE | |||
There was a problem hiding this comment.
the same: should we add a comment on why we are commenting out?
| @@ -75,6 +75,7 @@ - (void)testPausesWithWhenInUseAuthAndBackgrounded { | |||
| XCTAssert(self.eventsManager.paused == YES); | |||
| } | |||
|
|
|||
| #if !SWIFT_PACKAGE | |||
| command: | | ||
| # xcodebuild will try to use the Package manifest if it cannot find a project | ||
| mkdir temp && mv MapboxMobileEvents.xcodeproj temp | ||
| xcodebuild -scheme MapboxMobileEvents test -destination "platform=iOS Simulator,name=iPhone 11,OS=latest" |
There was a problem hiding this comment.
Does it make sense to specify the platform value through the build parameter?
There was a problem hiding this comment.
Yes, as a follow-up because then it should also propagate to CocoaPods and Carthage tests.
I also cleaned up excessive schemes in the main project to speed up carthage build process
Refactored the project to address the following issues
MapboxMobileEventsto avoid confusion and speed up Carthage builds by a factor of 3xThe SPM setup is running all XCTests
MapboxMobileEvents.xcodeproj is running all Cedar and XCTests
cc @alfwatt @nagineni @mr1sunshine