Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Check for null camera engine before returning from MapboxNavigation #866

Merged
merged 2 commits into from
Apr 17, 2018
Merged

Check for null camera engine before returning from MapboxNavigation #866

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
602f589
Check for null camera engine before returning
danesfeder Apr 16, 2018
4e9f914
Extract milestone ID into variable
danesfeder Apr 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .../src/main/java/com/mapbox/services/android/navigation/v5/navigation/MapboxNavigation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,11 @@ public void setCameraEngine(@NonNull Camera cameraEngine) {
* @return camera engine used to configure camera position while routing
* @since 0.10.0
*/
@NonNull
public Camera getCameraEngine() {
if (cameraEngine == null) {
return new SimpleCamera();
}
return cameraEngine;
}

Expand Down
168 changes: 113 additions & 55 deletions .../test/java/com/mapbox/services/android/navigation/v5/navigation/MapboxNavigationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,13 @@
import com.mapbox.services.android.navigation.v5.BaseTest;
import com.mapbox.services.android.navigation.v5.milestone.Milestone;
import com.mapbox.services.android.navigation.v5.milestone.StepMilestone;
import com.mapbox.services.android.navigation.v5.navigation.camera.SimpleCamera;
import com.mapbox.services.android.navigation.v5.offroute.OffRoute;
import com.mapbox.services.android.navigation.v5.offroute.OffRouteDetector;
import com.mapbox.services.android.navigation.v5.snap.Snap;
import com.mapbox.services.android.navigation.v5.snap.SnapToRoute;
import com.mapbox.services.android.telemetry.location.LocationEngine;

import org.junit.Before;
import org.junit.Test;

import static com.mapbox.services.android.navigation.v5.navigation.NavigationConstants.BANNER_INSTRUCTION_MILESTONE_ID;
Expand All @@ -28,199 +28,257 @@

public class MapboxNavigationTest extends BaseTest {

private MapboxNavigation navigation;
@Test
public void sanityTest() {
MapboxNavigation navigation = buildMapboxNavigation();

@Before
public void before() throws Exception {
navigation = new MapboxNavigation(mock(Context.class), ACCESS_TOKEN, mock(NavigationTelemetry.class),
mock(LocationEngine.class));
assertNotNull(navigation);
}

@Test
public void sanityTest() {
assertNotNull("should not be null", navigation);
public void sanityTestWithOptions() {
MapboxNavigationOptions options = MapboxNavigationOptions.builder().build();
MapboxNavigation navigationWithOptions = new MapboxNavigation(mock(Context.class),
ACCESS_TOKEN, options, mock(NavigationTelemetry.class), mock(LocationEngine.class));
assertNotNull("should not be null", navigationWithOptions);
MapboxNavigation navigationWithOptions = buildMapboxNavigationWithOptions(options);

assertNotNull(navigationWithOptions);
}

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

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

assertEquals(identifier, VOICE_INSTRUCTION_MILESTONE_ID);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👇

}

@Test
public void bannerMilestone_onInitializationDoesGetAdded() throws Exception {
assertTrue(navigation.getMilestones().get(1).getIdentifier() == BANNER_INSTRUCTION_MILESTONE_ID);
MapboxNavigation navigation = buildMapboxNavigation();

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

assertEquals(identifier, BANNER_INSTRUCTION_MILESTONE_ID);
}


@Test
public void defaultMilestones_onInitializationDoNotGetAdded() 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);

assertEquals(0, navigationWithOptions.getMilestones().size());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

@Test
public void defaultEngines_didGetInitialized() throws Exception {
assertNotNull(navigation.getSnapEngine());
public void defaultEngines_offRouteEngineDidGetInitialized() throws Exception {
MapboxNavigation navigation = buildMapboxNavigation();

assertNotNull(navigation.getOffRouteEngine());
}

@Test
public void defaultEngines_snapEngineDidGetInitialized() throws Exception {
MapboxNavigation navigation = buildMapboxNavigation();

assertNotNull(navigation.getSnapEngine());
}

@Test
public void offRouteEngine_doesNotGetInitializedWithFalseOption() throws Exception {
MapboxNavigationOptions options = MapboxNavigationOptions.builder()
.enableOffRouteDetection(false)
.build();
navigation = new MapboxNavigation(mock(Context.class), ACCESS_TOKEN, options, mock(NavigationTelemetry.class),
mock(LocationEngine.class));
assertNull(navigation.getOffRouteEngine());
MapboxNavigation navigationWithOptions = buildMapboxNavigationWithOptions(options);

assertNull(navigationWithOptions.getOffRouteEngine());
}

@Test
public void snapToRouteEngine_doesNotGetInitializedWithFalseOption() throws Exception {
MapboxNavigationOptions options = MapboxNavigationOptions.builder()
.snapToRoute(false)
.build();
navigation = new MapboxNavigation(mock(Context.class), ACCESS_TOKEN, options, mock(NavigationTelemetry.class),
mock(LocationEngine.class));
assertNull(navigation.getSnapEngine());
MapboxNavigation navigationWithOptions = buildMapboxNavigationWithOptions(options);

assertNull(navigationWithOptions.getSnapEngine());
}

@Test
public void fasterRouteEngine_doesNotGetInitializedWithFalseOption() throws Exception {
MapboxNavigationOptions options = MapboxNavigationOptions.builder()
.enableFasterRouteDetection(false)
.build();
navigation = new MapboxNavigation(mock(Context.class), ACCESS_TOKEN, options, mock(NavigationTelemetry.class),
mock(LocationEngine.class));
assertNull(navigation.getFasterRouteEngine());
MapboxNavigation navigationWithOptions = buildMapboxNavigationWithOptions(options);

assertNull(navigationWithOptions.getFasterRouteEngine());
}

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

navigation.addMilestone(milestone);

assertTrue(navigation.getMilestones().contains(milestone));
}

@Test
public void addMilestone_milestoneOnlyGetsAddedOnce() 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().build();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

navigationWithOptions.addMilestone(milestone);
navigationWithOptions.addMilestone(milestone);

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

@Test
public void removeMilestone_milestoneDidGetRemoved() 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().build();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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

Should be in the Arrange part.

navigationWithOptions.addMilestone(milestone);
assertEquals(1, navigationWithOptions.getMilestones().size());
navigationWithOptions.removeMilestone(milestone);

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

@Test
public void removeMilestone_milestoneDoesNotExist() 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().build();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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.

navigationWithOptions.addMilestone(new StepMilestone.Builder().build());
navigationWithOptions.removeMilestone(milestone);

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

@Test
public void removeMilestone_nullRemovesAllMilestones() 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);
navigationWithOptions.addMilestone(new StepMilestone.Builder().build());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could extract this block of code into a private method called addABunchOfMilestones for example. It'd reflect better our intention and it'd be clearer and easier to understand.

navigationWithOptions.addMilestone(new StepMilestone.Builder().build());
navigationWithOptions.addMilestone(new StepMilestone.Builder().build());
navigationWithOptions.addMilestone(new StepMilestone.Builder().build());
assertEquals(4, navigationWithOptions.getMilestones().size());

navigationWithOptions.removeMilestone(null);

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));
Milestone milestone = new StepMilestone.Builder().setIdentifier(5678).build();
MapboxNavigation navigationWithOptions = buildMapboxNavigationWithOptions(options);
int removedMilestoneIdentifier = 5678;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Milestone milestone = new StepMilestone.Builder().setIdentifier(removedMilestoneIdentifier).build();
navigationWithOptions.addMilestone(milestone);
assertEquals(1, navigationWithOptions.getMilestones().size());
navigationWithOptions.removeMilestone(5678);

