NAVAND-1472: do not always relaunch timer when routes are updated #7497

dzinad · 2023-09-03T19:46:17Z

No description provided.

github-actions · 2023-09-03T19:46:47Z

Changelog

Features

Bug fixes and improvements

Improved navigation positioning in tunnels. [#7514](https://github.com/mapbox/mapbox-navigation-android/pull/7514)
Fixed an issue where route refresh interval might have been too long in case of a specific alternatives update rate. [#dd](https://github.com/mapbox/mapbox-navigation-android/pull/dd)

Known issues ⚠️

Other changes

Android Auto Changelog

Features

Bug fixes and improvements

The app is now considered as the one in active navigation only when an active route is set to MapboxNavigation. Previously it was always considered active. [#7366](https://github.com/mapbox/mapbox-navigation-android/pull/7366)
When Android Auto host tells the app to stop active navigation because another app starts navigating, now SDK will enter FreeDrive mode instead of stopping trip session completely. [#7366](https://github.com/mapbox/mapbox-navigation-android/pull/7366)

codecov · 2023-09-03T20:36:26Z

Codecov Report

Merging #7497 (40aef2a) into main (285009e) will increase coverage by 0.03%.
The diff coverage is 98.73%.

@@             Coverage Diff              @@
##               main    #7497      +/-   ##
============================================
+ Coverage     74.13%   74.16%   +0.03%     
- Complexity     6141     6151      +10     
============================================
  Files           829      830       +1     
  Lines         32883    32927      +44     
  Branches       3916     3925       +9     
============================================
+ Hits          24377    24420      +43     
- Misses         6973     6974       +1     
  Partials       1533     1533

Files Changed	Coverage Δ
...n/java/com/mapbox/navigation/core/utils/Delayer.kt	`92.85% <92.85%> (ø)`
...ava/com/mapbox/navigation/core/MapboxNavigation.kt	`71.19% <100.00%> (ø)`
...re/routerefresh/ImmediateRouteRefreshController.kt	`100.00% <100.00%> (ø)`
...core/routerefresh/PlannedRouteRefreshController.kt	`100.00% <100.00%> (ø)`
...gation/core/routerefresh/RouteRefreshController.kt	`100.00% <100.00%> (ø)`
...ore/routerefresh/RouteRefreshControllerProvider.kt	`100.00% <100.00%> (ø)`
...gation/core/routerefresh/RouteRefresherExecutor.kt	`100.00% <100.00%> (ø)`

...ndroidTest/java/com/mapbox/navigation/instrumentation_tests/core/RouteRefreshOnDemandTest.kt

VysotskiVadim · 2023-09-04T10:12:36Z

I'm wondering, how stable the tests are. I see many assumptions that 1-1.5 second accuracy i enough, did you check if that works?

VysotskiVadim · 2023-09-04T10:20:25Z

...igation-core/src/main/java/com/mapbox/navigation/core/routerefresh/RouteRefreshController.kt

@@ -62,7 +62,7 @@ class RouteRefreshController internal constructor(
        }
        plannedRouteRefreshController.pause()
        immediateRouteRefreshController.requestRoutesRefresh(routes) {
-            if (it.value?.anySuccess() == false) {
+            if (it.isValue) {


why don't we resume in case of failture?

Null here doesn't mean failure. Null means it wasn't executed because a newer request came along.

does null mean that another request was scheduled and current is cancelled so that we don't need to resume planned refreshes?

Yeah, because we'll think of that when the new request finishes, this on is just replaced by a new one.
It's not super clear, I think it'd be better to introduce some descriptive enum instead of Expected.

I think it'd be better to introduce some descriptive enum instead of Expected.

Great idea 👍

VysotskiVadim · 2023-09-04T10:23:15Z

...ndroidTest/java/com/mapbox/navigation/instrumentation_tests/core/RouteRefreshOnDemandTest.kt

+    }
+
+    @Test
+    fun route_refresh_on_demand_after_reroute() = sdkTest {


have you considered verifying behaviour in a unit test which tests RouteRefreshController which uses real implementation or Immediate and planned route request so that only router is mocked? I think those types of situations could be emulated there.

Yes. We have such unit tests. But the point here is that routes change externally (from MapboxNavigation). There is also filtering by reason there. That's why I want to check everything together here.

There is also filtering by reason there.

Why not to move filtering by reason to route refresh controller?

I updated the tests.

dzinad · 2023-09-04T12:07:32Z

I'm wondering, how stable the tests are. I see many assumptions that 1-1.5 second accuracy i enough, did you check if that works?

Looks like they are fine.

VysotskiVadim · 2023-09-05T08:36:40Z

...-core/src/main/java/com/mapbox/navigation/core/routerefresh/PlannedRouteRefreshController.kt

+                    val refreshedRoutes = listOf(it.primaryRouteRefresherResult.route) +
+                        it.alternativesRouteRefresherResults.map { it.route }
+                    routesToRefresh = refreshedRoutes
+                    scheduleNewUpdate(refreshedRoutes, false)


I really like that route refresh controller doesn't wait for new routes set from directions session to start the next cycle of refresh, we probably should have done it from the beginning. That decreases coupling + this dependency has never been obvious to me.

++, it took me a while to understand how that works.

VysotskiVadim · 2023-09-05T08:50:07Z

...igation-core/src/main/java/com/mapbox/navigation/core/routerefresh/RouteRefreshController.kt

@@ -111,6 +111,7 @@ class RouteRefreshController internal constructor(

    internal fun requestPlannedRouteRefresh(routes: List<NavigationRoute>) {
        routeRefresherResultProcessor.reset()
+        immediateRouteRefreshController.cancel()


is that supposed to be called only for new routes?

I don't think so. Otherwise we might get updated routes that are now changed.
When immediate refresh finishes, the routes might have already been changed. We'd get a race here. We'll notify RoutesObserver with old refreshed routes.

...ests/src/androidTest/java/com/mapbox/navigation/instrumentation_tests/core/RefreshTtlTest.kt

VysotskiVadim · 2023-09-19T14:45:10Z

...ts/src/androidTest/java/com/mapbox/navigation/instrumentation_tests/core/RouteRefreshTest.kt

+
+        val routeOptions = generateRouteOptions(twoCoordinates)
+
+        mockWebServerRule.requestHandlers.remove(failByRequestRouteRefreshResponse)


why do you remove failByRequestRouteRefreshResponse? 🤔

I want to replace refresh handler. For that I need to remove the old one.

VysotskiVadim · 2023-09-19T14:57:15Z

...ts/src/androidTest/java/com/mapbox/navigation/instrumentation_tests/core/RouteRefreshTest.kt

+            listOf(
+                RouteRefreshExtra.REFRESH_STATE_STARTED,
+                RouteRefreshExtra.REFRESH_STATE_CANCELED,
+                RouteRefreshExtra.REFRESH_STATE_STARTED,
+                RouteRefreshExtra.REFRESH_STATE_FINISHED_SUCCESS,
+            ),


given the first delay of 5 seconds and the second of 2 seconds and refresh interval 3 seconds, so that 2 route refresh happen, shouldn't states contain two REFRESH_STATE_FINISHED_SUCCESS?

This one actually is duplicated by a unit test so it has to be deleted.
Wrt your question, the first attempt will fail, so no, only one success.

Oh, I didn't notice NthAttemptHandler

.../src/test/java/com/mapbox/navigation/core/routerefresh/PlannedRouteRefreshIntegrationTest.kt

dzinad requested a review from a team as a code owner September 3, 2023 19:46

VysotskiVadim reviewed Sep 4, 2023

View reviewed changes

...ndroidTest/java/com/mapbox/navigation/instrumentation_tests/core/RouteRefreshOnDemandTest.kt Outdated Show resolved Hide resolved

VysotskiVadim reviewed Sep 4, 2023

View reviewed changes

...ndroidTest/java/com/mapbox/navigation/instrumentation_tests/core/RouteRefreshOnDemandTest.kt Outdated Show resolved Hide resolved

VysotskiVadim reviewed Sep 4, 2023

View reviewed changes

dzinad force-pushed the NAVAND-1472-dd branch from 1c6af44 to 3def102 Compare September 4, 2023 12:07

VysotskiVadim reviewed Sep 5, 2023

View reviewed changes

...ests/src/androidTest/java/com/mapbox/navigation/instrumentation_tests/core/RefreshTtlTest.kt Show resolved Hide resolved

dzinad force-pushed the NAVAND-1472-dd branch from 3def102 to d12a500 Compare September 18, 2023 14:49

dzinad requested a review from VysotskiVadim September 18, 2023 14:49

VysotskiVadim reviewed Sep 19, 2023

View reviewed changes

dzinad force-pushed the NAVAND-1472-dd branch 4 times, most recently from d5f0d37 to acf4a34 Compare September 19, 2023 16:42

VysotskiVadim approved these changes Sep 20, 2023

View reviewed changes

.../src/test/java/com/mapbox/navigation/core/routerefresh/PlannedRouteRefreshIntegrationTest.kt Outdated Show resolved Hide resolved

dzinad force-pushed the NAVAND-1472-dd branch from 7cc9b30 to 556f2ef Compare September 20, 2023 14:06

dzinad enabled auto-merge (squash) September 20, 2023 14:06

dzinad disabled auto-merge September 20, 2023 15:38

dzinad enabled auto-merge (squash) September 20, 2023 15:44

dzinad added 3 commits September 21, 2023 10:52

NAVAND-1472: do not always relaunch timer when routes are updated

9a2a44e

NAVAND-1472: move tests to JUnit

cf71be8

NAVAND-1472: use a descriptive sealed class instead of Expected

fcf83fb

dzinad force-pushed the NAVAND-1472-dd branch from 1b94a5d to fcf83fb Compare September 21, 2023 07:52

Rename changelog files

40aef2a

dzinad merged commit 9643076 into main Sep 21, 2023

dzinad deleted the NAVAND-1472-dd branch September 21, 2023 17:24

mapbox-github-ci-issues-4 bot mentioned this pull request Sep 21, 2023

PR #7497 merged without successful status checks (mapbox-navigation-android) #7527

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NAVAND-1472: do not always relaunch timer when routes are updated #7497

NAVAND-1472: do not always relaunch timer when routes are updated #7497

dzinad commented Sep 3, 2023

github-actions bot commented Sep 3, 2023 •

edited

Loading

Features

Bug fixes and improvements

Known issues ⚠️

Other changes

Features

Bug fixes and improvements

codecov bot commented Sep 3, 2023 •

edited

Loading

VysotskiVadim commented Sep 4, 2023

VysotskiVadim Sep 4, 2023

dzinad Sep 4, 2023

VysotskiVadim Sep 5, 2023

dzinad Sep 5, 2023

VysotskiVadim Sep 19, 2023

VysotskiVadim Sep 4, 2023

dzinad Sep 4, 2023

VysotskiVadim Sep 4, 2023

dzinad Sep 18, 2023

dzinad commented Sep 4, 2023

VysotskiVadim Sep 5, 2023 •

edited

Loading

dzinad Sep 5, 2023

VysotskiVadim Sep 5, 2023

dzinad Sep 5, 2023

VysotskiVadim Sep 19, 2023

dzinad Sep 19, 2023

VysotskiVadim Sep 19, 2023

dzinad Sep 19, 2023

VysotskiVadim Sep 19, 2023


		val routeOptions = generateRouteOptions(twoCoordinates)

		mockWebServerRule.requestHandlers.remove(failByRequestRouteRefreshResponse)

NAVAND-1472: do not always relaunch timer when routes are updated #7497

NAVAND-1472: do not always relaunch timer when routes are updated #7497

Conversation

dzinad commented Sep 3, 2023

github-actions bot commented Sep 3, 2023 • edited Loading

Features

Bug fixes and improvements

Known issues ⚠️

Other changes

Features

Bug fixes and improvements

codecov bot commented Sep 3, 2023 • edited Loading

Codecov Report

VysotskiVadim commented Sep 4, 2023

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

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

dzinad commented Sep 4, 2023

VysotskiVadim Sep 5, 2023 • edited Loading

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

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

github-actions bot commented Sep 3, 2023 •

edited

Loading

codecov bot commented Sep 3, 2023 •

edited

Loading

VysotskiVadim Sep 5, 2023 •

edited

Loading