Upgrade RouteProgress Step Data #812

danesfeder · 2018-03-29T21:36:53Z

Closes #325

In preparation for a tunnel and dead reckoning work, we need to expose more data in the RouteProgress object. This PR does this and updates the tests accordingly

devotaaabel · 2018-04-02T21:23:59Z

...droid-navigation-ui/src/test/java/com/mapbox/services/android/navigation/ui/v5/BaseTest.java

@@ -27,4 +33,12 @@ protected String loadJsonFixture(String filename) throws IOException {
    Scanner scanner = new Scanner(inputStream, UTF_8.name()).useDelimiter("\\A");
    return scanner.hasNext() ? scanner.next() : "";
  }
+
+  protected LegStep getFirstStep(DirectionsRoute route) {


Is there somewhere else these could go? It seems very specific to be included in all tests

devotaaabel · 2018-04-02T21:35:28Z

...src/main/java/com/mapbox/services/android/navigation/v5/routeprogress/RouteStepProgress.java

+      double distanceRemaining = distanceRemaining();
+      double distanceTraveled = calculateDistanceTraveled(step, distanceRemaining);
+      distanceTraveled(distanceTraveled);
+      float fractionTraveled = calculateFractionTraveled(step, distanceTraveled);


Why are we calculating all of this before it's needed?

danesfeder · 2018-04-09T21:21:56Z

@Guardiola31337 @devotaaabel this is ready for 👀, will finish up adding tests tomorrow.

danesfeder · 2018-04-10T21:56:39Z

@Guardiola31337 @devotaaabel tests finished! This is good to go 🚀 for 👀

devotaaabel · 2018-04-16T16:03:12Z

...droid-navigation-ui/src/test/java/com/mapbox/services/android/navigation/ui/v5/BaseTest.java

@@ -27,4 +42,55 @@ protected String loadJsonFixture(String filename) throws IOException {
    Scanner scanner = new Scanner(inputStream, UTF_8.name()).useDelimiter("\\A");
    return scanner.hasNext() ? scanner.next() : "";
  }
+
+  protected RouteProgress buildRouteProgress(DirectionsRoute route,


I think instead of putting this in BaseTest we should have classes that fake data for us, like RouteProgressFaker, just so we don't clutter up BaseTest with implementation specific code

@danesfeder

I extracted the building logic from BaseTest into classes that separate the building concerns.

How did you address @devotaaabel's comment? buildRouteProgress is still in BaseTest 🤔 Could you clarify?

Guardiola31337

Great tests refactoring work @danesfeder 👏

This is looking great. A few comments / cleanup.

Guardiola31337 · 2018-04-16T11:27:33Z

...-ui/src/test/java/com/mapbox/services/android/navigation/ui/v5/camera/DynamicCameraTest.java

-      .build();
+    double stepDistanceRemainingFinal = stepDistanceRemaining == null ? 100 : stepDistanceRemaining;
+    return buildRouteProgress(aRoute, stepDistanceRemainingFinal,
+      0, 0, 0, 0);


NIT unnecessary new line

Guardiola31337 · 2018-04-16T11:36:17Z

...-ui/src/test/java/com/mapbox/services/android/navigation/ui/v5/camera/DynamicCameraTest.java

@@ -141,14 +141,9 @@ private Location buildLocationUpdate(double lng, double lat, long time) {



Why are we using the RobolectricTestRunner? If it's only for mocking Location it could be mocked using Mockito following the same approach as in #784 👀

mapbox-navigation-android/libandroid-navigation/src/test/java/com/mapbox/services/android/navigation/v5/BaseTest.java

Lines 78 to 86 in 1e5c274

protected Location buildLocationUpdate(double lng, double lat, float speed, float horizontalAccuracy, long time) {

Location location = mock(Location.class);

when(location.getLongitude()).thenReturn(lng);

when(location.getLatitude()).thenReturn(lat);

when(location.getSpeed()).thenReturn(speed);

when(location.getAccuracy()).thenReturn(horizontalAccuracy);

when(location.getTime()).thenReturn(time);

return location;

}

Guardiola31337 · 2018-04-16T11:39:55Z

...droid-navigation-ui/src/test/java/com/mapbox/services/android/navigation/ui/v5/BaseTest.java

@@ -15,6 +29,7 @@
  public static final double LARGE_DELTA = 0.1;


👀 visibility DELTA 👇

Access can be protected

Guardiola31337 · 2018-04-16T11:42:05Z

...droid-navigation-ui/src/test/java/com/mapbox/services/android/navigation/ui/v5/BaseTest.java

@@ -15,6 +29,7 @@
  public static final double LARGE_DELTA = 0.1;

  public static final String ACCESS_TOKEN = "pk.XXX";
+  private static final int FIRST_POINT = 0;


👀

Private field 'FIRST_POINT' is never used

The same thing happens with LARGE_DELTA, ACCESS_TOKEN ☝️ and compareJson 👇

Guardiola31337 · 2018-04-16T12:39:18Z

...ion/src/main/java/com/mapbox/services/android/navigation/v5/navigation/NavigationHelper.java

@@ -52,6 +70,17 @@ static Point userSnappedToRoutePosition(Location location, List<Point> coordinat
    return ((Point) feature.geometry());
  }

+  static Location buildSnappedLocation(MapboxNavigation mapboxNavigation, boolean snapToRouteEnabled,


This is only called once from NavigationEngine#handleRequest. IMO, objects and methods should live/be where their context is. From my experience utility classes become a drawer easily and we should treat them carefully. What about moving this utility method to NavigationEngine class or even a new class holding all the methods used in NavigationEngine?
The same thing happens with checkMilestones, isUserOffRoute and shouldCheckFasterRoute.

Given the size of this PR already and that this is core logic we will be touching, let's save this refactor for another PR that touches this code.

Please write a reminder down so we don't forget to 👀 this comment when touching NavigationHelper 😬

Better said we should 👀 all the comments when working with NavigationHelper again 😛

Guardiola31337 · 2018-04-17T15:40:24Z

...java/com/mapbox/services/android/navigation/v5/navigation/NavigationEventDispatcherTest.java

@@ -324,28 +317,16 @@ public void setNavigationMetricListener_didNotGetTriggeredUntilArrival() throws
    navigationEventDispatcher.addMetricArrivalListener(arrivalListener);

    // Progress that hasn't arrived


Why having comments in a test? Also 👇

Guardiola31337 · 2018-04-17T15:44:54Z

...src/test/java/com/mapbox/services/android/navigation/v5/navigation/NavigationHelperTest.java

@@ -36,77 +45,57 @@
 import static junit.framework.Assert.assertNotSame;
 import static junit.framework.Assert.assertTrue;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;

 @RunWith(RobolectricTestRunner.class)
 @Config(constants = BuildConfig.class, sdk = 25)
 public class NavigationHelperTest extends BaseTest {


Overall same comments pointed out above ☝️

RobolectricTestRunner is needed to test the milestones being triggered - removing this results in a crash

Guardiola31337 · 2018-04-17T15:46:54Z

...src/test/java/com/mapbox/services/android/navigation/v5/navigation/NavigationHelperTest.java

    assertTrue(nextManeuver.equals(route.legs().get(1).steps().get(0).maneuver().location()));
  }
+
+  @Test
+  public void createIntersectionList_returnsCompleteIntersectionList() throws Exception {


There are some blocks of duplicated code 👇
What about extracting them into private "factory" methods and used them across the tests?

If you're talking about:

RouteProgress routeProgress = buildMultiLegRouteProgress(); LegStep currentStep = routeProgress.currentLegProgress().currentStep(); LegStep upcomingStep = routeProgress.currentLegProgress().upComingStep();

This can't be extracted, as we need both current and upcoming steps for the Arrange

Guardiola31337 · 2018-04-17T15:48:52Z

...n/src/test/java/com/mapbox/services/android/navigation/v5/offroute/OffRouteDetectorTest.java

@@ -43,7 +43,6 @@ public void setup() throws Exception {

    offRouteDetector = new OffRouteDetector();
    offRouteDetector.setOffRouteCallback(mockCallback);
-    setupStepPoints(offRouteDetector);
  }



Overall same comments pointed out above ☝️

Not sure what the comment above here means

Not sure what the comment above here means

Didn't want to repeat myself but what I meant is that most of the comments already pointed out before apply here as well (e.g. #812 (comment) #812 (comment) #812 (comment)).

Guardiola31337 · 2018-04-17T15:49:13Z

.../test/java/com/mapbox/services/android/navigation/v5/routeprogress/RouteLegProgressTest.java

-    String body = loadJsonFixture(DIRECTIONS_PRECISION_6);
-    DirectionsResponse response = gson.fromJson(body, DirectionsResponse.class);
-    route = response.routes().get(0);
+  public void setup() throws Exception {


Same with RouteLegProgressTest.

The same thing happens with RouteProgressTest, RouteStepProgressTest, SnapToRouteTest and ToleranceUtilsTest.

- Update tests

- Begin current and upcoming StepIntersections

danesfeder · 2018-04-17T21:55:23Z

@Guardiola31337 Outside of your comments surrounding NavigationHelper, I believe I addressed most of your concerns / had some follow up questions. In the spirit of our recent talks, I don't think we should open a ticket, but keep it in mind moving forward. This core logic will surely be touched soon and grant us an opportunity to clean up / remove the NavigationHelper class.

@devotaaabel I extracted the building logic from BaseTest into classes that separate the building concerns.

Thanks for the great review!

Guardiola31337 · 2018-04-18T09:27:26Z

...droid-navigation-ui/src/test/java/com/mapbox/services/android/navigation/ui/v5/BaseTest.java

@@ -27,4 +42,55 @@ protected String loadJsonFixture(String filename) throws IOException {
    Scanner scanner = new Scanner(inputStream, UTF_8.name()).useDelimiter("\\A");
    return scanner.hasNext() ? scanner.next() : "";
  }
+
+  protected RouteProgress buildRouteProgress(DirectionsRoute route,


@danesfeder

I extracted the building logic from BaseTest into classes that separate the building concerns.

How did you address @devotaaabel's comment? buildRouteProgress is still in BaseTest 🤔 Could you clarify?

Guardiola31337 · 2018-04-18T09:43:31Z

...-ui/src/test/java/com/mapbox/services/android/navigation/ui/v5/camera/DynamicCameraTest.java


-@RunWith(RobolectricTestRunner.class)
-@Config(constants = BuildConfig.class, manifest = Config.DEFAULT_MANIFEST_NAME)
 public class DynamicCameraTest extends BaseTest {


In general is hard to know where the assert values come from. I know that we're taking the values from "directions_v5_precision_6.json" fixture but IMO it's not clear when you 👀 the tests at first glance. There is some kind of magic behind the scenes that forces you to 👀 the implementation to fully understand what's going on. We should remove that necessity.

Guardiola31337 · 2018-04-18T09:47:40Z

...ion/src/main/java/com/mapbox/services/android/navigation/v5/navigation/NavigationHelper.java

@@ -52,6 +70,17 @@ static Point userSnappedToRoutePosition(Location location, List<Point> coordinat
    return ((Point) feature.geometry());
  }

+  static Location buildSnappedLocation(MapboxNavigation mapboxNavigation, boolean snapToRouteEnabled,


Please write a reminder down so we don't forget to 👀 this comment when touching NavigationHelper 😬

Guardiola31337

Great refactoring work @danesfeder 👏

A few more comments 😬

Guardiola31337 · 2018-04-18T09:51:09Z

...ion/src/main/java/com/mapbox/services/android/navigation/v5/navigation/NavigationHelper.java

@@ -52,6 +70,17 @@ static Point userSnappedToRoutePosition(Location location, List<Point> coordinat
    return ((Point) feature.geometry());
  }

+  static Location buildSnappedLocation(MapboxNavigation mapboxNavigation, boolean snapToRouteEnabled,


Better said we should 👀 all the comments when working with NavigationHelper again 😛

Guardiola31337 · 2018-04-18T09:53:43Z

...ion/src/main/java/com/mapbox/services/android/navigation/v5/navigation/NavigationHelper.java

@@ -240,9 +495,48 @@ static Point nextManeuverPosition(int stepIndex, List<LegStep> steps, List<Point
    return !coords.isEmpty() ? coords.get(coords.size() - 1) : coords.get(coords.size());


Made it package private to test it.

Pretty sure it can be tested through the public methods that use it 😉 but yeah let's revisit when touching NavigationHelper again.

Guardiola31337 · 2018-04-18T10:01:11Z

...main/java/com/mapbox/services/android/navigation/v5/navigation/NavigationRouteProcessor.java

+  private void checkNewRoute(MapboxNavigation mapboxNavigation) {
+    DirectionsRoute directionsRoute = mapboxNavigation.getRoute();
+    if (RouteUtils.isNewRoute(routeProgress, directionsRoute)) {
+


NIT unnecessary new line
If we want to remark two different operations, what about extracting blocks into well-named methods?

Guardiola31337 · 2018-04-18T10:01:27Z

...main/java/com/mapbox/services/android/navigation/v5/navigation/NavigationRouteProcessor.java

+
+      indices = NavigationIndices.create(FIRST_LEG_INDEX, FIRST_STEP_INDEX);
+      processNewIndex(mapboxNavigation);
+


Same here ☝️

Guardiola31337 · 2018-04-18T10:03:57Z

...ation/src/main/java/com/mapbox/services/android/navigation/v5/offroute/OffRouteDetector.java

@@ -24,7 +23,6 @@
  private Point lastReroutePoint;
  private OffRouteCallback callback;
  private RingBuffer<Integer> distancesAwayFromManeuver = new RingBuffer<>(3);
-  private List<Point> stepPoints = new ArrayList<>();



Same here ☝️

I meant same comments as in #812 (comment) but for OffRouteDetector in this case.

Guardiola31337 · 2018-04-18T11:38:30Z

...navigation/src/test/java/com/mapbox/services/android/navigation/v5/snap/SnapToRouteTest.java

 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.robolectric.RobolectricTestRunner;
 import org.robolectric.annotation.Config;

-import java.util.List;
-
 import static junit.framework.Assert.assertNotNull;
 import static junit.framework.Assert.assertTrue;

 @RunWith(RobolectricTestRunner.class)


Why are we using the RobolectricTestRunner?

Guardiola31337 · 2018-04-18T11:40:00Z

...navigation/src/test/java/com/mapbox/services/android/navigation/v5/snap/SnapToRouteTest.java

-      .build();
-
-  }
-
  @Test
  public void sanity() throws Exception {
    Snap snap = new SnapToRoute();
    assertNotNull(snap);


👀 #812 (comment)

Guardiola31337 · 2018-04-18T11:40:46Z

...gation/src/test/java/com/mapbox/services/android/navigation/v5/utils/ToleranceUtilsTest.java

+    DirectionsRoute route = buildTestDirectionsRoute();
+    RouteProgress routeProgress = buildDefaultTestRouteProgress();
+    double distanceToIntersection = route.distance() - 39;
+    LineString lineString = LineString.fromPolyline(route.geometry(), Constants.PRECISION_6);
    Point closePoint
      = TurfMeasurement.along(lineString, distanceToIntersection, TurfConstants.UNIT_METERS);


NIT unnecessary new line

Guardiola31337 · 2018-04-18T11:41:09Z

...gation/src/test/java/com/mapbox/services/android/navigation/v5/utils/ToleranceUtilsTest.java

+    DirectionsRoute route = buildTestDirectionsRoute();
+    RouteProgress routeProgress = buildDefaultTestRouteProgress();
+    double distanceToIntersection = route.distance();
+    LineString lineString = LineString.fromPolyline(route.geometry(), Constants.PRECISION_6);
    Point closePoint
      = TurfMeasurement.along(lineString, distanceToIntersection, TurfConstants.UNIT_METERS);


NIT unnecessary new line

Guardiola31337 · 2018-04-18T11:41:54Z

.../test/java/com/mapbox/services/android/navigation/v5/routeprogress/RouteLegProgressTest.java

-      .legIndex(0)
-      .build();
+    routeProgress = routeProgress.toBuilder().stepIndex(lastStepIndex).build();
+
    assertNull(routeProgress.currentLegProgress().upComingStep());


Should be in the Arrange part 👀 upComingStep_returnsNull is telling us what's the Act here upComingStep.
Also 👀 #812 (comment)

This applies to other modified tests included in this PR (RouteProgressTest and RouteStepProgressTest).

Guardiola31337

A few minor comments not blocking the PR.

Really well done @danesfeder 👏

Guardiola31337 · 2018-04-18T19:22:31Z

...vigation-ui/src/test/java/com/mapbox/services/android/navigation/ui/v5/TestRouteBuilder.java

+
+import static okhttp3.internal.Util.UTF_8;
+
+class TestRouteBuilder {


👀 #812 (comment)

Guardiola31337 · 2018-04-18T19:26:01Z

...gation/src/test/java/com/mapbox/services/android/navigation/v5/TestRouteProgressBuilder.java

+import static com.mapbox.services.android.navigation.v5.navigation.NavigationHelper.findCurrentIntersection;
+import static com.mapbox.services.android.navigation.v5.navigation.NavigationHelper.findUpcomingIntersection;
+
+class TestRouteProgressBuilder {


👀 #812 (comment)

Guardiola31337 · 2018-04-18T19:34:27Z

...-ui/src/test/java/com/mapbox/services/android/navigation/ui/v5/TestRouteProgressBuilder.java

+import static com.mapbox.services.android.navigation.v5.navigation.NavigationHelper.findCurrentIntersection;
+import static com.mapbox.services.android.navigation.v5.navigation.NavigationHelper.findUpcomingIntersection;
+
+class TestRouteProgressBuilder {


👀 #812 (comment)

Guardiola31337 · 2018-04-18T19:36:46Z

...oid-navigation/src/test/java/com/mapbox/services/android/navigation/v5/TestRouteBuilder.java

+
+import static okhttp3.internal.Util.UTF_8;
+
+class TestRouteBuilder {


👀 #812 (comment)

danesfeder added the navigation-core label Mar 29, 2018

danesfeder self-assigned this Mar 29, 2018

danesfeder requested review from Guardiola31337 and devotaaabel March 29, 2018 21:36

danesfeder force-pushed the dan-add-rp-coordinate-intersection branch 2 times, most recently from b0280ba to 9ac2311 Compare April 2, 2018 20:08

devotaaabel reviewed Apr 2, 2018

View reviewed changes

danesfeder force-pushed the dan-add-rp-coordinate-intersection branch 2 times, most recently from fddcfb8 to 104a209 Compare April 4, 2018 19:49

danesfeder mentioned this pull request Apr 4, 2018

add nearby coords to RouteProgress #471

Closed

danesfeder force-pushed the dan-add-rp-coordinate-intersection branch 2 times, most recently from 38d18db to daf011a Compare April 5, 2018 18:22

akitchen mentioned this pull request Apr 6, 2018

User should see themselves progress through tunnel when GPS is weak or nonexistent #835

Closed

6 tasks

danesfeder force-pushed the dan-add-rp-coordinate-intersection branch 2 times, most recently from 9a9c914 to 7afd446 Compare April 9, 2018 21:20

danesfeder added this to the v0.13.0 milestone Apr 11, 2018

danesfeder force-pushed the dan-add-rp-coordinate-intersection branch from 205546f to 35d0bdb Compare April 11, 2018 20:41

devotaaabel reviewed Apr 16, 2018

View reviewed changes

Guardiola31337 requested changes Apr 17, 2018

View reviewed changes

danesfeder added 9 commits April 17, 2018 14:38

Add current and upcoming step coordinates to RouteProgress

4186562

Update route progress with current and upcoming step points

d771e82

- Update tests

Add tunnel distance calculation

6b37463

Finish intersection calculations

1a958a1

Move distance processing to RouteProcessor

46d36d6

Don't allow processing of first point

5d85306

Update next step nullable

7d3a2f8

Update tests

10cb103

Add current annotation work

ab99ef8

- Begin current and upcoming StepIntersections

danesfeder added 3 commits April 17, 2018 14:38

Add current and upcoming intersections in RouteStepProgress

830b130

Finish tests and update test fixtures

a1fa1b0

Remove tunnel milestone, no longer in scope

3b978cd

danesfeder force-pushed the dan-add-rp-coordinate-intersection branch 2 times, most recently from 67cfb4d to 5c96cda Compare April 17, 2018 21:51

Guardiola31337 reviewed Apr 18, 2018

View reviewed changes

danesfeder force-pushed the dan-add-rp-coordinate-intersection branch 2 times, most recently from c94c73c to 75fc29f Compare April 18, 2018 14:34

Guardiola31337 approved these changes Apr 18, 2018

View reviewed changes

Guardiola31337 reviewed Apr 18, 2018

View reviewed changes

Update CurrentLegAnnotation javadoc

2d7dd24

danesfeder force-pushed the dan-add-rp-coordinate-intersection branch from 75fc29f to 2d7dd24 Compare April 18, 2018 19:43

danesfeder merged commit 2387ad5 into master Apr 18, 2018

danesfeder deleted the dan-add-rp-coordinate-intersection branch April 18, 2018 20:05

Guardiola31337 mentioned this pull request Apr 19, 2018

Use BannerText roundabout degrees + Banner and Voice Milestone Tests #854

Merged

2 tasks

danesfeder mentioned this pull request May 3, 2018

Release v0.13.0 #909

Closed

13 tasks

		@@ -141,14 +141,9 @@ private Location buildLocationUpdate(double lng, double lat, long time) {

	protected Location buildLocationUpdate(double lng, double lat, float speed, float horizontalAccuracy, long time) {
	Location location = mock(Location.class);
	when(location.getLongitude()).thenReturn(lng);
	when(location.getLatitude()).thenReturn(lat);
	when(location.getSpeed()).thenReturn(speed);
	when(location.getAccuracy()).thenReturn(horizontalAccuracy);
	when(location.getTime()).thenReturn(time);
	return location;
	}

		@@ -15,6 +29,7 @@
		public static final double LARGE_DELTA = 0.1;

		@@ -324,28 +317,16 @@ public void setNavigationMetricListener_didNotGetTriggeredUntilArrival() throws
		navigationEventDispatcher.addMetricArrivalListener(arrivalListener);

		// Progress that hasn't arrived

		@@ -240,9 +495,48 @@ static Point nextManeuverPosition(int stepIndex, List<LegStep> steps, List<Point
		return !coords.isEmpty() ? coords.get(coords.size() - 1) : coords.get(coords.size());


		indices = NavigationIndices.create(FIRST_LEG_INDEX, FIRST_STEP_INDEX);
		processNewIndex(mapboxNavigation);


		import static okhttp3.internal.Util.UTF_8;

		class TestRouteBuilder {

Upgrade RouteProgress Step Data #812

Upgrade RouteProgress Step Data #812

Conversation

danesfeder commented Mar 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danesfeder commented Apr 9, 2018

danesfeder commented Apr 10, 2018

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

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

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

danesfeder commented Apr 17, 2018

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

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

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

danesfeder commented Mar 29, 2018 •

edited

Loading