navigationWithOptions.removeMilestone(removedMilestoneIdentifier);

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

@Test
public void removeMilestone_noMilestoneWithIdentifierFound() 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);
navigationWithOptions.addMilestone(new StepMilestone.Builder().build());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could take advantage of the naming here to understand clearer what's going on. Like naming new StepMilestone.Builder().build() as aMilestone for example and rename removedMilestoneIdentifier with inexistentMilestoneIdentifier for example.

assertEquals(1, navigationWithOptions.getMilestones().size());
navigationWithOptions.removeMilestone(5678);
int removedMilestoneIdentifier = 5678;

navigationWithOptions.removeMilestone(removedMilestoneIdentifier);

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);
LocationEngine locationEngineInstanceNotUsed = mock(LocationEngine.class);

navigation.setLocationEngine(locationEngine);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

assertNotSame(locationEngine2, navigation.getLocationEngine());

assertNotSame(locationEngineInstanceNotUsed, navigation.getLocationEngine());
assertEquals(locationEngine, navigation.getLocationEngine());
}

@Test
public void endNavigation_doesSendFalseToNavigationEvent() throws Exception {
MapboxNavigation navigation = buildMapboxNavigation();
NavigationEventListener navigationEventListener = mock(NavigationEventListener.class);

navigation.addNavigationEventListener(navigationEventListener);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

navigation.startNavigation(mock(DirectionsRoute.class));
navigation.endNavigation();

verify(navigationEventListener, times(1)).onRunning(false);
}

