Refactor MapboxTimer and Faster Route #2575

kmadsen · 2020-03-11T19:09:41Z

Description

Route Refresh is similar to Faster Route. After working on both of them some, I'm adding an incremental improvement. https://github.com/mapbox/navigation-sdks/issues/215

Would like to use this with Route Refresh #2570

I have added any issue links
I have added all related labels (bug, feature, new API(s), SEMVER, etc.)
I have added the appropriate milestone and project boards

Implementation

Make MapboxTimer tests simulate time rather than delay actual time
Remove faster route interval from NavigationOptions
-- Configurations here break customers downstream
-- Configurations here are immutable, this is making it mutable
Make MapboxTimer intervals mutable, we want to be able to safely change it whenever we want

Testing

Please describe the manual tests that you ran to verify your changes

I have tested locally (including SNAPSHOT upstream dependencies if needed) through testapp/demo app and run all activities to avoid regressions
I have tested via a test drive, or a simulation/mock location app
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have updated the CHANGELOG including this PR

cc: @abhishek1508 @olegzil @Guardiola31337

kmadsen · 2020-03-11T19:37:11Z

libnavigation-core/src/test/java/com/mapbox/navigation/core/MapboxNavigationTest.kt

@@ -195,48 +190,18 @@ class MapboxNavigationTest {
        verify(exactly = 1) { tripSession.unregisterAllVoiceInstructionsObservers() }
    }

