commit 7a0649902e98d2dd441769a26c73b56c9cb5a084
parent 0026ee1f90c892ec170835a64cadb03334ac158d
Author: mcarare <48995920+mcarare@users.noreply.github.com>
Date: Mon, 3 Nov 2025 10:21:26 +0000
Bug 1996627 - Refactor tabs cleanup logic and testing r=android-reviewers,giorga
This patch refactors the `TabsCleanupFeature` to improve its structure and testability.
The key changes are:
- A new `showUndoSnackbar` function is introduced to encapsulate the logic for displaying the undo snackbar, reducing code duplication.
- A `tabsClosedUndoMessage` extension function is added to `Context` to provide the correct snackbar message when multiple tabs (private or normal) are closed.
- The corresponding unit tests in `TabsCleanupFeatureTest` have been updated to use `spyk` and `runTest` instead of relying on `mockkStatic` and `MainCoroutineRule`. This simplifies the test setup and removes the need for static mocking.
Differential Revision: https://phabricator.services.mozilla.com/D270854
Diffstat:
4 files changed, 98 insertions(+), 152 deletions(-)
diff --git a/mobile/android/fenix/app/lint-baseline.xml b/mobile/android/fenix/app/lint-baseline.xml
@@ -334,50 +334,6 @@
<issue
id="NoStaticMocking"
message="Usage of MockK's 'mockkStatic' for static mocking is discouraged. Refactor to use dependency injection."
- errorLine1=" mockkStatic("org.mozilla.fenix.utils.UndoKt")"
- errorLine2=" ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~">
- <location
- file="src/test/java/org/mozilla/fenix/home/TabsCleanupFeatureTest.kt"
- line="76"
- column="9"/>
- </issue>
-
- <issue
- id="NoStaticMocking"
- message="Usage of MockK's 'mockkStatic' for static mocking is discouraged. Refactor to use dependency injection."
- errorLine1=" mockkStatic("mozilla.components.browser.state.selector.SelectorsKt")"
- errorLine2=" ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~">
- <location
- file="src/test/java/org/mozilla/fenix/home/TabsCleanupFeatureTest.kt"
- line="224"
- column="9"/>
- </issue>
-
- <issue
- id="NoStaticMocking"
- message="Usage of MockK's 'mockkStatic' for static mocking is discouraged. Refactor to use dependency injection."
- errorLine1=" mockkStatic("mozilla.components.browser.state.selector.SelectorsKt")"
- errorLine2=" ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~">
- <location
- file="src/test/java/org/mozilla/fenix/home/TabsCleanupFeatureTest.kt"
- line="255"
- column="9"/>
- </issue>
-
- <issue
- id="NoStaticMocking"
- message="Usage of MockK's 'mockkStatic' for static mocking is discouraged. Refactor to use dependency injection."
- errorLine1=" mockkStatic("mozilla.components.browser.state.selector.SelectorsKt")"
- errorLine2=" ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~">
- <location
- file="src/test/java/org/mozilla/fenix/home/TabsCleanupFeatureTest.kt"
- line="290"
- column="9"/>
- </issue>
-
- <issue
- id="NoStaticMocking"
- message="Usage of MockK's 'mockkStatic' for static mocking is discouraged. Refactor to use dependency injection."
errorLine1=" mockkStatic("mozilla.components.support.ktx.android.util.DisplayMetricsKt")"
errorLine2=" ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~">
<location
diff --git a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/ext/Context.kt b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/ext/Context.kt
@@ -148,6 +148,17 @@ fun Context.tabClosedUndoMessage(private: Boolean): String =
}
/**
+ * Returns the message to be shown when multiple tabs are closed based on whether the tabs were all private or not.
+ * @param private true if the tab was private, false otherwise.
+ */
+fun Context.tabsClosedUndoMessage(private: Boolean): String =
+ if (private) {
+ getString(R.string.snackbar_private_data_deleted)
+ } else {
+ getString(R.string.snackbar_tabs_closed)
+ }
+
+/**
* Helper function used to determine whether the app's total *window* size is at least that of a tablet.
* This relies on the window size check from [AcornWindowSize]. To determine whether the device's
* *physical* size is at least the size of a tablet, use [Context.isLargeScreenSize] instead.
diff --git a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/home/TabsCleanupFeature.kt b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/home/TabsCleanupFeature.kt
@@ -19,6 +19,7 @@ import org.mozilla.fenix.R
import org.mozilla.fenix.browser.browsingmode.BrowsingModeManager
import org.mozilla.fenix.components.usecases.FenixBrowserUseCases
import org.mozilla.fenix.ext.tabClosedUndoMessage
+import org.mozilla.fenix.ext.tabsClosedUndoMessage
import org.mozilla.fenix.home.HomeScreenViewModel.Companion.ALL_NORMAL_TABS
import org.mozilla.fenix.home.HomeScreenViewModel.Companion.ALL_PRIVATE_TABS
import org.mozilla.fenix.utils.Settings
@@ -69,6 +70,23 @@ class TabsCleanupFeature(
override fun stop() = Unit
+ /**
+ * Shows an undo snackbar after removing one or more tabs.
+ *
+ * @param message The message to display in the snackbar.
+ * @param onCancel The action to perform when the user clicks the "Undo" button.
+ */
+ @VisibleForTesting
+ internal fun showUndoSnackbar(message: String, onCancel: () -> Unit) {
+ viewLifecycleScope.allowUndo(
+ view = snackBarParentView,
+ message = message,
+ undoActionTitle = context.getString(R.string.snackbar_deleted_undo),
+ onCancel = onCancel,
+ operation = {},
+ )
+ }
+
private fun removeAllTabsAndShowSnackbar(sessionCode: String) {
if (sessionCode == ALL_PRIVATE_TABS) {
tabsUseCases.removePrivateTabs()
@@ -76,12 +94,6 @@ class TabsCleanupFeature(
tabsUseCases.removeNormalTabs()
}
- val snackbarMessage = if (sessionCode == ALL_PRIVATE_TABS) {
- context.getString(R.string.snackbar_private_data_deleted)
- } else {
- context.getString(R.string.snackbar_tabs_closed)
- }
-
var tabId: String? = null
if (settings.enableHomepageAsNewTab) {
// Add a new tab after all the tabs are removed to ensure there's always 1 tab.
@@ -92,14 +104,11 @@ class TabsCleanupFeature(
)
}
- viewLifecycleScope.allowUndo(
- view = snackBarParentView,
- message = snackbarMessage,
- undoActionTitle = context.getString(R.string.snackbar_deleted_undo),
+ showUndoSnackbar(
+ message = context.tabsClosedUndoMessage(private = sessionCode == ALL_PRIVATE_TABS),
onCancel = {
onUndoAllTabsRemoved(tabId)
},
- operation = {},
)
}
@@ -141,14 +150,11 @@ class TabsCleanupFeature(
)
}
- viewLifecycleScope.allowUndo(
- view = snackBarParentView,
+ showUndoSnackbar(
message = context.tabClosedUndoMessage(tab.content.private),
- undoActionTitle = context.getString(R.string.snackbar_deleted_undo),
onCancel = {
onUndoTabRemoved(tabId)
},
- operation = {},
)
}
diff --git a/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/home/TabsCleanupFeatureTest.kt b/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/home/TabsCleanupFeatureTest.kt
@@ -12,19 +12,16 @@ import io.mockk.every
import io.mockk.impl.annotations.RelaxedMockK
import io.mockk.just
import io.mockk.mockk
-import io.mockk.mockkStatic
+import io.mockk.spyk
import io.mockk.verify
import io.mockk.verifyOrder
import kotlinx.coroutines.CoroutineScope
-import mozilla.components.browser.state.selector.findTab
-import mozilla.components.browser.state.selector.privateTabs
+import kotlinx.coroutines.test.StandardTestDispatcher
import mozilla.components.browser.state.state.TabSessionState
import mozilla.components.browser.state.store.BrowserStore
import mozilla.components.feature.tabs.TabsUseCases
import mozilla.components.support.test.robolectric.testContext
-import mozilla.components.support.test.rule.MainCoroutineRule
import org.junit.Before
-import org.junit.Rule
import org.junit.Test
import org.junit.runner.RunWith
import org.mozilla.fenix.R
@@ -33,15 +30,13 @@ import org.mozilla.fenix.components.usecases.FenixBrowserUseCases
import org.mozilla.fenix.home.HomeScreenViewModel.Companion.ALL_NORMAL_TABS
import org.mozilla.fenix.home.HomeScreenViewModel.Companion.ALL_PRIVATE_TABS
import org.mozilla.fenix.utils.Settings
-import org.mozilla.fenix.utils.allowUndo
import org.robolectric.RobolectricTestRunner
@RunWith(RobolectricTestRunner::class)
class TabsCleanupFeatureTest {
- @get:Rule
- val coroutinesTestRule = MainCoroutineRule()
- private val testCoroutineScope = coroutinesTestRule.scope
+ private val testDispatcher = StandardTestDispatcher()
+ private val testCoroutineScope = CoroutineScope(testDispatcher)
@RelaxedMockK
private lateinit var viewModel: HomeScreenViewModel
@@ -73,31 +68,22 @@ class TabsCleanupFeatureTest {
fun setup() {
MockKAnnotations.init(this)
- mockkStatic("org.mozilla.fenix.utils.UndoKt")
- every {
- any<CoroutineScope>().allowUndo(
- view = any(),
- message = any(),
- undoActionTitle = any(),
- onCancel = any(),
- operation = any(),
- anchorView = any(),
- elevation = any(),
- )
- } just Runs
-
- feature = TabsCleanupFeature(
- context = testContext,
- viewModel = viewModel,
- browserStore = browserStore,
- browsingModeManager = browsingModeManager,
- navController = navController,
- settings = settings,
- tabsUseCases = tabsUseCases,
- fenixBrowserUseCases = fenixBrowserUseCases,
- snackBarParentView = snackBarParentView,
- viewLifecycleScope = testCoroutineScope,
+ feature = spyk(
+ TabsCleanupFeature(
+ context = testContext,
+ viewModel = viewModel,
+ browserStore = browserStore,
+ browsingModeManager = browsingModeManager,
+ navController = navController,
+ settings = settings,
+ tabsUseCases = tabsUseCases,
+ fenixBrowserUseCases = fenixBrowserUseCases,
+ snackBarParentView = snackBarParentView,
+ viewLifecycleScope = testCoroutineScope,
+ ),
)
+
+ every { feature.showUndoSnackbar(any(), any()) } just Runs
}
@Test
@@ -109,12 +95,9 @@ class TabsCleanupFeatureTest {
verify {
tabsUseCases.removeNormalTabs()
- testCoroutineScope.allowUndo(
- view = snackBarParentView,
- message = testContext.getString(R.string.snackbar_tabs_closed),
- undoActionTitle = testContext.getString(R.string.snackbar_deleted_undo),
- onCancel = any(),
- operation = any(),
+ feature.showUndoSnackbar(
+ testContext.getString(R.string.snackbar_tabs_closed),
+ any(),
)
viewModel.sessionToDelete = null
@@ -130,12 +113,9 @@ class TabsCleanupFeatureTest {
verify {
tabsUseCases.removePrivateTabs()
- testCoroutineScope.allowUndo(
- view = snackBarParentView,
- message = testContext.getString(R.string.snackbar_private_data_deleted),
- undoActionTitle = testContext.getString(R.string.snackbar_deleted_undo),
- onCancel = any(),
- operation = any(),
+ feature.showUndoSnackbar(
+ testContext.getString(R.string.snackbar_private_data_deleted),
+ any(),
)
viewModel.sessionToDelete = null
@@ -156,12 +136,9 @@ class TabsCleanupFeatureTest {
private = browsingModeManager.mode.isPrivate,
)
- testCoroutineScope.allowUndo(
- view = snackBarParentView,
- message = testContext.getString(R.string.snackbar_tabs_closed),
- undoActionTitle = testContext.getString(R.string.snackbar_deleted_undo),
- onCancel = any(),
- operation = any(),
+ feature.showUndoSnackbar(
+ testContext.getString(R.string.snackbar_tabs_closed),
+ any(),
)
viewModel.sessionToDelete = null
@@ -169,7 +146,7 @@ class TabsCleanupFeatureTest {
}
@Test
- fun `GIVEN homepage as a new tab is enabled and all private tabs to delete WHEN rfeature is started THEN remove all normal tabs, show undo snackbar and ensure 1 new tab remains`() {
+ fun `GIVEN homepage as a new tab is enabled and all private tabs to delete WHEN feature is started THEN remove all normal tabs, show undo snackbar and ensure 1 new tab remains`() {
every { settings.enableHomepageAsNewTab } returns true
every { viewModel.sessionToDelete } returns ALL_PRIVATE_TABS
@@ -182,12 +159,9 @@ class TabsCleanupFeatureTest {
private = browsingModeManager.mode.isPrivate,
)
- testCoroutineScope.allowUndo(
- view = snackBarParentView,
- message = testContext.getString(R.string.snackbar_private_data_deleted),
- undoActionTitle = testContext.getString(R.string.snackbar_deleted_undo),
- onCancel = any(),
- operation = any(),
+ feature.showUndoSnackbar(
+ testContext.getString(R.string.snackbar_private_data_deleted),
+ any(),
)
viewModel.sessionToDelete = null
@@ -203,12 +177,9 @@ class TabsCleanupFeatureTest {
verify {
tabsUseCases.removePrivateTabs()
- testCoroutineScope.allowUndo(
- view = snackBarParentView,
- message = testContext.getString(R.string.snackbar_private_data_deleted),
- undoActionTitle = testContext.getString(R.string.snackbar_deleted_undo),
- onCancel = any(),
- operation = any(),
+ feature.showUndoSnackbar(
+ testContext.getString(R.string.snackbar_private_data_deleted),
+ any(),
)
viewModel.sessionToDelete = null
@@ -218,11 +189,14 @@ class TabsCleanupFeatureTest {
@Test
fun `GIVEN a session ID to delete WHEN feature is started THEN remove tab and show undo snackbar`() {
val private = true
- val tab: TabSessionState = mockk { every { content.private } returns private }
val sessionId = "1"
- mockkStatic("mozilla.components.browser.state.selector.SelectorsKt")
- every { browserStore.state.findTab(sessionId) } returns tab
+ val tab: TabSessionState = mockk {
+ every { content.private } returns private
+ every { id } returns sessionId
+ }
+
+ every { browserStore.state.tabs } returns listOf(tab)
every { viewModel.sessionToDelete } returns sessionId
feature.start()
@@ -230,12 +204,9 @@ class TabsCleanupFeatureTest {
verify {
tabsUseCases.removeTab(sessionId)
- testCoroutineScope.allowUndo(
- view = snackBarParentView,
- message = testContext.getString(R.string.snackbar_private_tab_closed),
- undoActionTitle = testContext.getString(R.string.snackbar_deleted_undo),
- onCancel = any(),
- operation = any(),
+ feature.showUndoSnackbar(
+ testContext.getString(R.string.snackbar_private_tab_closed),
+ any(),
)
viewModel.sessionToDelete = null
@@ -245,16 +216,17 @@ class TabsCleanupFeatureTest {
@Test
fun `GIVEN homepage as a new tab is enabled and the last tab is to be removed WHEN feature is started THEN remove tab, show undo snackbar and ensure a new tab remains`() {
val private = true
- val tab: TabSessionState = mockk { every { content.private } returns private }
val sessionId = "1"
+ val tab: TabSessionState = mockk {
+ every { content.private } returns private
+ every { id } returns sessionId
+ }
every { settings.enableHomepageAsNewTab } returns true
every { browsingModeManager.mode.isPrivate } returns private
every { viewModel.sessionToDelete } returns sessionId
- mockkStatic("mozilla.components.browser.state.selector.SelectorsKt")
- every { browserStore.state.findTab(sessionId) } returns tab
- every { browserStore.state.privateTabs.size } returns 1
+ every { browserStore.state.tabs } returns listOf(tab)
feature.start()
@@ -265,12 +237,9 @@ class TabsCleanupFeatureTest {
private = private,
)
- testCoroutineScope.allowUndo(
- view = snackBarParentView,
- message = testContext.getString(R.string.snackbar_private_tab_closed),
- undoActionTitle = testContext.getString(R.string.snackbar_deleted_undo),
- onCancel = any(),
- operation = any(),
+ feature.showUndoSnackbar(
+ testContext.getString(R.string.snackbar_private_tab_closed),
+ any(),
)
viewModel.sessionToDelete = null
@@ -280,28 +249,32 @@ class TabsCleanupFeatureTest {
@Test
fun `GIVEN homepage as a new tab is enabled and a session ID to delete WHEN feature is started THEN remove tab and show undo snackbar`() {
val private = true
- val tab: TabSessionState = mockk { every { content.private } returns private }
val sessionId = "1"
+ val tab: TabSessionState = mockk {
+ every { content.private } returns private
+ every { id } returns sessionId
+ }
+
+ val secondTab: TabSessionState = mockk {
+ every { content.private } returns private
+ every { id } returns "2"
+ }
+
every { settings.enableHomepageAsNewTab } returns true
every { browsingModeManager.mode.isPrivate } returns private
every { viewModel.sessionToDelete } returns sessionId
- mockkStatic("mozilla.components.browser.state.selector.SelectorsKt")
- every { browserStore.state.findTab(sessionId) } returns tab
- every { browserStore.state.privateTabs.size } returns 2
+ every { browserStore.state.tabs } returns listOf(tab, secondTab)
feature.start()
verify {
tabsUseCases.removeTab(sessionId)
- testCoroutineScope.allowUndo(
- view = snackBarParentView,
- message = testContext.getString(R.string.snackbar_private_tab_closed),
- undoActionTitle = testContext.getString(R.string.snackbar_deleted_undo),
- onCancel = any(),
- operation = any(),
+ feature.showUndoSnackbar(
+ testContext.getString(R.string.snackbar_private_tab_closed),
+ any(),
)
viewModel.sessionToDelete = null