@Test
public void startNavigation_doesSendTrueToNavigationEvent() throws Exception {
MapboxNavigation navigation = buildMapboxNavigation();
NavigationEventListener navigationEventListener = mock(NavigationEventListener.class);

navigation.addNavigationEventListener(navigationEventListener);
navigation.startNavigation(mock(DirectionsRoute.class));

verify(navigationEventListener, times(1)).onRunning(true);
}

@Test
public void setSnapEngine_doesReplaceDefaultEngine() throws Exception {
Snap snap = navigation.getSnapEngine();
assertTrue(snap instanceof SnapToRoute);
snap = mock(Snap.class);
MapboxNavigation navigation = buildMapboxNavigation();

Snap snap = mock(Snap.class);
navigation.setSnapEngine(snap);

assertTrue(!(navigation.getSnapEngine() instanceof SnapToRoute));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about using assertFalse instead?

assertTrue(navigation.getSnapEngine() instanceof Snap);
}

@Test
public void setOffRouteEngine_doesReplaceDefaultEngine() throws Exception {
OffRoute offRoute = navigation.getOffRouteEngine();
assertTrue(offRoute instanceof OffRouteDetector);
offRoute = mock(OffRoute.class);
MapboxNavigation navigation = buildMapboxNavigation();

OffRoute offRoute = mock(OffRoute.class);
navigation.setOffRouteEngine(offRoute);

assertTrue(!(navigation.getOffRouteEngine() instanceof OffRouteDetector));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here ☝️

assertTrue(navigation.getOffRouteEngine() instanceof OffRoute);
}

@Test
public void getCameraEngine_returnsNonNullEngine() throws Exception {
MapboxNavigation navigation = buildMapboxNavigation();

navigation.setOffRouteEngine(null);

assertNotNull(navigation.getCameraEngine());
}

@Test
public void getCameraEngine_returnsSimpleCameraWhenNull() throws Exception {
MapboxNavigation navigation = buildMapboxNavigation();

navigation.setOffRouteEngine(null);

assertTrue(navigation.getCameraEngine() instanceof SimpleCamera);
}

private MapboxNavigation buildMapboxNavigation() {
return new MapboxNavigation(mock(Context.class), ACCESS_TOKEN, mock(NavigationTelemetry.class),
mock(LocationEngine.class));
}

private MapboxNavigation buildMapboxNavigationWithOptions(MapboxNavigationOptions options) {
return new MapboxNavigation(mock(Context.class), ACCESS_TOKEN, options, mock(NavigationTelemetry.class),
mock(LocationEngine.class));
}
}