NAVAND-775: support waypoints per route by dzinad · Pull Request #6555 · mapbox/mapbox-navigation-android

dzinad · 2022-11-03T19:03:39Z

No description provided.

dzinad · 2022-11-03T19:04:33Z

Adding the DO NOT MERGE label as it requires bumping NN to 119.0.2.
Right now some instrumentation tests are expected to fail.

codecov · 2022-11-04T10:37:04Z

Codecov Report

Merging #6555 (b3f5681) into main (4b43089) will increase coverage by 0.02%.
The diff coverage is 81.48%.

@@             Coverage Diff              @@
##               main    #6555      +/-   ##
============================================
+ Coverage     72.38%   72.40%   +0.02%     
- Complexity     5392     5399       +7     
============================================
  Files           764      764              
  Lines         29372    29377       +5     
  Branches       3477     3480       +3     
============================================
+ Hits          21260    21270      +10     
+ Misses         6719     6718       -1     
+ Partials       1393     1389       -4

Impacted Files	Coverage Δ
.../mapbox/navigation/base/internal/utils/RouterEx.kt	`45.45% <0.00%> (ø)`
...on/navigator/internal/MapboxNativeNavigatorImpl.kt	`0.00% <0.00%> (ø)`
...pbox/navigation/core/accounts/BillingController.kt	`88.09% <50.00%> (ø)`
...om/mapbox/navigation/base/route/NavigationRoute.kt	`53.95% <71.42%> (+1.80%)`	⬆️
...avigation/base/internal/route/NavigationRouteEx.kt	`83.90% <100.00%> (+0.76%)`	⬆️
...n/core/routerefresh/DirectionsRouteDiffProvider.kt	`83.72% <100.00%> (+5.14%)`	⬆️
...igation/base/internal/factory/RoadObjectFactory.kt	`86.36% <0.00%> (+4.54%)`	⬆️

vadzim-vys · 2022-11-08T10:01:08Z

+    fun ev_route_with_waypoints_in_response_root_by_default() = sdkTest {
+        addResponseHandler(R.raw.ev_route_response_with_waypoints_in_root, evCoordinates)
+        val routes = requestRoutes(evCoordinates, electric = true, waypointsPerRoute = null)
+
+        checkWaypointsInRoot(expectedEvWaypointsNamesAndLocations, routes[0])
+    }
+
+    @Test
+    fun ev_route_with_waypoints_in_response_root() = sdkTest {
+        addResponseHandler(R.raw.ev_route_response_with_waypoints_in_root, evCoordinates)
+        val routes = requestRoutes(evCoordinates, electric = true, waypointsPerRoute = false)
+
+        checkWaypointsInRoot(expectedEvWaypointsNamesAndLocations, routes[0])
+    }
+
+    @Test
+    fun ev_route_with_waypoints_per_route() = sdkTest {
+        addResponseHandler(R.raw.ev_route_response_with_waypoints_per_route, evCoordinates)
+        val routes = requestRoutes(evCoordinates, electric = true, waypointsPerRoute = true)
+
+        checkWaypointsPerRoute(expectedEvWaypointsNamesAndLocations, routes[0])
+    }
+
+    @Test
+    fun non_ev_route_with_waypoints_in_response_root_by_default() = sdkTest {
+        addResponseHandler(R.raw.route_response_with_waypoints_in_root, nonEvCoordinates)
+        val routes = requestRoutes(nonEvCoordinates, electric = false, waypointsPerRoute = null)
+
+        checkWaypointsInRoot(expectedFirstNonEvWaypointsNamesAndLocations, routes[0])
+        checkWaypointsInRoot(expectedFirstNonEvWaypointsNamesAndLocations, routes[1])
+    }
+
+    @Test
+    fun non_ev_route_with_waypoints_in_response_root() = sdkTest {
+        addResponseHandler(R.raw.route_response_with_waypoints_in_root, nonEvCoordinates)
+        val routes = requestRoutes(nonEvCoordinates, electric = false, waypointsPerRoute = false)
+
+        checkWaypointsInRoot(expectedFirstNonEvWaypointsNamesAndLocations, routes[0])
+        checkWaypointsInRoot(expectedFirstNonEvWaypointsNamesAndLocations, routes[1])
+    }
+
+    @Test
+    fun non_ev_route_with_waypoints_per_route() = sdkTest {
+        addResponseHandler(R.raw.route_response_with_waypoints_per_route, nonEvCoordinates)
+        val routes = requestRoutes(nonEvCoordinates, electric = false, waypointsPerRoute = true)
+
+        checkWaypointsPerRoute(expectedFirstNonEvWaypointsNamesAndLocations, routes[0])
+        checkWaypointsPerRoute(expectedSecondNonEvWaypointsNamesAndLocations, routes[1])
+    }


