-
Notifications
You must be signed in to change notification settings - Fork 319
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
Fast integration test for artificial driver #6799
Fast integration test for artificial driver #6799
Conversation
220d55a
to
1ea6c0a
Compare
ChangelogFeatures
Bug fixes and improvements
Known issues
|
Codecov Report
@@ Coverage Diff @@
## main #6799 +/- ##
=========================================
Coverage 72.64% 72.64%
+ Complexity 5569 5565 -4
=========================================
Files 780 781 +1
Lines 30104 30110 +6
Branches 3553 3554 +1
=========================================
+ Hits 21869 21874 +5
- Misses 6808 6809 +1
Partials 1427 1427
|
for (location in events.map { it.location.mapToLocation() }) { | ||
assertTrue(nativeNavigator.updateLocation(location.toFixLocation())) | ||
} |
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.
@mapbox/navnative , we have already discussed it, but can you please confirm that this is valid usage of native navigator?
I generated location updates so that they looks like the regular ones, but I pass them to NN faster than they happen in reality keeping the original time, i.e. for the native navigator time goes faster than real time.
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.
Like history recording replay, you can take the locations with proper monotonic time in them and feed back-to-back into the navigator - it will produce the correct result.
internal fun ReplayEventLocation.mapToLocation( | ||
eventTimeOffset: Double = time ?: 0.0, | ||
@VisibleForTesting currentTimeMilliseconds: Long = Date().time, | ||
@VisibleForTesting elapsedTimeNano: Long = SystemClock.elapsedRealtimeNanos() | ||
): Location { |
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.
@kmadsen , I didn't understand which problems mapboxReplayer.eventRealtimeOffset
solves, so maybe it isn't the best API for the mapper. Any suggestions?
.build() | ||
) | ||
block(mapboxNavigation, MapboxNativeNavigatorImpl) | ||
mapboxNavigation.onDestroy() |
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.
onDestroyed
won't be called if the test fails, you should use a rule for that.
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.
thanks. I think try-finaly will work too, won't they? 250aa56
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.
It will, yes.
|
||
val states = statusesTracking.await() | ||
val historyFile = suspendCoroutine<String> { continuation -> | ||
mapboxNavigation.historyRecorder.stopRecording { |
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.
There is a suspend version of this method:
val historyFile = mapboxNavigation.historyRecorder.startRecording()
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.
Can you please help me find it? I don't see suspend version here
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.
You probably meant this one, but artificial driver test is in a different module as it accesses internal APIs
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.
Yes, this one. Oh, it's a different module. OK then.
continuation.resume(it ?: "null") | ||
} | ||
} | ||
val notTrackingStates = states.filter { it.routeState != RouteState.TRACKING } |
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 this the right check? I mean do we want to test that we only have tracking or that we don't have off_route? What about INITIALIZED state for example?
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.
what do you think the right check should be?
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.
I wrote this test to verify absence of reroutes when location updates are simulated by our artificial driver. That's why I check only tracking part. But I'm open to suggestions, if you think a different check will bring us more benefits, I'm happy to change the current one 🙂
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.
Oops, I thought I left a comment but I didn't. Judging by the name of the test, I'd do sth like:
val offRouteStates = states.filter { it.routeState == RouteState.OFF_ROUTE }
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.
Good idea! applied
250aa56
to
d9471d9
Compare
261c1f7
to
428cd54
Compare
"&language=en" + | ||
"&overview=full" + | ||
"&steps=true" + | ||
"&access_token=YOUR_MAPBOX_ACCESS_TOKEN", |
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.
See NativeNavigatorCallbackOrderTest
, it already uses access token that is being injected on CI.
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.
Ah, you don't want it to be present in the URL.
Added a test which verifies that NN follows location updates generated by "artificial driver" without reroutes.
It works very fast, because it makes NN process location updates as fast as it can, so I can this PR a POC of fast integration tests(https://mapbox.atlassian.net/browse/NN-418). Route Munich-Nuremberg(duration 16240.29) completes in 1 minute on Pixel 6.