-    @Test
-    fun fasterRoute_timerStarted() {


Faster route and route refresh are not required for navigation. Move this to be tested independently

codecov-io · 2020-03-11T21:17:32Z

Codecov Report

Merging #2575 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #2575   +/-   ##
=========================================
  Coverage     29.16%   29.16%           
  Complexity     1061     1061           
=========================================
  Files           281      281           
  Lines         10731    10731           
  Branches        870      870           
=========================================
  Hits           3130     3130           
  Misses         7258     7258           
  Partials        343      343

kmadsen · 2020-03-11T23:30:41Z

libnavigation-util/src/test/java/com/mapbox/navigation/utils/timer/MapboxTimerTest.kt


-    @Test
-    fun multipleStartCalls_doNotExecuteMultipleJobs() = runBlocking {


This test is not applicable to everything that has a MapboxTimer.

Moving this logic so it can be handled by FasterRoute
#2577

abhishek1508 · 2020-03-12T15:03:52Z

@kmadsen I looked at the PR, but I was curious as to why did we remove the configuration where the client can decide how often he wants the SDK to request faster route. Also I am not sure if making the MapboxTimer polling rate mutable. This can be easily misused intentionally/unintentionally.

kmadsen · 2020-03-12T16:03:47Z

I was curious as to why did we remove the configuration where the client can decide how often he wants the SDK to request faster route. Also I am not sure if making the MapboxTimer polling rate mutable. This can be easily misused intentionally/unintentionally.

@abhishek1508 I noticed the faster route configuration in the constructor when it made the driver app require a migration to support a new sdk version. Also, when it is in the constructor customers have to decide their interval rate once and for all when compiling/initializing.

Many companies have server side configurations, where they can control their knobs at runtime. This is why I'm moving things to be mutable at runtime.

kmadsen · 2020-03-12T16:13:30Z

Also I am not sure if making the MapboxTimer polling rate mutable. This can be easily misused intentionally/unintentionally.

@abhishek1508 I'm unable to share this concern at this point. MapboxTimer calls a delay. Before this change, a customer could accidentally set the FasterRoute to seconds instead of milliseconds. Creating a polling loop that is 1000x faster than expected. My bigger concern is giving customers direct access to the polling loop.

Guardiola31337

Left a couple of minor comments / questions. Other than that this looks good to me @kmadsen 👍

Guardiola31337 · 2020-03-13T14:04:07Z

libnavigation-util/src/main/java/com/mapbox/navigation/utils/timer/MapboxTimer.kt


-    private val timerJob: Job by lazy {
-        mainControllerJobScope.scope.launch {
+    var routeRefreshInterval = TimeUnit.MINUTES.toMillis(1)


If this is a "generic" timer, shouldn't we use a more abstract naming? 🤔 Also in this particular case it's route refresh is confusing because we already have the concept (feature) of route refresh 😅 The same thing happens to startRouteRefresh 👇 What do you think?

It's still a MapboxTimer, not a EveryoneTimer. This class is only used to update traffic routes, tend to make things more specific at the beginning. When it's used in more places it could become more abstract.

But actually, this topic is out of scope of the refactor I'm doing so will go back to the original name

Guardiola31337 · 2020-03-13T14:04:17Z

libnavigation-util/src/test/java/com/mapbox/navigation/utils/timer/MapboxTimerTest.kt

-        MapboxTimer(100L, testLambda).start()
-        delay(220)
+    @Test
+    fun `should not call before interval`() = coroutineRule.runBlockingTest {


NIT Normally is good to structure the tests following the triple A 👀 https://speakerdeck.com/guardiola31337/elegant-unit-testing-mobilization-2016?slide=40 http://wiki.c2.com/?ArrangeActAssert
If the Arrange part gets too long we should create factory or builder methods.

The same thing applies to the rest of the tests 👇

updated the whitespace, is more AAA now. Arrange is setting the timer interval, so only a single line

Guardiola31337

Minor comment not blocking the PR 🚀

Guardiola31337 · 2020-03-13T18:09:46Z

libnavigation-util/src/test/java/com/mapbox/navigation/utils/timer/MapboxTimerTest.kt

+        mapboxTimer.restartAfter = TimeUnit.MINUTES.toMillis(5)
+
+        // Should emit 1
+        val job1 = mapboxTimer.startTimer(mockLambda)


NIT not a big fan of naming like this job1, job2, etc. 😬 but not a big deal, could we come with a better name?

hmm yeah i'm typically not either but this 1 means first. not sure if i rename it to first, if that seems passive to your comment. would you have a suggestion?

firstJob, secondJob

jobFirst, jobSecond

firstTimerJob, secondTimerJob

firstJob, secondJob

Sounds good to me 🚀

Lebedinsky · 2020-03-13T18:19:59Z

libnavigation-util/src/main/java/com/mapbox/navigation/utils/timer/MapboxTimer.kt


-    private val timerJob: Job by lazy {
-        mainControllerJobScope.scope.launch {
+    var restartAfter = TimeUnit.MINUTES.toMillis(1)


Should we rename it to restartAfterMillis or smth like it to show info about used time unit in variable name? wdyt?

i'm also a fan of putting units on all the variables with units

Yeah that's a good point. Great catch!

kmadsen force-pushed the km-update-mapbox-timer branch from 7156de4 to 9d8da20 Compare March 11, 2020 19:35

kmadsen commented Mar 11, 2020

View reviewed changes

kmadsen force-pushed the km-update-mapbox-timer branch from 9d8da20 to 9c20915 Compare March 11, 2020 21:04

kmadsen force-pushed the km-update-mapbox-timer branch from 9c20915 to b166a48 Compare March 11, 2020 22:13

kmadsen mentioned this pull request Mar 11, 2020

Move faster route out of mapbox navigation #2577

Merged

10 tasks

kmadsen commented Mar 11, 2020

View reviewed changes

Guardiola31337 reviewed Mar 13, 2020

View reviewed changes

kmadsen force-pushed the km-update-mapbox-timer branch 4 times, most recently from ba6799c to fab3dcd Compare March 13, 2020 16:02

Guardiola31337 approved these changes Mar 13, 2020

View reviewed changes

Lebedinsky reviewed Mar 13, 2020

View reviewed changes

Improve mapbox timer

fb755b7

kmadsen force-pushed the km-update-mapbox-timer branch from 6c83e38 to fb755b7 Compare March 13, 2020 18:24

kmadsen merged commit d06ed3b into master Mar 13, 2020

kmadsen deleted the km-update-mapbox-timer branch March 13, 2020 18:45

kmadsen mentioned this pull request Mar 25, 2020

Add route refresh #2570

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor MapboxTimer and Faster Route #2575

Refactor MapboxTimer and Faster Route #2575

kmadsen commented Mar 11, 2020 •

edited

Loading

kmadsen Mar 11, 2020

codecov-io commented Mar 11, 2020 •

edited

Loading

kmadsen Mar 11, 2020

abhishek1508 commented Mar 12, 2020

kmadsen commented Mar 12, 2020 •

edited

Loading

kmadsen commented Mar 12, 2020 •

edited

Loading

Guardiola31337 left a comment

Guardiola31337 Mar 13, 2020

kmadsen Mar 13, 2020

Guardiola31337 Mar 13, 2020

kmadsen Mar 13, 2020

Guardiola31337 left a comment

Guardiola31337 Mar 13, 2020

kmadsen Mar 13, 2020 •

edited

Loading

Guardiola31337 Mar 13, 2020

Lebedinsky Mar 13, 2020

kmadsen Mar 13, 2020 •

edited

Loading

Guardiola31337 Mar 13, 2020


		@Test
		fun multipleStartCalls_doNotExecuteMultipleJobs() = runBlocking {

Refactor MapboxTimer and Faster Route #2575

Refactor MapboxTimer and Faster Route #2575

Conversation

kmadsen commented Mar 11, 2020 • edited Loading

Description

Implementation

Testing

Checklist

Choose a reason for hiding this comment

codecov-io commented Mar 11, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

abhishek1508 commented Mar 12, 2020

kmadsen commented Mar 12, 2020 • edited Loading

kmadsen commented Mar 12, 2020 • edited Loading

Guardiola31337 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Guardiola31337 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmadsen Mar 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmadsen Mar 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmadsen commented Mar 11, 2020 •

edited

Loading

codecov-io commented Mar 11, 2020 •

edited

Loading

kmadsen commented Mar 12, 2020 •

edited

Loading

kmadsen commented Mar 12, 2020 •

edited

Loading

kmadsen Mar 13, 2020 •

edited

Loading

kmadsen Mar 13, 2020 •

edited

Loading