They seem like unit tests which are executed on device. I understand why it's done like this, we can't access NN in unit tests. I added NAVSDK-726 to address absence of NN in unit tests later.

No, not because of that. It's not a unit test. It's an integration test: we request routes and parse the response.
We test the significant part of the SDK, not some specific class.
We already have such tests: see NavigationRouteTest.
And we don't have access to NN classes here either: everything that's checked here is represented by the SDK classes.

It seems that NavigationRoute#internalWaypoints is calculated using NN, isn't it?

Yes and they are checked here as well. What are you saying?

dzinad · 2022-11-14T08:18:29Z

Supported in NN and checked. Removing DO NOT MERGE.

vadzim-vys · 2022-11-15T10:02:18Z

+    override fun equals(other: Any?): Boolean {
+        if (this === other) return true
+        if (javaClass != other?.javaClass) return false
+
+        other as ApproximateCoordinates
+
+        if (abs(latitude - other.latitude) > tolerance) return false
+        if (abs(longitude - other.longitude) > tolerance) return false
+
+        return true
+    }
+
+    override fun hashCode(): Int {
+        var result = latitude.hashCode()
+        result = 31 * result + longitude.hashCode()
+        return result
+    }


as I understand two approximate coordinates could be equals, but their hash codes will be different. In this case data structures like hash tables won't be able to find equal objects. Not sure if it's important problem, but this could be unexpected

Hmmm, I think you're right. It's not a problem now because it's used from assertEquals.
But I don't see the point of using ApproximateCoordinates as a key in hash table. Because for 2 different coordinates you'll get the same key... However, I think it would make sense to just return 0 here to avoid this unexpectedness.

vadzim-vys · 2022-11-15T10:34:57Z

 }

