-
Notifications
You must be signed in to change notification settings - Fork 90
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
Fix support for Xcode 9 -> 10.2 #363
Conversation
1b836b0
to
e6fb27a
Compare
e6fb27a
to
943311b
Compare
ea249bc
to
b27503c
Compare
b27503c
to
b8e0c86
Compare
@@ -60,7 +60,7 @@ class V5Tests: XCTestCase { | |||
} | |||
XCTAssertNotNil(task) | |||
|
|||
waitForExpectations(timeout: 2) { (error) in | |||
waitForExpectations(timeout: 5) { (error) in |
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.
Even locally, 2 seconds did timeout 1/16 times for the 3.2 MB GeoJSON
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.
Yikes, somehow I didn’t realize our test fixtures were that big. This isn’t a performance test, so we should probably think about breaking this test case up into minimal test cases the next time we need to fix it.
21ef2c6
to
639dda0
Compare
639dda0
to
021348b
Compare
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.
Suggested changelog entry:
- Fixed compatibility issues with Xcode 10.2 when the SDK is installed using Carthage. (#363)
@@ -1,2 +1,2 @@ | |||
binary "https://www.mapbox.com/ios-sdk/Mapbox-iOS-SDK.json" ~> 4.4 | |||
github "AliSoftware/OHHTTPStubs" ~> 6.0 | |||
github "linksmt/OHHTTPStubs" "563f48d3fab84ef04639649c770b00f4fa502cca" |
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.
Is there a PR for this change or something else we can keep track of for when master gets updated? AliSoftware/OHHTTPStubs#295 is already in v7.0.0; is this for something else?
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.
Oh, looks like this commit also pulls in AliSoftware/OHHTTPStubs#286. The CircleCI configuration isn’t testing on watchOS anyways. So I think we can pin to the main pod’s v7.x.
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.
Yea, the reason why I switched to the fork here is the missing watchOS scheme in the original. We don't test on watchOS but we do compile for watchOS. We could switch back but it would require a workaround to run carthage bootstrap and ignore the .private and .resolved files.
@@ -60,7 +60,7 @@ class V5Tests: XCTestCase { | |||
} | |||
XCTAssertNotNil(task) | |||
|
|||
waitForExpectations(timeout: 2) { (error) in | |||
waitForExpectations(timeout: 5) { (error) in |
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.
Yikes, somehow I didn’t realize our test fixtures were that big. This isn’t a performance test, so we should probably think about breaking this test case up into minimal test cases the next time we need to fix it.
Upgraded to CircleCI 2.1
Running tests on
Xcode 9.4.1 iOS 9.3
Xcode 10.0 iOS 12.0
Xcode 10.2 iOS 12.2
Despite the branch name, this PR doesn't migrate to Swift 5, it merely validates compatibility with various versions of Xcode.
cc @1ec5