From ed16a55111b747e8e00321fcc6f91a25be006855 Mon Sep 17 00:00:00 2001 From: 0nko Date: Thu, 1 May 2025 11:28:44 +0200 Subject: [PATCH 1/2] Fix the new tab creation logic and add unit tests --- .../app/browser/tabs/DefaultTabManager.kt | 33 +++++++-- .../app/browser/tabs/DefaultTabManagerTest.kt | 72 +++++++++++++++++++ 2 files changed, 98 insertions(+), 7 deletions(-) diff --git a/app/src/main/java/com/duckduckgo/app/browser/tabs/DefaultTabManager.kt b/app/src/main/java/com/duckduckgo/app/browser/tabs/DefaultTabManager.kt index e3726066df8f..37fab99d96a5 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/tabs/DefaultTabManager.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/tabs/DefaultTabManager.kt @@ -18,20 +18,27 @@ package com.duckduckgo.app.browser.tabs import com.duckduckgo.app.browser.SkipUrlConversionOnNewTabFeature import com.duckduckgo.app.browser.omnibar.OmnibarEntryConverter +import com.duckduckgo.app.browser.tabs.TabManager.Companion.NEW_TAB_CREATION_TIMEOUT_LIMIT import com.duckduckgo.app.tabs.model.TabEntity import com.duckduckgo.app.tabs.model.TabRepository import com.duckduckgo.common.utils.DispatcherProvider import com.duckduckgo.di.scopes.ActivityScope import com.squareup.anvil.annotations.ContributesBinding +import kotlinx.coroutines.FlowPreview +import kotlinx.coroutines.TimeoutCancellationException +import kotlinx.coroutines.flow.catch import javax.inject.Inject -import kotlinx.coroutines.flow.first +import kotlinx.coroutines.flow.firstOrNull +import kotlinx.coroutines.flow.timeout import kotlinx.coroutines.flow.transformWhile import kotlinx.coroutines.withContext import timber.log.Timber +import kotlin.time.Duration.Companion.seconds interface TabManager { companion object { const val MAX_ACTIVE_TABS = 20 + const val NEW_TAB_CREATION_TIMEOUT_LIMIT = 1 // seconds } fun registerCallbacks(onTabsUpdated: (List) -> Unit) @@ -76,15 +83,27 @@ class DefaultTabManager @Inject constructor( } } + @OptIn(FlowPreview::class) override suspend fun requestAndWaitForNewTab(): TabEntity = withContext(dispatchers.io()) { val tabId = openNewTab() - return@withContext tabRepository.flowTabs.transformWhile { result -> - result.firstOrNull { it.tabId == tabId }?.let { entity -> - emit(entity) - return@transformWhile true + return@withContext tabRepository.flowTabs + .transformWhile { result -> + result.firstOrNull { it.tabId == tabId }?.let { entity -> + emit(entity) + return@transformWhile false // stop after finding the tab + } + return@transformWhile true // continue looking if not found } - return@transformWhile false - }.first() + .timeout(NEW_TAB_CREATION_TIMEOUT_LIMIT.seconds) + .catch { e -> + if (e is TimeoutCancellationException) { + // timeout expired and the new tab was not found + throw IllegalStateException("A new tab failed to be created within 1 second") + } else { + throw e + } + } + .firstOrNull() ?: throw IllegalStateException("Tabs flow completed before finding the new tab") } override suspend fun switchToTab(tabId: String) = withContext(dispatchers.io()) { diff --git a/app/src/test/java/com/duckduckgo/app/browser/tabs/DefaultTabManagerTest.kt b/app/src/test/java/com/duckduckgo/app/browser/tabs/DefaultTabManagerTest.kt index 37dc1406e8c1..d886f868481b 100644 --- a/app/src/test/java/com/duckduckgo/app/browser/tabs/DefaultTabManagerTest.kt +++ b/app/src/test/java/com/duckduckgo/app/browser/tabs/DefaultTabManagerTest.kt @@ -8,9 +8,12 @@ import com.duckduckgo.app.tabs.model.TabRepository import com.duckduckgo.common.test.CoroutineTestRule import com.duckduckgo.feature.toggles.api.FakeFeatureToggleFactory import com.duckduckgo.feature.toggles.api.Toggle.State +import kotlinx.coroutines.delay +import kotlinx.coroutines.flow.flow import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.test.runTest import org.junit.Assert.assertEquals +import org.junit.Assert.fail import org.junit.Before import org.junit.Rule import org.junit.Test @@ -164,4 +167,73 @@ class DefaultTabManagerTest { verify(tabRepository).add(url = query, skipHome = false) verify(omnibarEntryConverter, never()).convertQueryToUrl(query) } + + @Test + fun whenRequestAndWaitForNewTabAndTabIsFoundImmediatelyThenReturnTabEntity() = runTest { + val tabId = "tabId" + val tabEntity = TabEntity(tabId = tabId, position = 0) + whenever(tabRepository.flowTabs).thenReturn(flowOf(listOf(tabEntity))) + whenever(tabRepository.add()).thenReturn(tabId) + + val result = testee.requestAndWaitForNewTab() + + assertEquals(tabEntity, result) + } + + @Test + fun whenRequestAndWaitForNewTabAndTabIsFoundAfterDelayThenReturnTabEntity() = runTest { + val tabId = "tabId" + val tabEntity = TabEntity(tabId = tabId, position = 0) + + val flow = flow { + emit(emptyList()) + delay(500) // Delay less than the timeout + emit(listOf(tabEntity)) + } + whenever(tabRepository.flowTabs).thenReturn(flow) + whenever(tabRepository.add()).thenReturn(tabId) + + val result = testee.requestAndWaitForNewTab() + + assertEquals(tabEntity, result) + } + + @Test + fun whenRequestAndWaitForNewTabAndTimeoutOccursThenThrowException() = runTest { + val tabId = "tabId" + + val flow = flow { + while(true) { + emit(emptyList()) + delay(300) + } + } + whenever(tabRepository.flowTabs).thenReturn(flow) + whenever(tabRepository.add()).thenReturn(tabId) + + try { + testee.requestAndWaitForNewTab() + fail("Expected IllegalStateException was not thrown") + } catch (e: IllegalStateException) { + assertEquals("A new tab failed to be created within 1 second", e.message) + } + } + + @Test + fun whenRequestAndWaitForNewTabAndFlowCompletesWithoutFindingTabThenThrowException() = runTest { + val tabId = "tabId" + val otherTabId = "otherTabId" + val otherTabEntity = TabEntity(tabId = otherTabId, position = 0) + + val flow = flowOf(listOf(otherTabEntity)) + whenever(tabRepository.flowTabs).thenReturn(flow) + whenever(tabRepository.add()).thenReturn(tabId) + + try { + testee.requestAndWaitForNewTab() + fail("Expected IllegalStateException was not thrown") + } catch (e: IllegalStateException) { + assertEquals("Tabs flow completed before finding the new tab", e.message) + } + } } From ddfd249b058187d019b8ff0e0a16669af560006d Mon Sep 17 00:00:00 2001 From: 0nko Date: Mon, 5 May 2025 11:55:42 +0200 Subject: [PATCH 2/2] Fix ktlint issues --- .../java/com/duckduckgo/app/browser/tabs/DefaultTabManager.kt | 4 ++-- .../com/duckduckgo/app/browser/tabs/DefaultTabManagerTest.kt | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/src/main/java/com/duckduckgo/app/browser/tabs/DefaultTabManager.kt b/app/src/main/java/com/duckduckgo/app/browser/tabs/DefaultTabManager.kt index 37fab99d96a5..63bffee9b443 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/tabs/DefaultTabManager.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/tabs/DefaultTabManager.kt @@ -24,16 +24,16 @@ import com.duckduckgo.app.tabs.model.TabRepository import com.duckduckgo.common.utils.DispatcherProvider import com.duckduckgo.di.scopes.ActivityScope import com.squareup.anvil.annotations.ContributesBinding +import javax.inject.Inject +import kotlin.time.Duration.Companion.seconds import kotlinx.coroutines.FlowPreview import kotlinx.coroutines.TimeoutCancellationException import kotlinx.coroutines.flow.catch -import javax.inject.Inject import kotlinx.coroutines.flow.firstOrNull import kotlinx.coroutines.flow.timeout import kotlinx.coroutines.flow.transformWhile import kotlinx.coroutines.withContext import timber.log.Timber -import kotlin.time.Duration.Companion.seconds interface TabManager { companion object { diff --git a/app/src/test/java/com/duckduckgo/app/browser/tabs/DefaultTabManagerTest.kt b/app/src/test/java/com/duckduckgo/app/browser/tabs/DefaultTabManagerTest.kt index d886f868481b..da6aefe8343f 100644 --- a/app/src/test/java/com/duckduckgo/app/browser/tabs/DefaultTabManagerTest.kt +++ b/app/src/test/java/com/duckduckgo/app/browser/tabs/DefaultTabManagerTest.kt @@ -203,7 +203,7 @@ class DefaultTabManagerTest { val tabId = "tabId" val flow = flow { - while(true) { + while (true) { emit(emptyList()) delay(300) }