Alternatives: adopt new NN handling alternatives route flow (#6214)

Author: RingerJK

CHANGELOG.md

@@ -340,7 +340,6 @@ This release depends on, and has been tested with, the following Mapbox dependen
 - Mapbox Android Core `v5.0.2` ([release notes](https://github.com/mapbox/mapbox-events-android/releases/tag/core-5.0.2))
 - Mapbox Android Telemetry `v8.1.5` ([release notes](https://github.com/mapbox/mapbox-events-android/releases/tag/telem-8.1.5-core-5.0.2))
 
-
 ## Mapbox Navigation SDK 2.8.0-beta.1 - 25 August, 2022
 ### Changelog
 [Changes between v2.8.0-alpha.3 and v2.8.0-beta.1](https://github.com/mapbox/mapbox-navigation-android/compare/v2.8.0-alpha.3...v2.8.0-beta.1)

instrumentation-tests/src/androidTest/java/com/mapbox/navigation/instrumentation_tests/core/RouteAlternativesTest.kt

@@ -2,14 +2,20 @@ package com.mapbox.navigation.instrumentation_tests.core
 
 import android.location.Location
 import androidx.test.espresso.Espresso
+import com.mapbox.api.directions.v5.models.DirectionsResponse
 import com.mapbox.api.directions.v5.models.DirectionsRoute
 import com.mapbox.api.directions.v5.models.RouteOptions
 import com.mapbox.geojson.Point
 import com.mapbox.navigation.base.extensions.applyDefaultNavigationOptions
 import com.mapbox.navigation.base.options.NavigationOptions
+import com.mapbox.navigation.base.route.NavigationRoute
 import com.mapbox.navigation.base.route.RouteAlternativesOptions
+import com.mapbox.navigation.base.route.RouterOrigin
+import com.mapbox.navigation.base.trip.model.RouteProgress
 import com.mapbox.navigation.core.MapboxNavigation
 import com.mapbox.navigation.core.MapboxNavigationProvider
+import com.mapbox.navigation.core.routealternatives.NavigationRouteAlternativesObserver
+import com.mapbox.navigation.core.routealternatives.RouteAlternativesError
 import com.mapbox.navigation.instrumentation_tests.R
 import com.mapbox.navigation.instrumentation_tests.activity.EmptyTestActivity
 import com.mapbox.navigation.instrumentation_tests.utils.MapboxNavigationRule
@@ -25,6 +31,8 @@ import com.mapbox.navigation.testing.ui.utils.getMapboxAccessTokenFromResources
 import com.mapbox.navigation.testing.ui.utils.runOnMainSync
 import com.mapbox.navigation.utils.internal.logE
 import org.junit.Assert.assertEquals
+import org.junit.Assert.assertFalse
+import org.junit.Assert.assertNotEquals
 import org.junit.Assert.assertNotNull
 import org.junit.Assert.assertTrue
 import org.junit.Before
@@ -54,6 +62,10 @@ class RouteAlternativesTest : BaseTest<EmptyTestActivity>(EmptyTestActivity::cla
         Point.fromLngLat(-122.2647245, 37.8138895)
     )
 
+    private companion object {
+        private const val LOG_CATEGORY = "RouteAlternativesTest"
+    }
+
     override fun setupMockLocation(): Location = mockLocationUpdatesRule.generateLocationUpdate {
         latitude = coordinates[0].latitude()
         longitude = coordinates[0].longitude()
@@ -81,7 +93,7 @@ class RouteAlternativesTest : BaseTest<EmptyTestActivity>(EmptyTestActivity::cla
         setupMockRequestHandlers(coordinates)
         val routes = requestDirectionsRouteSync(coordinates)
 
-        // Play the slower alternative route.
+        // Play primary route
         runOnMainSync {
             mapboxNavigation.historyRecorder.startRecording()
             mockLocationReplayerRule.playRoute(routes.first())
@@ -107,7 +119,61 @@ class RouteAlternativesTest : BaseTest<EmptyTestActivity>(EmptyTestActivity::cla
         val countDownLatch = CountDownLatch(1)
         runOnMainSync {
             mapboxNavigation.historyRecorder.stopRecording {
-                logE("history path=$it", "DEBUG")
+                logE("history path=$it", LOG_CATEGORY)
+                countDownLatch.countDown()
+            }
+        }
+        countDownLatch.await()
+
+        // Verify alternative routes events were triggered.
+        assertEquals(2, routes.size)
+        assertTrue(mapboxNavigation.getNavigationRoutes().isNotEmpty())
+        firstAlternative.verifyOnRouteAlternativesAndProgressReceived()
+        firstAlternative.alternatives!!.forEach {
+            assertFalse(routes.contains(it))
+        }
+    }
+
+    @Test
+    fun alternative_routes_observer_is_not_triggered_for_set_routes() {
+        // Prepare with alternative routes.
+        setupMockRequestHandlers(coordinates)
+        val routes = requestDirectionsRouteSync(coordinates)
+
+        // Play primary route
+        runOnMainSync {
+            mapboxNavigation.historyRecorder.startRecording()
+            mockLocationReplayerRule.playRoute(routes.first())
+            mapboxNavigation.startTripSession()
+        }
+
+        // Wait for enhanced locations to start and then set the routes.
+        val firstLocationIdlingResource = FirstLocationIdlingResource(mapboxNavigation)
+        firstLocationIdlingResource.firstLocationSync()
+
+        // Subscribing for alternatives
+        val firstAlternative = RouteAlternativesIdlingResource(
+            mapboxNavigation
+        ) { _, alternatives, _ ->
+            alternatives.isNotEmpty()
+        }
+        firstAlternative.register()
+
+        runOnMainSync {
+            mapboxNavigation.setRoutes(routes)
+        }
+
+        mapboxHistoryTestRule.stopRecordingOnCrash("alternatives failed") {
+            Espresso.onIdle()
+        }
+        firstAlternative.unregister()
+
+        assertTrue(firstAlternative.calledOnMainThread)
+
+        val countDownLatch = CountDownLatch(1)
+        runOnMainSync {
+            mapboxNavigation.historyRecorder.stopRecording {
+                logE("history path=$it", LOG_CATEGORY)
                 countDownLatch.countDown()
             }
         }
@@ -116,8 +182,122 @@ class RouteAlternativesTest : BaseTest<EmptyTestActivity>(EmptyTestActivity::cla
         // Verify alternative routes events were triggered.
         assertEquals(2, routes.size)
         assertTrue(mapboxNavigation.getRoutes().isNotEmpty())
-        assertNotNull(firstAlternative.routeProgress)
+        firstAlternative.verifyOnRouteAlternativesAndProgressReceived()
+
+        assertNotEquals(
+            "Verify setRoutes and alternatives are not the same routes",
+            routes.drop(1).sortedBy { it.hashCode() }, // drop primary route
+            firstAlternative.alternatives!!.sortedBy { it.hashCode() }
+        )
+    }
+
+    /**
+     * The test verifies that if set routes to Navigation that are come from alternatives + n
+     * (where n >= 1) external routes, alternatives routes observer is not triggered with these routes.
+     * That was happening in legacy versions of NN.
+     */
+    @Test
+    fun additional_alternative_is_not_force_to_invoke_alternatives_observer() {
+        // Prepare with alternative routes.
+        setupMockRequestHandlers(coordinates)
+        val routes = requestDirectionsRouteSync(coordinates)
+
+        // Play primary route
+        runOnMainSync {
+            mapboxNavigation.historyRecorder.startRecording()
+            mockLocationReplayerRule.playRoute(routes.first())
+            mapboxNavigation.startTripSession()
+        }
+
+        // Wait for enhanced locations to start and then set the routes.
+        val firstLocationIdlingResource = FirstLocationIdlingResource(mapboxNavigation)
+        firstLocationIdlingResource.firstLocationSync()
+        runOnMainSync {
+            // infinity subscription to avoid triggering NN on every new observer
+            mapboxNavigation.registerRouteAlternativesObserver(
+                object : NavigationRouteAlternativesObserver {
+                    override fun onRouteAlternatives(
+                        routeProgress: RouteProgress,
+                        alternatives: List<NavigationRoute>,
+                        routerOrigin: RouterOrigin
+                    ) = Unit
+
+                    override fun onRouteAlternativesError(error: RouteAlternativesError) = Unit
+                }
+            )
+            mapboxNavigation.setRoutes(routes)
+        }
+
+        // Subscribing for alternatives
+        val firstAlternative = RouteAlternativesIdlingResource(
+            mapboxNavigation
+        ) { _, alternatives, _ ->
+            alternatives.isNotEmpty()
+        }
+        firstAlternative.register()
+        mapboxHistoryTestRule.stopRecordingOnCrash("alternatives failed") {
+            Espresso.onIdle()
+        }
+        firstAlternative.unregister()
+
         assertNotNull(firstAlternative.alternatives)
+
+        val nextAlternativeObserver = RouteAlternativesIdlingResource(
+            mapboxNavigation
+        ) { _, alternatives, _ ->
+            alternatives.isNotEmpty()
+        }
+        nextAlternativeObserver.register()
+
+        val externalAlternatives = DirectionsResponse.fromJson(
+            readRawFileText(activity, R.raw.route_response_alternative_continue)
+        ).routes().also {
+            assertEquals(1, it.size)
+        }
+
+        lateinit var setRoutes: List<DirectionsRoute>
+        runOnMainSync {
+            setRoutes = (
+                mutableListOf(
+                    mapboxNavigation.getRoutes().first()
+                ) + firstAlternative.alternatives!! + externalAlternatives
+                ).also {
+                assertTrue(
+                    "Primary route + >=1 alternatives + external alternatives",
+                    it.size >= 3
+                )
+            }
+            mapboxNavigation.setRoutes(setRoutes)
+        }
+
+        mapboxHistoryTestRule.stopRecordingOnCrash("next alternatives failed") {
+            Espresso.onIdle()
+        }
+        nextAlternativeObserver.unregister()
+
+        val countDownLatch = CountDownLatch(1)
+        runOnMainSync {
+            mapboxNavigation.historyRecorder.stopRecording {
+                logE("history path=$it", LOG_CATEGORY)
+                countDownLatch.countDown()
+            }
+        }
+        countDownLatch.await()
+
+        // Verify alternative routes events were triggered.
+        firstAlternative.verifyOnRouteAlternativesAndProgressReceived()
+
+        // Verify next alternative routes events were triggered
+        nextAlternativeObserver.verifyOnRouteAlternativesAndProgressReceived()
+
+        // verify that set routes with  alternatives + 1 external alternatives is not triggered
+        // alternative observer
+        assertNotEquals(
+            "Alternative are not the same as setRoutes (alternatives might have " +
+                "additional routes or remove one or a few but not equal)",
+            setRoutes.drop(1).sortedBy { it.hashCode() }, // drop primary route
+            nextAlternativeObserver.alternatives!!.sortedBy { it.hashCode() }
+        )
     }
 
     private fun setupMockRequestHandlers(coordinates: List<Point>) {

instrumentation-tests/src/androidTest/java/com/mapbox/navigation/instrumentation_tests/utils/idling/RouteAlternativesIdlingResource.kt

@@ -9,6 +9,7 @@ import com.mapbox.navigation.core.MapboxNavigation
 import com.mapbox.navigation.core.routealternatives.RouteAlternativesObserver
 import com.mapbox.navigation.testing.ui.idling.NavigationIdlingResource
 import com.mapbox.navigation.testing.ui.utils.runOnMainSync
+import junit.framework.Assert.assertNotNull
 
 /**
  * Becomes idle when [RouteAlternativesObserver.onRouteAlternatives] is invoked,
@@ -17,16 +18,25 @@ import com.mapbox.navigation.testing.ui.utils.runOnMainSync
  *
  * If your test needs to receive the next set of alternatives, you must create a new
  * instance of [RouteAlternativesIdlingResource] to capture the next callback.
+ * @param mapboxNavigation the instance of [MapboxNavigation]
+ * @param onRoutesAlternatives lambda returns boolean value: true if alternatives
+ * observer must keep working, false otherwise. If false is returned resource transiting for busy
+ * to idling (see [IdlingResource.ResourceCallback.onTransitionToIdle]).
  */
 class RouteAlternativesIdlingResource(
-    val mapboxNavigation: MapboxNavigation
+    val mapboxNavigation: MapboxNavigation,
+    private val onRoutesAlternatives: (
+        RouteProgress,
+        List<DirectionsRoute>,
+        RouterOrigin
+    ) -> Boolean = { _, _, _ -> true },
 ) : NavigationIdlingResource(), RouteAlternativesObserver {
 
     var routeProgress: RouteProgress? = null
         private set
     var alternatives: List<DirectionsRoute>? = null
         private set
-    var calledOnMainThread = true
+    var calledOnMainThread = false
 
     private var callback: IdlingResource.ResourceCallback? = null
 
@@ -52,14 +62,24 @@ class RouteAlternativesIdlingResource(
         alternatives: List<DirectionsRoute>,
         routerOrigin: RouterOrigin
     ) {
+        if (!onRoutesAlternatives.invoke(routeProgress, alternatives, routerOrigin)) {
+            return
+        }
+
         // Verify this happens on the main thread.
-        if (Looper.myLooper() != Looper.getMainLooper()) {
-            calledOnMainThread = false
+        if (Looper.myLooper() == Looper.getMainLooper()) {
+            calledOnMainThread = true
         }
 
         mapboxNavigation.unregisterRouteAlternativesObserver(this)
         this.routeProgress = routeProgress
         this.alternatives = alternatives
+
         callback?.onTransitionToIdle()
     }
+
+    fun verifyOnRouteAlternativesAndProgressReceived() {
+        assertNotNull(routeProgress)
+        assertNotNull(alternatives)
+    }
 }