Skip to content

Commit

Permalink
NAVAND-1472: use a descriptive sealed class instead of Expected
Browse files Browse the repository at this point in the history
  • Loading branch information
dzinad committed Sep 19, 2023
1 parent 4b091b9 commit 3c74d94
Show file tree
Hide file tree
Showing 8 changed files with 96 additions and 54 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ internal class ImmediateRouteRefreshController(
@Throws(IllegalArgumentException::class)
fun requestRoutesRefresh(
routes: List<NavigationRoute>,
callback: (Expected<String, RoutesRefresherResult>) -> Unit
callback: (RoutesRefresherExecutorResult) -> Unit
) {
if (routes.isEmpty()) {
throw IllegalArgumentException("Routes to refresh should not be empty")
Expand All @@ -39,18 +39,24 @@ internal class ImmediateRouteRefreshController(
}

callback(result)
result.fold(
{ logW("Route refresh on-demand error: $it", RouteRefreshLog.LOG_CATEGORY) },
{
attemptListener.onRoutesRefreshAttemptFinished(it)
if (it.anySuccess()) {
when (result) {
is RoutesRefresherExecutorResult.ReplacedByNewer -> {
logW(
"Route refresh on-demand error: " +
"request is skipped as a newer one is available",
RouteRefreshLog.LOG_CATEGORY
)
}
is RoutesRefresherExecutorResult.Finished -> {
attemptListener.onRoutesRefreshAttemptFinished(result.value)
if (result.value.anySuccess()) {
stateHolder.onSuccess()
} else {
stateHolder.onFailure(null)
}
listener.onRoutesRefreshed(it)
listener.onRoutesRefreshed(result.value)
}
)
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -141,28 +141,40 @@ internal class PlannedRouteRefreshController @VisibleForTesting constructor(
}
}
)
routeRefresherResult.fold(
{ logW("Planned route refresh error: $it", RouteRefreshLog.LOG_CATEGORY) },
{
attemptListener.onRoutesRefreshAttemptFinished(it)
if (it.anySuccess()) {
when (routeRefresherResult) {
is RoutesRefresherExecutorResult.ReplacedByNewer -> {
logW(
"Planned route refresh error: " +
"request is skipped as a newer one is available",
RouteRefreshLog.LOG_CATEGORY
)
}
is RoutesRefresherExecutorResult.Finished -> {
attemptListener.onRoutesRefreshAttemptFinished(routeRefresherResult.value)
if (routeRefresherResult.value.anySuccess()) {
stateHolder.onSuccess()
listener.onRoutesRefreshed(it)
val refreshedRoutes = listOf(it.primaryRouteRefresherResult.route) +
it.alternativesRouteRefresherResults.map { it.route }
listener.onRoutesRefreshed(routeRefresherResult.value)
val refreshedRoutes = listOf(
routeRefresherResult.value.primaryRouteRefresherResult.route
) + routeRefresherResult.value.alternativesRouteRefresherResults.map {
it.route
}
routesToRefresh = refreshedRoutes
scheduleNewUpdate(refreshedRoutes, false)
} else {
if (it.anyRequestFailed() && retryStrategy.shouldRetry()) {
if (
routeRefresherResult.value.anyRequestFailed() &&
retryStrategy.shouldRetry()
) {
scheduleUpdateRetry(routes, shouldNotifyOnStart = false)
} else {
stateHolder.onFailure(null)
listener.onRoutesRefreshed(it)
listener.onRoutesRefreshed(routeRefresherResult.value)
scheduleNewUpdate(routes, false)
}
}
}
)
}
}

private fun recreateScope() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ class RouteRefreshController internal constructor(
}
plannedRouteRefreshController.pause()
immediateRouteRefreshController.requestRoutesRefresh(routes) {
if (it.isValue) {
if (it is RoutesRefresherExecutorResult.Finished) {
plannedRouteRefreshController.resume()
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,17 +1,22 @@
package com.mapbox.navigation.core.routerefresh

import com.mapbox.bindgen.Expected
import com.mapbox.bindgen.ExpectedFactory
import com.mapbox.navigation.base.route.NavigationRoute
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.launch
import kotlinx.coroutines.suspendCancellableCoroutine
import kotlin.coroutines.resume

internal sealed class RoutesRefresherExecutorResult {

internal data class Finished(val value: RoutesRefresherResult) : RoutesRefresherExecutorResult()

internal object ReplacedByNewer : RoutesRefresherExecutorResult()
}

private data class QueuedRequest(
val routes: List<NavigationRoute>,
val startCallback: () -> Unit,
val finishCallback: (Expected<String, RoutesRefresherResult>) -> Unit,
val finishCallback: (RoutesRefresherExecutorResult) -> Unit,
)

internal class RouteRefresherExecutor(
Expand All @@ -26,7 +31,7 @@ internal class RouteRefresherExecutor(
suspend fun executeRoutesRefresh(
routes: List<NavigationRoute>,
startCallback: () -> Unit,
): Expected<String, RoutesRefresherResult> = suspendCancellableCoroutine { cont ->
): RoutesRefresherExecutorResult = suspendCancellableCoroutine { cont ->
cont.invokeOnCancellation {
currentRequest = null
queuedRequest = null
Expand All @@ -39,10 +44,10 @@ internal class RouteRefresherExecutor(
private fun executeRoutesRefresh(
routes: List<NavigationRoute>,
startCallback: () -> Unit,
finishCallback: (Expected<String, RoutesRefresherResult>) -> Unit,
finishCallback: (RoutesRefresherExecutorResult) -> Unit,
) {
queuedRequest?.finishCallback?.invoke(
ExpectedFactory.createError("Skipping request as a newer one is queued.")
RoutesRefresherExecutorResult.ReplacedByNewer
)
queuedRequest = QueuedRequest(routes, startCallback, finishCallback)
runQueue()
Expand All @@ -60,7 +65,7 @@ internal class RouteRefresherExecutor(
currentRequest = null
}
runQueue()
localCurrentRequest.finishCallback(ExpectedFactory.createValue(result))
localCurrentRequest.finishCallback(RoutesRefresherExecutorResult.Finished(result))
}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
package com.mapbox.navigation.core.routerefresh

import com.mapbox.bindgen.Expected
import com.mapbox.bindgen.ExpectedFactory
import com.mapbox.navigation.base.ExperimentalPreviewMapboxNavigationAPI
import com.mapbox.navigation.base.route.NavigationRoute
import com.mapbox.navigation.testing.LoggingFrontendTestRule
Expand Down Expand Up @@ -34,7 +32,7 @@ class ImmediateRouteRefreshControllerTest {
private val attemptListener = mockk<RoutesRefreshAttemptListener>(relaxed = true)
private val listener = mockk<RouteRefresherListener>(relaxed = true)
private val clientCallback =
mockk<(Expected<String, RoutesRefresherResult>) -> Unit>(relaxed = true)
mockk<(RoutesRefresherExecutorResult) -> Unit>(relaxed = true)
private val routes = listOf<NavigationRoute>(mockk())

private val sut = ImmediateRouteRefreshController(
Expand Down Expand Up @@ -74,14 +72,18 @@ class ImmediateRouteRefreshControllerTest {
val result = mockk<RoutesRefresherResult> { every { anySuccess() } returns true }
coEvery {
routeRefresherExecutor.executeRoutesRefresh(any(), any())
} returns ExpectedFactory.createValue(result)
} returns RoutesRefresherExecutorResult.Finished(result)

sut.requestRoutesRefresh(routes, clientCallback)

verify(exactly = 1) { attemptListener.onRoutesRefreshAttemptFinished(result) }
verify(exactly = 1) { stateHolder.onSuccess() }
verify(exactly = 1) { listener.onRoutesRefreshed(result) }
verify(exactly = 1) { clientCallback(match { it.value == result }) }
verify(exactly = 1) {
clientCallback(
match { (it as RoutesRefresherExecutorResult.Finished).value == result }
)
}
verify(exactly = 0) { stateHolder.onCancel() }
}

Expand All @@ -93,21 +95,22 @@ class ImmediateRouteRefreshControllerTest {
}
coEvery {
routeRefresherExecutor.executeRoutesRefresh(any(), any())
} returns ExpectedFactory.createValue(result)
} returns RoutesRefresherExecutorResult.Finished(result)

sut.requestRoutesRefresh(routes, clientCallback)

verify(exactly = 1) { attemptListener.onRoutesRefreshAttemptFinished(result) }
verify(exactly = 1) { stateHolder.onFailure(null) }
verify(exactly = 1) { clientCallback(match { it.value == result }) }
verify(exactly = 1) {
clientCallback(match { (it as RoutesRefresherExecutorResult.Finished).value == result })
}
verify(exactly = 1) { listener.onRoutesRefreshed(result) }
verify(exactly = 0) { stateHolder.onCancel() }
}

@Test
fun routesRefreshFinishedWithError() = coroutineRule.runBlockingTest {
val error: Expected<String, RoutesRefresherResult> =
ExpectedFactory.createError("Some error")
val error = RoutesRefresherExecutorResult.ReplacedByNewer
coEvery {
routeRefresherExecutor.executeRoutesRefresh(any(), any())
} returns error
Expand All @@ -124,7 +127,7 @@ class ImmediateRouteRefreshControllerTest {
verify(exactly = 1) { clientCallback.invoke(error) }
verify(exactly = 1) {
logger.logW(
"Route refresh on-demand error: Some error",
"Route refresh on-demand error: request is skipped as a newer one is available",
"RouteRefreshController"
)
}
Expand Down Expand Up @@ -169,7 +172,7 @@ class ImmediateRouteRefreshControllerTest {

coEvery {
routeRefresherExecutor.executeRoutesRefresh(any(), any())
} returns ExpectedFactory.createError("some error")
} returns RoutesRefresherExecutorResult.ReplacedByNewer
sut.requestRoutesRefresh(routes, clientCallback)

coVerify(exactly = 1) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -529,7 +529,7 @@ class PlannedRouteRefreshControllerTest {

sut.startRoutesRefreshing(routes)
clearMocks(retryStrategy, answers = false)
finishRequest(ExpectedFactory.createError("Some error"))
finishRequest(RoutesRefresherExecutorResult.ReplacedByNewer)

verify(exactly = 0) {
attemptListener.onRoutesRefreshAttemptFinished(any())
Expand All @@ -544,7 +544,10 @@ class PlannedRouteRefreshControllerTest {
executor.executeRoutesRefresh(any(), any())
}
verify(exactly = 1) {
logger.logW("Planned route refresh error: Some error", "RouteRefreshController")
logger.logW(
"Planned route refresh error: request is skipped as a newer one is available",
"RouteRefreshController"
)
}
}

Expand All @@ -556,10 +559,10 @@ class PlannedRouteRefreshControllerTest {
}

private suspend fun finishRequest(result: RoutesRefresherResult) {
finishRequest(ExpectedFactory.createValue(result))
finishRequest(RoutesRefresherExecutorResult.Finished(result))
}

private suspend fun finishRequest(result: Expected<String, RoutesRefresherResult>) {
private suspend fun finishRequest(result: RoutesRefresherExecutorResult) {
coEvery {
executor.executeRoutesRefresh(any(), any())
} returns result
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ class RouteRefreshControllerTest {
@Test
fun requestImmediateRouteRefreshWithNonEmptySuccess() {
val routes = listOf<NavigationRoute>(mockk())
val callback = slot<(Expected<String, RoutesRefresherResult>) -> Unit>()
val callback = slot<(RoutesRefresherExecutorResult) -> Unit>()
every { plannedRouteRefreshController.routesToRefresh } returns routes

sut.requestImmediateRouteRefresh()
Expand All @@ -183,7 +183,7 @@ class RouteRefreshControllerTest {
immediateRouteRefreshController.requestRoutesRefresh(routes, capture(callback))
}
callback.captured(
ExpectedFactory.createValue(
RoutesRefresherExecutorResult.Finished(
mockk { every { anySuccess() } returns true }
)
)
Expand All @@ -195,7 +195,7 @@ class RouteRefreshControllerTest {
@Test
fun requestImmediateRouteRefreshWithNonEmptyFailure() {
val routes = listOf<NavigationRoute>(mockk())
val callback = slot<(Expected<String, RoutesRefresherResult>) -> Unit>()
val callback = slot<(RoutesRefresherExecutorResult) -> Unit>()
every { plannedRouteRefreshController.routesToRefresh } returns routes

sut.requestImmediateRouteRefresh()
Expand All @@ -204,7 +204,7 @@ class RouteRefreshControllerTest {
immediateRouteRefreshController.requestRoutesRefresh(routes, capture(callback))
}
callback.captured(
ExpectedFactory.createValue(
RoutesRefresherExecutorResult.Finished(
mockk {
every { anySuccess() } returns false
}
Expand All @@ -218,15 +218,15 @@ class RouteRefreshControllerTest {
@Test
fun requestImmediateRouteRefreshWithNonEmptyError() {
val routes = listOf<NavigationRoute>(mockk())
val callback = slot<(Expected<String, RoutesRefresherResult>) -> Unit>()
val callback = slot<(RoutesRefresherExecutorResult) -> Unit>()
every { plannedRouteRefreshController.routesToRefresh } returns routes

sut.requestImmediateRouteRefresh()

verify(exactly = 1) {
immediateRouteRefreshController.requestRoutesRefresh(routes, capture(callback))
}
callback.captured(ExpectedFactory.createError("error"))
callback.captured(RoutesRefresherExecutorResult.ReplacedByNewer)
verify(exactly = 0) {
plannedRouteRefreshController.resume()
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import kotlinx.coroutines.async
import kotlinx.coroutines.delay
import kotlinx.coroutines.launch
import org.junit.Assert.assertEquals
import org.junit.Assert.assertTrue
import org.junit.Rule
import org.junit.Test

Expand Down Expand Up @@ -39,7 +40,10 @@ class RouteRefresherExecutorTest {
startCallback.invoke()
routeRefresher.refresh(routes, timeout)
}
assertEquals(routesRefresherResult, actual.value)
assertEquals(
routesRefresherResult,
(actual as RoutesRefresherExecutorResult.Finished).value
)
}

@Test
Expand Down Expand Up @@ -85,10 +89,16 @@ class RouteRefresherExecutorTest {
sut.executeRoutesRefresh(routes4, startCallback4)
}

assertEquals(routesRefresherResult, result1.await().value)
assertEquals("Skipping request as a newer one is queued.", result2.await().error)
assertEquals("Skipping request as a newer one is queued.", result3.await().error)
assertEquals(routesRefresherResult4, result4.await().value)
assertEquals(
routesRefresherResult,
(result1.await() as RoutesRefresherExecutorResult.Finished).value
)
assertTrue(result2.await() is RoutesRefresherExecutorResult.ReplacedByNewer)
assertTrue(result3.await() is RoutesRefresherExecutorResult.ReplacedByNewer)
assertEquals(
routesRefresherResult4,
(result4.await() as RoutesRefresherExecutorResult.Finished).value
)
}

@Test
Expand All @@ -104,6 +114,9 @@ class RouteRefresherExecutorTest {

val actual = sut.executeRoutesRefresh(routes, startCallback)

assertEquals(routesRefresherResult, actual.value)
assertEquals(
routesRefresherResult,
(actual as RoutesRefresherExecutorResult.Finished).value
)
}
}

0 comments on commit 3c74d94

Please sign in to comment.