-private fun DirectionsResponse.Builder.updateWaypoints(
+private fun buildNewWaypoints(


I should have asked this before, but I noticed this functionality only now.
What kind of update in waypoints do we expect? We don't expect to receive waypoints with different coordinates, do we?

We don't. Only the metadata will be updated for now. Do you think we should update only unrecognizedProperties? Maybe it makes sense: we have a white-list of what we update.

yep, reading the code, it's not obvious what's expected to change and what's not. It would be nice to explicitly update fields which we expect to be changed or explicitly save the ones we don't expect to be updated.

vadzim-vys · 2022-11-15T10:42:28Z

+        route1 = NavigationRoute(
+            directionsResponse1,
+            0,
+            routeOptions1,
+            mockk(relaxUnitFun = true) {
+                every { routeId } returns id1
+                every { responseUuid } returns "uuid#0"
+                every { routerOrigin } returns RouterOrigin.ONBOARD
+                every { routeInfo } returns RouteInfo(listOf(mockk(relaxed = true)))
+            }
+        )


maybe?

Suggested change

route1 = NavigationRoute(

directionsResponse1,

0,

routeOptions1,

mockk(relaxUnitFun = true) {

every { routeId } returns id1

every { responseUuid } returns "uuid#0"

every { routerOrigin } returns RouterOrigin.ONBOARD

every { routeInfo } returns RouteInfo(listOf(mockk(relaxed = true)))

}

)

route1 = createNavigationRoutes(

response = directionsResponse1,

options = routeOptions1

).first()

I need to mock id. I can use spy but IMO switching to createNavigationRoutes gives no profit here.

createNavigationRoutes generates in the same way as NN does, so it's very similar, if not identical, to how the real system works. is that enough for unit tests?

How can I mock the id if I use createNavigationRoutes?
I only see they way with creating a spy.
My question is: why is it better than the current approach?

vadzim-vys · 2022-11-15T10:56:22Z

+                // #0
+                arrayOf(null, null, null, null),
+                arrayOf(false, null, null, null),
+                arrayOf(true, null, null, null),


How can it happen that a route doesn't have waypoints? Is that done for backward compatibility?

That's the point of unit tests. Technically they can be null and the method that is being tested accounts for this. And to invoke this method we don't need to know the whole business logic of the SDK (that it can't IRL have null waypoints).

I see your point.

that it can't IRL have null waypoints

should the API on NavigationRoute be nullable then?

Technically they can be null. Because both directionsRoute.waypoints() and directionsResponse.waypoints() are nullable. Our backend claims one of them will be returned (depending on the waypoints_per_route parameter).
But what if this behaviour changes? And do we really trust backend that much? You mentioned yourself that it's better to introduce custom request timeout than rely on NN.
I can return an empty list instead of course. WDYT?

vadzim-vys · 2022-11-15T10:58:00Z

+                arrayOf(null, filledWaypoints2, null, filledWaypoints2),
+                arrayOf(false, filledWaypoints2, null, filledWaypoints2),
+                arrayOf(true, filledWaypoints2, null, null),
+                // #21


just curious, why do you need this numbers?

In the test description the index is used as a description. This way it's easy to understand which case fails. Another way is adding a specific description and I like it, but I think that here it's not really necessary because it's quite clear from the input parameters and it would only kinda duplicate them. For more complex tests description is good though.

vadzim-vys · 2022-11-15T11:01:35Z

+        val route = NavigationRoute(
+            DirectionsResponse.builder()
+                .routes(
+                    listOf(
+                        DirectionsRoute.builder()
+                            .distance(1.0)
+                            .duration(2.9)
+                            .waypoints(routeWaypoints)
+                            .build()
+                    )
+                )
+                .waypoints(responseWaypoints)
+                .code("Ok")
+                .build(),
+            0,
+            RouteOptions.builder()
+                .profile(DirectionsCriteria.PROFILE_DRIVING_TRAFFIC)
+                .coordinatesList(
+                    listOf(
+                        Point.fromLngLat(1.1, 2.2),
+                        Point.fromLngLat(3.3, 4.4)
+                    )
+                )
+                .waypointsPerRoute(waypointsPerRoute)
+                .build(),
+            mockk(relaxed = true)
+        )


would com.mapbox.navigation.testing.factories.NavigationRouteFactoryKt#createNavigationRoutes simplify this setup?

vadzim-vys · 2022-11-15T11:08:36Z

+        mapboxNavigation.setNavigationRoutesAndWaitForUpdate(requestedRoutes)
+        mapboxNavigation.startTripSession()
+        stayOnInitialPosition()
+        val updatedRoutes = waitUntilRefresh().navigationRoutes


we set route created with charge level 18000 to navigation which already has charge level 17000. Is that expected?

Sure. The routes were requested when the charge was 18000. Then the charge dropped to 17000 and we told MapboxNavigation about it. Then we set the requested routes. I don't see anything invalid in this case. The latest data (17000) should be used in the refresh request.

LukasPaczos · 2022-11-28T13:03:52Z

I added missing docs and updated the BillingController to also use NavigationRoute#waypoints. LGTM but I'd appreciate another round from @mapbox/navigation-android.

tomaszrybakiewicz · 2022-11-29T17:35:14Z

I've rebased on top of the latest main branch and resolved conflicts in CHANGELOG.

dzinad requested a review from a team as a code owner November 3, 2022 19:03

dzinad force-pushed the NAVAND-775-dd-sdk branch from d47869e to 9dce7a8 Compare November 3, 2022 19:04

dzinad added the ⚠️ DO NOT MERGE PR should not be merged! label Nov 3, 2022

dzinad force-pushed the NAVAND-775-dd-sdk branch 2 times, most recently from 30dfbf8 to 3b903b3 Compare November 4, 2022 09:53

dzinad commented Nov 4, 2022

View reviewed changes

Comment thread libnavigation-base/src/main/java/com/mapbox/navigation/base/route/NavigationRoute.kt

dzinad force-pushed the NAVAND-775-dd-sdk branch from 3b903b3 to c65370e Compare November 8, 2022 09:35

vadzim-vys reviewed Nov 8, 2022

View reviewed changes

dzinad mentioned this pull request Nov 9, 2022

log state_of_charge updates #6579

Merged

dzinad force-pushed the NAVAND-775-dd-sdk branch 2 times, most recently from 9131ebd to feb2153 Compare November 14, 2022 08:18

dzinad removed the ⚠️ DO NOT MERGE PR should not be merged! label Nov 14, 2022

dzinad force-pushed the NAVAND-775-dd-sdk branch from feb2153 to bf30de9 Compare November 14, 2022 08:19

vadzim-vys reviewed Nov 15, 2022

View reviewed changes

dzinad force-pushed the NAVAND-775-dd-sdk branch from bf30de9 to 31898b1 Compare November 15, 2022 10:06

vadzim-vys reviewed Nov 15, 2022

View reviewed changes

dzinad force-pushed the NAVAND-775-dd-sdk branch 2 times, most recently from afec69a to 7b23f59 Compare November 16, 2022 10:26

RingerJK reviewed Nov 17, 2022

View reviewed changes

Comment thread CHANGELOG.md Outdated

dzinad force-pushed the NAVAND-775-dd-sdk branch 2 times, most recently from e3d6434 to b231e9f Compare November 18, 2022 14:50

dzinad requested a review from vadzim-vys November 22, 2022 07:32

dzinad requested review from LukasPaczos and RingerJK November 22, 2022 07:32

LukasPaczos reviewed Nov 24, 2022

View reviewed changes

Comment thread libnavigation-base/src/main/java/com/mapbox/navigation/base/internal/route/NavigationRouteEx.kt

Comment thread libnavigation-base/src/main/java/com/mapbox/navigation/base/route/NavigationRoute.kt

dzinad force-pushed the NAVAND-775-dd-sdk branch from b231e9f to 6f92122 Compare November 25, 2022 15:05

tomaszrybakiewicz force-pushed the NAVAND-775-dd-sdk branch from 1c1b226 to c39c7f7 Compare November 29, 2022 17:34

tomaszrybakiewicz approved these changes Nov 29, 2022

View reviewed changes

dzinad and others added 3 commits November 29, 2022 17:20

NAVAND-775: support waypoints per route

e0388da

use NavigationRoute#waypoints in BillingController

cc3532d

fix NavigationRoute#waypoitns docs

b3f5681

tomaszrybakiewicz force-pushed the NAVAND-775-dd-sdk branch from c39c7f7 to b3f5681 Compare November 29, 2022 22:21

tomaszrybakiewicz enabled auto-merge (squash) November 29, 2022 22:22

tomaszrybakiewicz merged commit 592818b into main Nov 29, 2022

tomaszrybakiewicz deleted the NAVAND-775-dd-sdk branch November 29, 2022 23:07

Conversation

dzinad commented Nov 3, 2022

Uh oh!

dzinad commented Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Nov 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dzinad commented Nov 14, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vadzim-vys Nov 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LukasPaczos commented Nov 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomaszrybakiewicz commented Nov 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dzinad commented Nov 3, 2022 •

edited

Loading

codecov bot commented Nov 4, 2022 •

edited

Loading

vadzim-vys Nov 15, 2022 •

edited

Loading

LukasPaczos commented Nov 28, 2022 •

edited

Loading