From 02614f5505f9bda368dfdfaa5289180590aa8298 Mon Sep 17 00:00:00 2001 From: Dzina Date: Tue, 19 Sep 2023 18:06:20 +0300 Subject: [PATCH] NAVAND-1472: use a descriptive sealed class instead of Expected --- .../ImmediateRouteRefreshController.kt | 23 ++++++++----- .../PlannedRouteRefreshController.kt | 34 +++++++++++++------ .../routerefresh/RouteRefreshController.kt | 2 +- .../routerefresh/RouteRefresherExecutor.kt | 19 +++++++---- .../ImmediateRouteRefreshControllerTest.kt | 25 ++++++++------ .../PlannedRouteRefreshControllerTest.kt | 13 +++---- .../RouteRefreshControllerTest.kt | 14 ++++---- .../RouteRefresherExecutorTest.kt | 25 ++++++++++---- 8 files changed, 96 insertions(+), 59 deletions(-) diff --git a/libnavigation-core/src/main/java/com/mapbox/navigation/core/routerefresh/ImmediateRouteRefreshController.kt b/libnavigation-core/src/main/java/com/mapbox/navigation/core/routerefresh/ImmediateRouteRefreshController.kt index 44e695c0337..aa7f8736223 100644 --- a/libnavigation-core/src/main/java/com/mapbox/navigation/core/routerefresh/ImmediateRouteRefreshController.kt +++ b/libnavigation-core/src/main/java/com/mapbox/navigation/core/routerefresh/ImmediateRouteRefreshController.kt @@ -1,6 +1,5 @@ package com.mapbox.navigation.core.routerefresh -import com.mapbox.bindgen.Expected import com.mapbox.navigation.base.ExperimentalPreviewMapboxNavigationAPI import com.mapbox.navigation.base.route.NavigationRoute import com.mapbox.navigation.utils.internal.logW @@ -22,7 +21,7 @@ internal class ImmediateRouteRefreshController( @Throws(IllegalArgumentException::class) fun requestRoutesRefresh( routes: List, - callback: (Expected) -> Unit + callback: (RoutesRefresherExecutorResult) -> Unit ) { if (routes.isEmpty()) { throw IllegalArgumentException("Routes to refresh should not be empty") @@ -39,18 +38,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) } - ) + } } } diff --git a/libnavigation-core/src/main/java/com/mapbox/navigation/core/routerefresh/PlannedRouteRefreshController.kt b/libnavigation-core/src/main/java/com/mapbox/navigation/core/routerefresh/PlannedRouteRefreshController.kt index 3de6956489d..48468199ed2 100644 --- a/libnavigation-core/src/main/java/com/mapbox/navigation/core/routerefresh/PlannedRouteRefreshController.kt +++ b/libnavigation-core/src/main/java/com/mapbox/navigation/core/routerefresh/PlannedRouteRefreshController.kt @@ -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() { diff --git a/libnavigation-core/src/main/java/com/mapbox/navigation/core/routerefresh/RouteRefreshController.kt b/libnavigation-core/src/main/java/com/mapbox/navigation/core/routerefresh/RouteRefreshController.kt index 3f0a9ae8641..fd2190605a4 100644 --- a/libnavigation-core/src/main/java/com/mapbox/navigation/core/routerefresh/RouteRefreshController.kt +++ b/libnavigation-core/src/main/java/com/mapbox/navigation/core/routerefresh/RouteRefreshController.kt @@ -63,7 +63,7 @@ class RouteRefreshController internal constructor( } plannedRouteRefreshController.pause() immediateRouteRefreshController.requestRoutesRefresh(routes) { - if (it.isValue) { + if (it is RoutesRefresherExecutorResult.Finished) { plannedRouteRefreshController.resume() } } diff --git a/libnavigation-core/src/main/java/com/mapbox/navigation/core/routerefresh/RouteRefresherExecutor.kt b/libnavigation-core/src/main/java/com/mapbox/navigation/core/routerefresh/RouteRefresherExecutor.kt index 11a73988563..8ea2b76c425 100644 --- a/libnavigation-core/src/main/java/com/mapbox/navigation/core/routerefresh/RouteRefresherExecutor.kt +++ b/libnavigation-core/src/main/java/com/mapbox/navigation/core/routerefresh/RouteRefresherExecutor.kt @@ -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, val startCallback: () -> Unit, - val finishCallback: (Expected) -> Unit, + val finishCallback: (RoutesRefresherExecutorResult) -> Unit, ) internal class RouteRefresherExecutor( @@ -26,7 +31,7 @@ internal class RouteRefresherExecutor( suspend fun executeRoutesRefresh( routes: List, startCallback: () -> Unit, - ): Expected = suspendCancellableCoroutine { cont -> + ): RoutesRefresherExecutorResult = suspendCancellableCoroutine { cont -> cont.invokeOnCancellation { currentRequest = null queuedRequest = null @@ -39,10 +44,10 @@ internal class RouteRefresherExecutor( private fun executeRoutesRefresh( routes: List, startCallback: () -> Unit, - finishCallback: (Expected) -> 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() @@ -60,7 +65,7 @@ internal class RouteRefresherExecutor( currentRequest = null } runQueue() - localCurrentRequest.finishCallback(ExpectedFactory.createValue(result)) + localCurrentRequest.finishCallback(RoutesRefresherExecutorResult.Finished(result)) } } } diff --git a/libnavigation-core/src/test/java/com/mapbox/navigation/core/routerefresh/ImmediateRouteRefreshControllerTest.kt b/libnavigation-core/src/test/java/com/mapbox/navigation/core/routerefresh/ImmediateRouteRefreshControllerTest.kt index f526ac754d9..257414f4a45 100644 --- a/libnavigation-core/src/test/java/com/mapbox/navigation/core/routerefresh/ImmediateRouteRefreshControllerTest.kt +++ b/libnavigation-core/src/test/java/com/mapbox/navigation/core/routerefresh/ImmediateRouteRefreshControllerTest.kt @@ -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 @@ -34,7 +32,7 @@ class ImmediateRouteRefreshControllerTest { private val attemptListener = mockk(relaxed = true) private val listener = mockk(relaxed = true) private val clientCallback = - mockk<(Expected) -> Unit>(relaxed = true) + mockk<(RoutesRefresherExecutorResult) -> Unit>(relaxed = true) private val routes = listOf(mockk()) private val sut = ImmediateRouteRefreshController( @@ -74,14 +72,18 @@ class ImmediateRouteRefreshControllerTest { val result = mockk { 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() } } @@ -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 = - ExpectedFactory.createError("Some error") + val error = RoutesRefresherExecutorResult.ReplacedByNewer coEvery { routeRefresherExecutor.executeRoutesRefresh(any(), any()) } returns error @@ -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" ) } @@ -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) { diff --git a/libnavigation-core/src/test/java/com/mapbox/navigation/core/routerefresh/PlannedRouteRefreshControllerTest.kt b/libnavigation-core/src/test/java/com/mapbox/navigation/core/routerefresh/PlannedRouteRefreshControllerTest.kt index 25b2c07731b..b1ccdf86060 100644 --- a/libnavigation-core/src/test/java/com/mapbox/navigation/core/routerefresh/PlannedRouteRefreshControllerTest.kt +++ b/libnavigation-core/src/test/java/com/mapbox/navigation/core/routerefresh/PlannedRouteRefreshControllerTest.kt @@ -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.core.internal.utils.CoroutineUtils @@ -529,7 +527,7 @@ class PlannedRouteRefreshControllerTest { sut.startRoutesRefreshing(routes) clearMocks(retryStrategy, answers = false) - finishRequest(ExpectedFactory.createError("Some error")) + finishRequest(RoutesRefresherExecutorResult.ReplacedByNewer) verify(exactly = 0) { attemptListener.onRoutesRefreshAttemptFinished(any()) @@ -544,7 +542,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" + ) } } @@ -556,10 +557,10 @@ class PlannedRouteRefreshControllerTest { } private suspend fun finishRequest(result: RoutesRefresherResult) { - finishRequest(ExpectedFactory.createValue(result)) + finishRequest(RoutesRefresherExecutorResult.Finished(result)) } - private suspend fun finishRequest(result: Expected) { + private suspend fun finishRequest(result: RoutesRefresherExecutorResult) { coEvery { executor.executeRoutesRefresh(any(), any()) } returns result diff --git a/libnavigation-core/src/test/java/com/mapbox/navigation/core/routerefresh/RouteRefreshControllerTest.kt b/libnavigation-core/src/test/java/com/mapbox/navigation/core/routerefresh/RouteRefreshControllerTest.kt index 9db8773b2b5..a846dcc4f3d 100644 --- a/libnavigation-core/src/test/java/com/mapbox/navigation/core/routerefresh/RouteRefreshControllerTest.kt +++ b/libnavigation-core/src/test/java/com/mapbox/navigation/core/routerefresh/RouteRefreshControllerTest.kt @@ -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.core.RoutesInvalidatedObserver @@ -174,7 +172,7 @@ class RouteRefreshControllerTest { @Test fun requestImmediateRouteRefreshWithNonEmptySuccess() { val routes = listOf(mockk()) - val callback = slot<(Expected) -> Unit>() + val callback = slot<(RoutesRefresherExecutorResult) -> Unit>() every { plannedRouteRefreshController.routesToRefresh } returns routes sut.requestImmediateRouteRefresh() @@ -183,7 +181,7 @@ class RouteRefreshControllerTest { immediateRouteRefreshController.requestRoutesRefresh(routes, capture(callback)) } callback.captured( - ExpectedFactory.createValue( + RoutesRefresherExecutorResult.Finished( mockk { every { anySuccess() } returns true } ) ) @@ -195,7 +193,7 @@ class RouteRefreshControllerTest { @Test fun requestImmediateRouteRefreshWithNonEmptyFailure() { val routes = listOf(mockk()) - val callback = slot<(Expected) -> Unit>() + val callback = slot<(RoutesRefresherExecutorResult) -> Unit>() every { plannedRouteRefreshController.routesToRefresh } returns routes sut.requestImmediateRouteRefresh() @@ -204,7 +202,7 @@ class RouteRefreshControllerTest { immediateRouteRefreshController.requestRoutesRefresh(routes, capture(callback)) } callback.captured( - ExpectedFactory.createValue( + RoutesRefresherExecutorResult.Finished( mockk { every { anySuccess() } returns false } @@ -218,7 +216,7 @@ class RouteRefreshControllerTest { @Test fun requestImmediateRouteRefreshWithNonEmptyError() { val routes = listOf(mockk()) - val callback = slot<(Expected) -> Unit>() + val callback = slot<(RoutesRefresherExecutorResult) -> Unit>() every { plannedRouteRefreshController.routesToRefresh } returns routes sut.requestImmediateRouteRefresh() @@ -226,7 +224,7 @@ class RouteRefreshControllerTest { verify(exactly = 1) { immediateRouteRefreshController.requestRoutesRefresh(routes, capture(callback)) } - callback.captured(ExpectedFactory.createError("error")) + callback.captured(RoutesRefresherExecutorResult.ReplacedByNewer) verify(exactly = 0) { plannedRouteRefreshController.resume() } diff --git a/libnavigation-core/src/test/java/com/mapbox/navigation/core/routerefresh/RouteRefresherExecutorTest.kt b/libnavigation-core/src/test/java/com/mapbox/navigation/core/routerefresh/RouteRefresherExecutorTest.kt index e0daf43b0c6..02e7ae982fb 100644 --- a/libnavigation-core/src/test/java/com/mapbox/navigation/core/routerefresh/RouteRefresherExecutorTest.kt +++ b/libnavigation-core/src/test/java/com/mapbox/navigation/core/routerefresh/RouteRefresherExecutorTest.kt @@ -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 @@ -39,7 +40,10 @@ class RouteRefresherExecutorTest { startCallback.invoke() routeRefresher.refresh(routes, timeout) } - assertEquals(routesRefresherResult, actual.value) + assertEquals( + routesRefresherResult, + (actual as RoutesRefresherExecutorResult.Finished).value + ) } @Test @@ -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 @@ -104,6 +114,9 @@ class RouteRefresherExecutorTest { val actual = sut.executeRoutesRefresh(routes, startCallback) - assertEquals(routesRefresherResult, actual.value) + assertEquals( + routesRefresherResult, + (actual as RoutesRefresherExecutorResult.Finished).value + ) } }