Check for null camera engine before returning from MapboxNavigation #866

danesfeder · 2018-04-16T21:00:42Z

Closes #842

Always return default SimpleCamera engine if one hasn't already been initialized.

Took a moment to clean up MapboxNavigationTest as well.

Guardiola31337

Great you took the opportunity to refactor MapboxNavigationTest 🙇

Well done @danesfeder ❤️ it!

A few comments / cleanup not blocking the PR.

Guardiola31337 · 2018-04-17T16:37:17Z

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

-  public void before() throws Exception {
-    navigation = new MapboxNavigation(mock(Context.class), ACCESS_TOKEN, mock(NavigationTelemetry.class),
-      mock(LocationEngine.class));
+    assertNotNull("should not be null", navigation);


IMO Adding error messages to tests is kind of confusing because when you 👀 a test using JUnit you think always as expected and actual values being the first parameters and in this case "should not be null" is not the expected value is actually the error message shown when the test fails (which is not giving any extra value because if it fails you're going to get ❌)
I'd rather working on improving the name of the test if needed in these cases.

Guardiola31337 · 2018-04-17T16:37:32Z

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

-    MapboxNavigation navigationWithOptions = new MapboxNavigation(mock(Context.class),
-      ACCESS_TOKEN, options, mock(NavigationTelemetry.class), mock(LocationEngine.class));
+    MapboxNavigation navigationWithOptions = buildMapboxNavigationWithOptions(options);
+
    assertNotNull("should not be null", navigationWithOptions);


Same here ☝️

Guardiola31337 · 2018-04-17T17:02:43Z

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

    assertNotNull("should not be null", navigationWithOptions);
  }

  @Test
  public void voiceMilestone_onInitializationDoesGetAdded() throws Exception {
+    MapboxNavigation navigation = buildMapboxNavigation();
+
    assertTrue(navigation.getMilestones().get(0).getIdentifier() == VOICE_INSTRUCTION_MILESTONE_ID);


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

Here we're including the Act inside the Assert. It'd be great to have them separated and take advantage of giving great names to variables so it's easy to understand what's going on.

This applies to other tests included in MapboxNavigationTest.

Guardiola31337 · 2018-04-17T17:06:32Z

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

  }

  @Test
  public void addMilestone_milestoneDidGetAdded() throws Exception {
+    MapboxNavigation navigation = buildMapboxNavigation();
+
    Milestone milestone = new StepMilestone.Builder().build();


IMO These two lines of code belongs to the Arrange part.

This applies to other tests included in MapboxNavigationTest.

Guardiola31337 · 2018-04-17T17:09:58Z

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

    assertEquals(0, navigationWithOptions.getMilestones().size());
  }

  @Test
  public void removeMilestone_correctMilestoneWithIdentifierGetsRemoved() throws Exception {
    MapboxNavigationOptions options = MapboxNavigationOptions.builder().defaultMilestonesEnabled(false).build();
-    MapboxNavigation navigationWithOptions = new MapboxNavigation(mock(Context.class),
-      ACCESS_TOKEN, options, mock(NavigationTelemetry.class), mock(LocationEngine.class));
+    MapboxNavigation navigationWithOptions = buildMapboxNavigationWithOptions(options);
    Milestone milestone = new StepMilestone.Builder().setIdentifier(5678).build();


What about using a well-named variable for 5678 reflecting what's useful for here? Maybe theMilestone?

Guardiola31337 · 2018-04-17T17:10:48Z

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

    navigationWithOptions.addMilestone(new StepMilestone.Builder().build());
-    assertEquals(1, navigationWithOptions.getMilestones().size());
+
    navigationWithOptions.removeMilestone(5678);


Same here ☝️ but reflecting that the milestone was not found.

Guardiola31337 · 2018-04-17T17:13:12Z

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

    assertEquals(1, navigationWithOptions.getMilestones().size());
  }

  @Test
  public void getLocationEngine_returnsCorrectLocationEngine() throws Exception {
+    MapboxNavigation navigation = buildMapboxNavigation();
    LocationEngine locationEngine = mock(LocationEngine.class);
    LocationEngine locationEngine2 = mock(LocationEngine.class);


We should avoid adding "numbers" to variable names. Could we refactor-rename adding more context so it'll be easier to read and understand what's going on in the test?

danesfeder · 2018-04-17T17:36:04Z

@Guardiola31337 addressed, thanks!

Guardiola31337

@danesfeder Some minor comments to take into account in the future when working again with MapboxNavigationTest.

Guardiola31337 · 2018-04-18T08:33:20Z

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

+
+    int identifier = navigation.getMilestones().get(0).getIdentifier();
+
+    assertEquals(identifier, VOICE_INSTRUCTION_MILESTONE_ID);


This should be the other way around, "expected" value 👉 VOICE_INSTRUCTION_MILESTONE_ID which would be better represented using the literal value instead of using the constant (👀 Expect Literals), "actual" value 👉 identifier.
The same thing happens in bannerMilestone_onInitializationDoesGetAdded test 👇

Guardiola31337 · 2018-04-18T08:37:09Z

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

-    MapboxNavigation navigationWithOptions = new MapboxNavigation(mock(Context.class),
-      ACCESS_TOKEN, options, mock(NavigationTelemetry.class), mock(LocationEngine.class));
+    MapboxNavigation navigationWithOptions = buildMapboxNavigationWithOptions(options);
+
    assertEquals(0, navigationWithOptions.getMilestones().size());


👀 #866 (comment)
navigationWithOptions.getMilestones().size() is the Act here.
This applies to other tests included in MapboxNavigationTest 👇

Guardiola31337 · 2018-04-18T08:40:53Z

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

-    MapboxNavigation navigationWithOptions = new MapboxNavigation(mock(Context.class),
-      ACCESS_TOKEN, options, mock(NavigationTelemetry.class), mock(LocationEngine.class));
+    MapboxNavigation navigationWithOptions = buildMapboxNavigationWithOptions(options);
+
    Milestone milestone = new StepMilestone.Builder().build();


Milestone milestone = new StepMilestone.Builder().build(); should be in the Arrange part.

Guardiola31337 · 2018-04-18T08:42:06Z

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

-    MapboxNavigation navigationWithOptions = new MapboxNavigation(mock(Context.class),
-      ACCESS_TOKEN, options, mock(NavigationTelemetry.class), mock(LocationEngine.class));
+    MapboxNavigation navigationWithOptions = buildMapboxNavigationWithOptions(options);
+
    Milestone milestone = new StepMilestone.Builder().build();


Milestone milestone = new StepMilestone.Builder().build(); navigationWithOptions.addMilestone(milestone);

Should be in the Arrange part.

Guardiola31337 · 2018-04-18T08:47:05Z

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

-    MapboxNavigation navigationWithOptions = new MapboxNavigation(mock(Context.class),
-      ACCESS_TOKEN, options, mock(NavigationTelemetry.class), mock(LocationEngine.class));
+    MapboxNavigation navigationWithOptions = buildMapboxNavigationWithOptions(options);
+
    Milestone milestone = new StepMilestone.Builder().build();


Milestone milestone = new StepMilestone.Builder().build(); navigationWithOptions.addMilestone(new StepMilestone.Builder().build());

Should be in the Arrange part.
We could take advantage of the naming here to understand clearer what's going on. Like naming new StepMilestone.Builder().build() as inexistentMilestone for example.

Guardiola31337 · 2018-04-18T08:56:41Z

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

-      ACCESS_TOKEN, options, mock(NavigationTelemetry.class), mock(LocationEngine.class));
-    Milestone milestone = new StepMilestone.Builder().setIdentifier(5678).build();
+    MapboxNavigation navigationWithOptions = buildMapboxNavigationWithOptions(options);
+    int removedMilestoneIdentifier = 5678;


IMO removedMilestoneIdentifier is not a good name here 👀 #866 (comment)

Guardiola31337 · 2018-04-18T08:59:01Z

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

    LocationEngine locationEngine = mock(LocationEngine.class);
-    LocationEngine locationEngine2 = mock(LocationEngine.class);
+    LocationEngine locationEngineInstanceNotUsed = mock(LocationEngine.class);
+
    navigation.setLocationEngine(locationEngine);


Should be in the Arrange part 👀 getLocationEngine_returnsCorrectLocationEngine is telling us what's the Act here getLocationEngine.

Guardiola31337 · 2018-04-18T08:59:16Z

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

    NavigationEventListener navigationEventListener = mock(NavigationEventListener.class);
+
    navigation.addNavigationEventListener(navigationEventListener);


Same here ☝️
This applies to other tests included in MapboxNavigationTest 👇

Guardiola31337 · 2018-04-18T09:01:17Z

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

    navigation.setSnapEngine(snap);
+
    assertTrue(!(navigation.getSnapEngine() instanceof SnapToRoute));


What about using assertFalse instead?

Guardiola31337 · 2018-04-18T09:01:29Z

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

    navigation.setOffRouteEngine(offRoute);
+
    assertTrue(!(navigation.getOffRouteEngine() instanceof OffRouteDetector));


Same here ☝️

danesfeder added bug Defect to be fixed. navigation-core labels Apr 16, 2018

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

danesfeder self-assigned this Apr 16, 2018

danesfeder requested review from Guardiola31337 and devotaaabel April 16, 2018 21:00

Guardiola31337 approved these changes Apr 17, 2018

View reviewed changes

Check for null camera engine before returning

602f589

danesfeder force-pushed the dan-camera-bug branch from 40e9e12 to 602f589 Compare April 17, 2018 17:33

Extract milestone ID into variable

4e9f914

danesfeder merged commit 9e95f4b into master Apr 17, 2018

danesfeder deleted the dan-camera-bug branch April 17, 2018 18:36

Guardiola31337 reviewed Apr 18, 2018

View reviewed changes

danesfeder mentioned this pull request May 3, 2018

Release v0.13.0 #909

Closed

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check for null camera engine before returning from MapboxNavigation #866

Check for null camera engine before returning from MapboxNavigation #866

danesfeder commented Apr 16, 2018 •

edited

Loading

Guardiola31337 left a comment

Guardiola31337 Apr 17, 2018

Guardiola31337 Apr 17, 2018

Guardiola31337 Apr 17, 2018

Guardiola31337 Apr 17, 2018

Guardiola31337 Apr 17, 2018

Guardiola31337 Apr 17, 2018

Guardiola31337 Apr 17, 2018

danesfeder commented Apr 17, 2018

Guardiola31337 left a comment

Guardiola31337 Apr 18, 2018

Guardiola31337 Apr 18, 2018

Guardiola31337 Apr 18, 2018

Guardiola31337 Apr 18, 2018

Guardiola31337 Apr 18, 2018

Guardiola31337 Apr 18, 2018

Guardiola31337 Apr 18, 2018

Guardiola31337 Apr 18, 2018

Guardiola31337 Apr 18, 2018

Guardiola31337 Apr 18, 2018


		int identifier = navigation.getMilestones().get(0).getIdentifier();

		assertEquals(identifier, VOICE_INSTRUCTION_MILESTONE_ID);

		NavigationEventListener navigationEventListener = mock(NavigationEventListener.class);

		navigation.addNavigationEventListener(navigationEventListener);

		navigation.setSnapEngine(snap);

		assertTrue(!(navigation.getSnapEngine() instanceof SnapToRoute));

		navigation.setOffRouteEngine(offRoute);

		assertTrue(!(navigation.getOffRouteEngine() instanceof OffRouteDetector));

Check for null camera engine before returning from MapboxNavigation #866

Check for null camera engine before returning from MapboxNavigation #866

Conversation

danesfeder commented Apr 16, 2018 • edited Loading

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

danesfeder commented Apr 17, 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

danesfeder commented Apr 16, 2018 •

edited

Loading