From d640f583168c1fa2719eabdb6cf4d838a07b85bc Mon Sep 17 00:00:00 2001 From: Tiger Oakes Date: Tue, 21 Jul 2020 10:58:28 -0700 Subject: [PATCH] For #12565 - Clean up controller and add tests --- .../mozilla/fenix/search/SearchController.kt | 28 ++--- .../mozilla/fenix/search/SearchFragment.kt | 14 ++- .../search/DefaultSearchControllerTest.kt | 109 +++++++++++------- 3 files changed, 94 insertions(+), 57 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/search/SearchController.kt b/app/src/main/java/org/mozilla/fenix/search/SearchController.kt index afda73d54..b2fe99221 100644 --- a/app/src/main/java/org/mozilla/fenix/search/SearchController.kt +++ b/app/src/main/java/org/mozilla/fenix/search/SearchController.kt @@ -8,6 +8,7 @@ import android.content.Intent import androidx.navigation.NavController import mozilla.components.browser.search.SearchEngine import mozilla.components.browser.session.Session +import mozilla.components.browser.session.SessionManager import mozilla.components.support.ktx.kotlin.isUrl import org.mozilla.fenix.BrowserDirection import org.mozilla.fenix.HomeActivity @@ -16,15 +17,14 @@ import org.mozilla.fenix.components.metrics.Event import org.mozilla.fenix.components.metrics.Event.PerformedSearch.SearchAccessPoint.ACTION import org.mozilla.fenix.components.metrics.Event.PerformedSearch.SearchAccessPoint.NONE import org.mozilla.fenix.components.metrics.Event.PerformedSearch.SearchAccessPoint.SUGGESTION +import org.mozilla.fenix.components.metrics.MetricController import org.mozilla.fenix.components.metrics.MetricsUtils import org.mozilla.fenix.components.searchengine.CustomSearchEngineStore import org.mozilla.fenix.crashes.CrashListActivity -import org.mozilla.fenix.ext.components -import org.mozilla.fenix.ext.metrics import org.mozilla.fenix.ext.navigateSafe -import org.mozilla.fenix.ext.settings import org.mozilla.fenix.settings.SupportUtils import org.mozilla.fenix.settings.SupportUtils.MozillaPage.MANIFESTO +import org.mozilla.fenix.utils.Settings /** * An interface that handles the view manipulation of the Search, triggered by the Interactor @@ -42,11 +42,14 @@ interface SearchController { fun handleSearchShortcutsButtonClicked() } -@Suppress("TooManyFunctions") +@Suppress("TooManyFunctions", "LongParameterList") class DefaultSearchController( private val activity: HomeActivity, + private val sessionManager: SessionManager, private val store: SearchFragmentStore, private val navController: NavController, + private val settings: Settings, + private val metrics: MetricController, private val clearToolbarFocus: () -> Unit ) : SearchController { @@ -76,7 +79,7 @@ class DefaultSearchController( val event = if (url.isUrl()) { Event.EnteredUrl(false) } else { - activity.settings().incrementActiveSearchCount() + settings.incrementActiveSearchCount() val searchAccessPoint = when (store.state.searchAccessPoint) { NONE -> ACTION @@ -92,7 +95,7 @@ class DefaultSearchController( } } - event?.let { activity.metrics.track(it) } + event?.let { metrics.track(it) } } override fun handleEditingCancelled() { @@ -100,7 +103,6 @@ class DefaultSearchController( } override fun handleTextChanged(text: String) { - val settings = activity.settings() // Display the search shortcuts on each entry of the search fragment (see #5308) val textMatchesCurrentUrl = store.state.url == text val textMatchesCurrentSearch = store.state.searchTerms == text @@ -129,11 +131,11 @@ class DefaultSearchController( from = BrowserDirection.FromSearch ) - activity.metrics.track(Event.EnteredUrl(false)) + metrics.track(Event.EnteredUrl(false)) } override fun handleSearchTermsTapped(searchTerms: String) { - activity.settings().incrementActiveSearchCount() + settings.incrementActiveSearchCount() activity.openToBrowserAndLoad( searchTermOrURL = searchTerms, @@ -155,14 +157,14 @@ class DefaultSearchController( sap ) } - event?.let { activity.metrics.track(it) } + event?.let { metrics.track(it) } } override fun handleSearchShortcutEngineSelected(searchEngine: SearchEngine) { store.dispatch(SearchFragmentAction.SearchShortcutEngineSelected(searchEngine)) val isCustom = CustomSearchEngineStore.isCustomSearchEngine(activity, searchEngine.identifier) - activity.metrics.track(Event.SearchShortcutSelected(searchEngine, isCustom)) + metrics.track(Event.SearchShortcutSelected(searchEngine, isCustom)) } override fun handleSearchShortcutsButtonClicked() { @@ -176,14 +178,14 @@ class DefaultSearchController( } override fun handleExistingSessionSelected(session: Session) { - activity.components.core.sessionManager.select(session) + sessionManager.select(session) activity.openToBrowser( from = BrowserDirection.FromSearch ) } override fun handleExistingSessionSelected(tabId: String) { - val session = activity.components.core.sessionManager.findSessionById(tabId) + val session = sessionManager.findSessionById(tabId) if (session != null) { handleExistingSessionSelected(session) } diff --git a/app/src/main/java/org/mozilla/fenix/search/SearchFragment.kt b/app/src/main/java/org/mozilla/fenix/search/SearchFragment.kt index e0d832ad5..f0ca9bff3 100644 --- a/app/src/main/java/org/mozilla/fenix/search/SearchFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/search/SearchFragment.kt @@ -86,6 +86,7 @@ class SearchFragment : Fragment(), UserInteractionHandler { savedInstanceState: Bundle? ): View? { val activity = activity as HomeActivity + val settings = activity.settings() val args by navArgs() val tabId = args.sessionId @@ -112,13 +113,13 @@ class SearchFragment : Fragment(), UserInteractionHandler { defaultEngineSource = currentSearchEngine, showSearchSuggestions = shouldShowSearchSuggestions(isPrivate), showSearchSuggestionsHint = false, - showSearchShortcuts = requireContext().settings().shouldShowSearchShortcuts && + showSearchShortcuts = settings.shouldShowSearchShortcuts && url.isEmpty() && areShortcutsAvailable, areShortcutsAvailable = areShortcutsAvailable, - showClipboardSuggestions = requireContext().settings().shouldShowClipboardSuggestions, - showHistorySuggestions = requireContext().settings().shouldShowHistorySuggestions, - showBookmarkSuggestions = requireContext().settings().shouldShowBookmarkSuggestions, + showClipboardSuggestions = settings.shouldShowClipboardSuggestions, + showHistorySuggestions = settings.shouldShowHistorySuggestions, + showBookmarkSuggestions = settings.shouldShowBookmarkSuggestions, tabId = tabId, pastedText = args.pastedText, searchAccessPoint = args.searchAccessPoint @@ -128,8 +129,11 @@ class SearchFragment : Fragment(), UserInteractionHandler { val searchController = DefaultSearchController( activity = activity, + sessionManager = requireComponents.core.sessionManager, store = searchStore, navController = findNavController(), + settings = settings, + metrics = requireComponents.analytics.metrics, clearToolbarFocus = ::clearToolbarFocus ) @@ -163,7 +167,7 @@ class SearchFragment : Fragment(), UserInteractionHandler { visible = { currentSearchEngine.searchEngine.identifier.contains("google") && speechIsAvailable() && - requireContext().settings().shouldShowVoiceSearch + settings.shouldShowVoiceSearch }, listener = ::launchVoiceSearch ) diff --git a/app/src/test/java/org/mozilla/fenix/search/DefaultSearchControllerTest.kt b/app/src/test/java/org/mozilla/fenix/search/DefaultSearchControllerTest.kt index e472b357d..6a8a25c81 100644 --- a/app/src/test/java/org/mozilla/fenix/search/DefaultSearchControllerTest.kt +++ b/app/src/test/java/org/mozilla/fenix/search/DefaultSearchControllerTest.kt @@ -4,71 +4,75 @@ package org.mozilla.fenix.search -import android.content.Intent import androidx.navigation.NavController import androidx.navigation.NavDirections +import io.mockk.MockKAnnotations +import io.mockk.Runs import io.mockk.every +import io.mockk.impl.annotations.MockK +import io.mockk.just import io.mockk.mockk +import io.mockk.mockkObject +import io.mockk.unmockkObject import io.mockk.verify import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.test.runBlockingTest import mozilla.components.browser.search.SearchEngine import mozilla.components.browser.session.Session import mozilla.components.browser.session.SessionManager -import mozilla.components.support.test.robolectric.testContext -import org.junit.Assert.assertFalse +import org.junit.After import org.junit.Before import org.junit.Test -import org.junit.runner.RunWith import org.mozilla.fenix.BrowserDirection import org.mozilla.fenix.HomeActivity import org.mozilla.fenix.R import org.mozilla.fenix.components.metrics.Event import org.mozilla.fenix.components.metrics.MetricController -import org.mozilla.fenix.crashes.CrashListActivity -import org.mozilla.fenix.ext.components -import org.mozilla.fenix.ext.intentFilterEq -import org.mozilla.fenix.ext.metrics +import org.mozilla.fenix.components.metrics.MetricsUtils import org.mozilla.fenix.ext.navigateSafe -import org.mozilla.fenix.ext.searchEngineManager -import org.mozilla.fenix.ext.settings -import org.mozilla.fenix.helpers.FenixRobolectricTestRunner import org.mozilla.fenix.settings.SupportUtils import org.mozilla.fenix.utils.Settings @ExperimentalCoroutinesApi -@RunWith(FenixRobolectricTestRunner::class) class DefaultSearchControllerTest { - private val activity: HomeActivity = mockk(relaxed = true) - private val store: SearchFragmentStore = mockk(relaxed = true) - private val navController: NavController = mockk(relaxed = true) - private val defaultSearchEngine: SearchEngine? = mockk(relaxed = true) - private val searchEngine: SearchEngine = mockk(relaxed = true) - private val metrics: MetricController = mockk(relaxed = true) - private val sessionManager: SessionManager = mockk(relaxed = true) - private val settings: Settings = mockk(relaxed = true) - private val clearToolbarFocus: (() -> Unit) = mockk(relaxed = true) + @MockK(relaxed = true) private lateinit var activity: HomeActivity + @MockK(relaxed = true) private lateinit var store: SearchFragmentStore + @MockK(relaxed = true) private lateinit var navController: NavController + @MockK private lateinit var searchEngine: SearchEngine + @MockK(relaxed = true) private lateinit var metrics: MetricController + @MockK(relaxed = true) private lateinit var settings: Settings + @MockK private lateinit var sessionManager: SessionManager + @MockK(relaxed = true) private lateinit var clearToolbarFocus: () -> Unit private lateinit var controller: DefaultSearchController @Before fun setUp() { - every { activity.searchEngineManager.defaultSearchEngine } returns defaultSearchEngine + MockKAnnotations.init(this) + mockkObject(MetricsUtils) + every { store.state.tabId } returns "test-tab-id" every { store.state.searchEngineSource.searchEngine } returns searchEngine - every { activity.metrics } returns metrics - every { activity.components.core.sessionManager } returns sessionManager - every { activity.components.settings } returns settings + every { sessionManager.select(any()) } just Runs + every { MetricsUtils.createSearchEvent(searchEngine, activity, any()) } returns null controller = DefaultSearchController( activity = activity, + sessionManager = sessionManager, store = store, navController = navController, + settings = settings, + metrics = metrics, clearToolbarFocus = clearToolbarFocus ) } + @After + fun teardown() { + unmockkObject(MetricsUtils) + } + @Test fun handleUrlCommitted() { val url = "https://www.google.com/" @@ -86,15 +90,32 @@ class DefaultSearchControllerTest { verify { metrics.track(Event.EnteredUrl(false)) } } + @Test + fun handleSearchCommitted() { + val searchTerm = "Firefox" + + controller.handleUrlCommitted(searchTerm) + + verify { + activity.openToBrowserAndLoad( + searchTermOrURL = searchTerm, + newTab = false, + from = BrowserDirection.FromSearch, + engine = searchEngine + ) + } + verify { settings.incrementActiveSearchCount() } + } + @Test fun handleCrashesUrlCommitted() { val url = "about:crashes" - every { activity.packageName } returns testContext.packageName + every { activity.packageName } returns "org.mozilla.fenix" controller.handleUrlCommitted(url) verify { - activity.startActivity(intentFilterEq(Intent(testContext, CrashListActivity::class.java))) + activity.startActivity(any()) } } @@ -117,13 +138,6 @@ class DefaultSearchControllerTest { @Test fun handleEditingCancelled() = runBlockingTest { - controller = DefaultSearchController( - activity = activity, - store = store, - navController = navController, - clearToolbarFocus = clearToolbarFocus - ) - controller.handleEditingCancelled() verify { @@ -183,8 +197,6 @@ class DefaultSearchControllerTest { fun `do not show search shortcuts when setting disabled AND query empty AND url not matching query`() { every { settings.shouldShowSearchShortcuts } returns false - assertFalse(testContext.settings().shouldShowSearchShortcuts) - val text = "" controller.handleTextChanged(text) @@ -196,8 +208,6 @@ class DefaultSearchControllerTest { fun `do not show search shortcuts when setting disabled AND query non-empty`() { every { settings.shouldShowSearchShortcuts } returns false - assertFalse(testContext.settings().shouldShowSearchShortcuts) - val text = "mozilla" controller.handleTextChanged(text) @@ -278,11 +288,32 @@ class DefaultSearchControllerTest { @Test fun handleExistingSessionSelected() { - val session: Session = mockk(relaxed = true) + val session = mockk() controller.handleExistingSessionSelected(session) verify { sessionManager.select(session) } verify { activity.openToBrowser(from = BrowserDirection.FromSearch) } } + + @Test + fun handleExistingSessionSelected_tabId_nullSession() { + every { sessionManager.findSessionById("tab-id") } returns null + + controller.handleExistingSessionSelected("tab-id") + + verify(inverse = true) { sessionManager.select(any()) } + verify(inverse = true) { activity.openToBrowser(from = BrowserDirection.FromSearch) } + } + + @Test + fun handleExistingSessionSelected_tabId() { + val session = mockk() + every { sessionManager.findSessionById("tab-id") } returns session + + controller.handleExistingSessionSelected("tab-id") + + verify { sessionManager.select(any()) } + verify { activity.openToBrowser(from = BrowserDirection.FromSearch) } + } }