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

testGeoJSON() fails intermittently (probably slow decoding) #395

Closed
1ec5 opened this issue Dec 21, 2019 · 3 comments
Closed

testGeoJSON() fails intermittently (probably slow decoding) #395

1ec5 opened this issue Dec 21, 2019 · 3 comments
Milestone

Comments

@1ec5
Copy link
Contributor

1ec5 commented Dec 21, 2019

V5Tests.testGeoJSON() has been failing occasionally for months, but ever since #382 landed, it has been failing much more frequently:

Test Case '-[MapboxDirectionsTests.V5Tests testGeoJSON]' started.
/Users/distiller/project/Tests/MapboxDirectionsTests/V5Tests.swift:64: error: -[MapboxDirectionsTests.V5Tests testGeoJSON] : Asynchronous wait failed: Exceeded timeout of 5 seconds, with unfulfilled expectations: "calculating directions should return results".
/Users/distiller/project/Tests/MapboxDirectionsTests/V5Tests.swift:65: error: -[MapboxDirectionsTests.V5Tests testGeoJSON] : XCTAssertNil failed: "Error Domain=com.apple.XCTestErrorDomain Code=0 "(null)"" - Error: Error Domain=com.apple.XCTestErrorDomain Code=0 "(null)"
/Users/distiller/project/Tests/MapboxDirectionsTests/V5Tests.swift:73: error: -[MapboxDirectionsTests.V5Tests testGeoJSON] : XCTAssertNotNil failed
Test Case '-[MapboxDirectionsTests.V5Tests testGeoJSON]' failed (5.306 seconds).

This is most likely a symptom of the Codable performance issue raised in #221 (comment). The issue lies upstream in Turf, which this library relies on to decode LineStrings, and technically further upstream in the Swift standard library.

This isn’t necessarily a showstopper, since the navigation SDK uses Polyline encoding by default instead of GeoJSON. However, we should find a way to get performance back to within the same ballpark as it was before adopting Codable. If it’s already in the same ballpark, then we should increase the timeout and also add a performance unit test to make any runtime changes more apparent.

/cc @mapbox/navigation-ios @frederoni

@1ec5 1ec5 added this to the v1.0 milestone Dec 21, 2019
@1ec5
Copy link
Contributor Author

1ec5 commented Dec 26, 2019

I’m not sure if equality comparisons are contributing to the test’s flakiness, but we could improve equality performance by storing Polyline-encoded representations of Route.shape and RouteStep.shape and comparing those strings instead of the full LineString structs.

Locally, testGeoJSON() takes a little over a second with a 10% standard deviation, much less than the 5-second timeout. I’m getting suspicious that this is something other than a performance issue.

@1ec5
Copy link
Contributor Author

1ec5 commented Dec 27, 2019

Per #354 #353 (comment), the failures may be due to a race condition or other nondeterminism rather than poor performance.

@1ec5
Copy link
Contributor Author

1ec5 commented Jan 7, 2020

Fixed for now in #404. If it is a race condition, we can revisit it when it comes up in some other form, but this end-to-end test isn’t a good vehicle for understanding any concurrency issue.

@1ec5 1ec5 closed this as completed Jan 7, 2020
@1ec5 1ec5 modified the milestones: v1.0.0, v0.31.0 (v0.40) Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant