Skip to content

Conversation

@danesfeder
Copy link
Contributor

@danesfeder danesfeder commented May 14, 2018

If default milestones are enabled (Voice/Banner instructions), we need to check for both in the RouteOptions before we proceed with processing. We are currently only checking for voice.

@danesfeder danesfeder added bug Defect to be fixed. navigation-core labels May 14, 2018
@danesfeder danesfeder added this to the 0.14.0 milestone May 14, 2018
@danesfeder danesfeder self-assigned this May 14, 2018
@danesfeder danesfeder force-pushed the dan-null-banner-voice branch from 9eaf8a2 to 7dfc5da Compare May 14, 2018 19:43
Copy link
Contributor

@Guardiola31337 Guardiola31337 left a comment

Choose a reason for hiding this comment

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

Minor comments not blocking the PR.

Good job @danesfeder

@Test(expected = MissingFormatArgumentException.class)
public void validDirectionsRoute_isInvalidWithNullRouteOptions() throws Exception {
DirectionsRoute route = buildTestDirectionsRoute(DIRECTIONS_WITHOUT_VOICE_INSTRUCTIONS);
route = route.toBuilder().routeOptions(null).build();
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 extracting the null into an explaining variable to make clear what we're specifically testing?


@Test(expected = MissingFormatArgumentException.class)
public void validDirectionsRoute_isInvalidWithNullInstructions() throws Exception {
DirectionsRoute route = buildRouteWithNullInstructions();
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 renaming route to make clear what we're specifically testing?


@Test(expected = MissingFormatArgumentException.class)
public void validDirectionsRoute_isInvalidWithFalseVoiceInstructions() throws Exception {
DirectionsRoute route = buildRouteWithFalseVoiceInstructions();
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 ☝️


@Test(expected = MissingFormatArgumentException.class)
public void validDirectionsRoute_isInvalidWithFalseBannerInstructions() throws Exception {
DirectionsRoute route = buildRouteWithFalseBannerInstructions();
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 ☝️

@danesfeder danesfeder force-pushed the dan-null-banner-voice branch 2 times, most recently from ce54d52 to 6466f28 Compare May 15, 2018 13:08
@danesfeder
Copy link
Contributor Author

Holding on merging this until after I pair with @riastrad to 👀 how he's able to reproduce the crash in #930

@danesfeder danesfeder force-pushed the dan-null-banner-voice branch from 6466f28 to 9b3b5e6 Compare May 16, 2018 12:49
@danesfeder
Copy link
Contributor Author

Per voice with @riastrad, we are waiting on further detail from the customer that was producing this crash. Going to go ahead and merge this PR, without closing #930 and will open a new one if further details warrant a fix. @riastrad please update #930 if / when you hear back - thank you!

@danesfeder danesfeder merged commit 159f166 into master May 16, 2018
@danesfeder danesfeder deleted the dan-null-banner-voice branch May 16, 2018 13:20
@danesfeder danesfeder mentioned this pull request May 31, 2018
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Defect to be fixed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants