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

danesfeder · 2018-04-11T21:40:38Z

The directions API now provides a float degrees in BannerText indicating the where you will be exiting the roundabout. This PR takes this value from the API and uses it to draw the roundabout in the ManeuverView dynamically.

This is also a good opportunity to clean up some of the model work. Currently in a bit of a 🍝 state. Going to exercise the boyscout rule and clean it up a bit in this PR.

TODO:

VoiceInstructionsMilestoneTest
BannerInstructionsMilestoneTest

danesfeder · 2018-04-17T17:26:20Z

@devotaaabel @Guardiola31337 tests added, this PR is good to go for 👀

Danny-James · 2018-04-19T10:08:24Z

@danesfeder Do we know when this will be complete? I'm just waiting on the stable v13 👍

Guardiola31337

Nice test coverage @danesfeder ❤️ it!

This is looking great. A few comments / cleanup.

Guardiola31337 · 2018-04-19T12:19:26Z

...src/main/java/com/mapbox/services/android/navigation/ui/v5/instruction/InstructionModel.java

 public class InstructionModel {

  private BannerText primaryBannerText;
  private BannerText secondaryBannerText;
  private BannerText thenBannerText;
+  private Float roundaboutAngle = null;
  private InstructionStepResources stepResources;
  private RouteProgress progress;
  private Locale locale;


Minor thing annotations normally are on top of the definition like 👇

@NavigationUnitType.UnitType private int unitType;

👇

Guardiola31337 · 2018-04-19T12:33:40Z

...ain/java/com/mapbox/services/android/navigation/ui/v5/instruction/maneuver/ManeuverView.java

@@ -135,7 +135,7 @@ protected void onDraw(Canvas canvas) {
      case STEP_MANEUVER_TYPE_ROUNDABOUT:


What about decomposing the switch somehow? It's too long... There's even another switch in one of the branches 😅

Guardiola31337 · 2018-04-19T12:35:29Z

...est/java/com/mapbox/services/android/navigation/ui/v5/NavigationViewEventDispatcherTest.java


 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;

-@RunWith(RobolectricTestRunner.class)
-@Config(constants = com.mapbox.services.android.navigation.BuildConfig.class, manifest = Config.DEFAULT_MANIFEST_NAME)
 public class NavigationViewEventDispatcherTest {


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

In general, we should avoid inline setup 👀 https://speakerdeck.com/guardiola31337/elegant-unit-testing-mobilization-2016?slide=65
Could you extract this block of code into private "factory" methods and use them only in the tests that actually need it/across the rest of the tests?
IMO, it reflects better what's going on in the test and you don't break the triple A "rule".

Guardiola31337 · 2018-04-19T12:41:44Z

...ain/java/com/mapbox/services/android/navigation/v5/milestone/BannerInstructionMilestone.java

-    return validInstructions(stepBannerInstructions)
-      && validInstructions(instructions)
-      && instructions.get(0).equals(stepBannerInstructions.get(0));
+  private boolean isFirstInstruction(BannerInstructions instructions) {


Why not inlining it and use an explaining variable as with the rest of the booleans in shouldBeShown?

Guardiola31337 · 2018-04-19T12:42:39Z

...main/java/com/mapbox/services/android/navigation/v5/milestone/VoiceInstructionMilestone.java

@@ -64,7 +55,10 @@ public String buildInstruction(RouteProgress routeProgress) {
   * @since 0.8.0
   */
  public String getSsmlAnnouncement() {
-    return ssmlAnnouncement;
+    if (instructions == null) {
+      return "";


NIT magic number

Guardiola31337 · 2018-04-19T13:03:11Z

.../java/com/mapbox/services/android/navigation/v5/milestone/VoiceInstructionMilestoneTest.java

+  public void onNullMilestoneInstructions_emptyInstructionsAreReturned() throws Exception {
+    VoiceInstructionMilestone milestone = buildVoiceInstructionMilestone();
+
+    assertEquals("", milestone.getAnnouncement());


Generally is preferable having only 1 assertion per test 👀 https://speakerdeck.com/guardiola31337/elegant-unit-testing-mobilization-2016?slide=57
What's the value of adding assertEquals("", milestone.getSsmlAnnouncement()); if you're already testing it in onNullMilestoneInstructions_emptySsmlInstructionsAreReturned?

Guardiola31337 · 2018-04-19T13:07:49Z

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

 import static junit.framework.Assert.assertEquals;
 import static junit.framework.Assert.assertNotNull;
 import static junit.framework.Assert.assertNotSame;
 import static junit.framework.Assert.assertNull;
-import static junit.framework.Assert.assertTrue;

 public class RouteLegProgressTest extends BaseTest {


👀 #812 (comment)

👀 asserts inside a for loop are weird. What are we trying to test in fractionTraveled_equalsCorrectValueAtIntervals test?

Guardiola31337 · 2018-04-19T13:13:19Z

...src/test/java/com/mapbox/services/android/navigation/v5/routeprogress/RouteProgressTest.java

+    double durationRemaining = route.duration();
+    double progressDurationRemaining = beginningRouteProgress.durationRemaining();
+
+    assertEquals(durationRemaining, progressDurationRemaining, BaseTest.DELTA);
  }



👀 asserts inside a for loop are weird. What are we trying to test in fractionTraveled_equalsCorrectValueAtIntervals and multiLeg_getFractionTraveled_equalsCorrectValueAtIntervals tests?

👀 #812 (comment)

Guardiola31337 · 2018-04-19T13:18:54Z

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

@@ -242,19 +242,23 @@ public void fractionTraveled_equalsOneAtEndOfStep() throws Exception {
  }



👀 #812 (comment)

👀 asserts inside a for loop are weird.

Guardiola31337 · 2018-04-19T13:21:24Z

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

+      currentStep, stepDistanceRemaining
+    );
+
+    assertEquals(null, currentBannerInstructions);


We could use assertNull instead.

The same thing happens with another tests included in RouteUtilsTest 👇

Guardiola31337

There are still some comments not addressed but this is in really good shape to 🚢

Great job @danesfeder

Loved how you completely removed that switch in ManeuverView 🙇

danesfeder added feature New feature request. navigation-ui labels Apr 11, 2018

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

danesfeder self-assigned this Apr 11, 2018

danesfeder requested review from Guardiola31337 and devotaaabel April 11, 2018 21:40

danesfeder force-pushed the dan-use-roundabout-degrees branch 2 times, most recently from fc70b86 to 77cb3ed Compare April 16, 2018 17:50

danesfeder changed the title ~~[WIP] Use BannerText roundabout degrees to draw roundabout maneuver~~ Use BannerText roundabout degrees + Banner and Voice Milestone Tests Apr 17, 2018

danesfeder force-pushed the dan-use-roundabout-degrees branch from 3c96e97 to a730bcf Compare April 17, 2018 18:38

danesfeder added 7 commits April 18, 2018 16:08

Use BannerText roundabout degrees to draw roundabout maneuver

dea70a7

Begin work on new method to find current BannerText

3e29157

Working findBannerText function

597926d

Working utils for voice and banner instructions

99b281c

Uncomment banner update

86b7bc0

Use RouteUtil in BannerInstructionMilestone

92d5007

Update text fixtures and add RouteUtils tests

3685eb1

danesfeder force-pushed the dan-use-roundabout-degrees branch from 6bcd008 to 7f3b205 Compare April 18, 2018 20:44

Guardiola31337 reviewed Apr 19, 2018

View reviewed changes

danesfeder force-pushed the dan-use-roundabout-degrees branch from 7f3b205 to 8d04e83 Compare April 19, 2018 18:36

Add Banner and Voice instruction milestone tests

4d7f353

danesfeder force-pushed the dan-use-roundabout-degrees branch from 8d04e83 to 4d7f353 Compare April 19, 2018 18:39

Guardiola31337 approved these changes Apr 19, 2018

View reviewed changes

danesfeder merged commit f43d662 into master Apr 19, 2018

danesfeder deleted the dan-use-roundabout-degrees branch April 19, 2018 19:12

danesfeder mentioned this pull request May 3, 2018

Release v0.13.0 #909

Closed

13 tasks

danesfeder mentioned this pull request Jun 5, 2018

Android: Incorrect Direction Images #396

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

danesfeder commented Apr 11, 2018 •

edited

Loading

danesfeder commented Apr 17, 2018

Danny-James commented Apr 19, 2018

Guardiola31337 left a comment

Guardiola31337 Apr 19, 2018

Guardiola31337 Apr 19, 2018

Guardiola31337 Apr 19, 2018

Guardiola31337 Apr 19, 2018

Guardiola31337 Apr 19, 2018

Guardiola31337 Apr 19, 2018

Guardiola31337 Apr 19, 2018

Guardiola31337 Apr 19, 2018

Guardiola31337 Apr 19, 2018

Guardiola31337 Apr 19, 2018

Guardiola31337 Apr 19, 2018

Guardiola31337 Apr 19, 2018

Guardiola31337 Apr 19, 2018

Guardiola31337 Apr 19, 2018

Guardiola31337 Apr 19, 2018

Guardiola31337 left a comment

		@@ -135,7 +135,7 @@ protected void onDraw(Canvas canvas) {
		case STEP_MANEUVER_TYPE_ROUNDABOUT:

		@@ -242,19 +242,23 @@ public void fractionTraveled_equalsOneAtEndOfStep() throws Exception {
		}

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

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

Conversation

danesfeder commented Apr 11, 2018 • edited Loading

danesfeder commented Apr 17, 2018

Danny-James commented Apr 19, 2018

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

Guardiola31337 left a comment

Choose a reason for hiding this comment

danesfeder commented Apr 11, 2018 •

edited

Loading