From 6abeb2d9e78df4e927ba58b150cd511a7e77c9d4 Mon Sep 17 00:00:00 2001 From: Michael Comella Date: Thu, 24 Sep 2020 15:21:15 -0700 Subject: [PATCH] For #13959: change StrictModeManager to class from object. I originally tried to create this PR leaving this as an object to keep the change simple but it wasn't worth it - once the object started to keep state, we'd need to manually reset the state between runs. Also, the tests were already getting hacky with static mocking so it was easier to address some of those issues this way too. --- .../org/mozilla/fenix/FenixApplication.kt | 3 +-- .../java/org/mozilla/fenix/HomeActivity.kt | 2 +- .../org/mozilla/fenix/StrictModeManager.kt | 21 +++++++-------- .../mozilla/fenix/components/Components.kt | 2 ++ .../mozilla/fenix/StrictModeManagerTest.kt | 27 ++++++++++--------- 5 files changed, 28 insertions(+), 27 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/FenixApplication.kt b/app/src/main/java/org/mozilla/fenix/FenixApplication.kt index e3b7e9ed65..8213dcb03a 100644 --- a/app/src/main/java/org/mozilla/fenix/FenixApplication.kt +++ b/app/src/main/java/org/mozilla/fenix/FenixApplication.kt @@ -40,7 +40,6 @@ import mozilla.components.support.rusthttp.RustHttpConfig import mozilla.components.support.rustlog.RustLog import mozilla.components.support.utils.logElapsedTime import mozilla.components.support.webextensions.WebExtensionSupport -import org.mozilla.fenix.StrictModeManager.enableStrictMode import org.mozilla.fenix.components.Components import org.mozilla.fenix.components.metrics.MetricServiceType import org.mozilla.fenix.ext.components @@ -126,7 +125,7 @@ open class FenixApplication : LocaleAwareApplication(), Provider { val megazordSetup = setupMegazord() setDayNightTheme() - enableStrictMode(true) + components.strictMode.enableStrictMode(true) warmBrowsersCache() // Make sure the engine is initialized and ready to use. diff --git a/app/src/main/java/org/mozilla/fenix/HomeActivity.kt b/app/src/main/java/org/mozilla/fenix/HomeActivity.kt index ea8d863de5..e997c9f55d 100644 --- a/app/src/main/java/org/mozilla/fenix/HomeActivity.kt +++ b/app/src/main/java/org/mozilla/fenix/HomeActivity.kt @@ -150,7 +150,7 @@ open class HomeActivity : LocaleAwareAppCompatActivity(), NavHostActivity { private lateinit var navigationToolbar: Toolbar final override fun onCreate(savedInstanceState: Bundle?) { - StrictModeManager.attachListenerToDisablePenaltyDeath(supportFragmentManager) + components.strictMode.attachListenerToDisablePenaltyDeath(supportFragmentManager) // There is disk read violations on some devices such as samsung and pixel for android 9/10 StrictMode.allowThreadDiskReads().resetPoliciesAfter { super.onCreate(savedInstanceState) diff --git a/app/src/main/java/org/mozilla/fenix/StrictModeManager.kt b/app/src/main/java/org/mozilla/fenix/StrictModeManager.kt index 25944f7d64..2b9594e503 100644 --- a/app/src/main/java/org/mozilla/fenix/StrictModeManager.kt +++ b/app/src/main/java/org/mozilla/fenix/StrictModeManager.kt @@ -9,24 +9,24 @@ import android.os.StrictMode import androidx.fragment.app.Fragment import androidx.fragment.app.FragmentManager +private const val MANUFACTURE_HUAWEI: String = "HUAWEI" +private const val MANUFACTURE_ONE_PLUS: String = "OnePlus" + /** * Manages strict mode settings for the application. - * - * Due to the static dependencies in this class, it's getting hard to test: if this class takes any - * many more static dependencies, consider switching it, and other code that uses [StrictMode] like - * [StrictMode.ThreadPolicy].resetPoliciesAfter, to a class with dependency injection. */ -object StrictModeManager { +class StrictModeManager(config: Config) { + + // The expression in this if is duplicated in StrictMode.ThreadPolicy.resetPoliciesAfter + // because we don't want to have to pass in a dependency each time the ext fn is called. + private val isEnabledByBuildConfig = config.channel.isDebug /*** * Enables strict mode for debug purposes. meant to be run only in the main process. * @param setPenaltyDeath boolean value to decide setting the penaltyDeath as a penalty. */ fun enableStrictMode(setPenaltyDeath: Boolean) { - // The expression in this if is duplicated in StrictMode.ThreadPolicy.resetPoliciesAfter - // because the tests break in unexpected ways if the value is shared as a constant in this - // class. It wasn't worth the time to address it. - if (Config.channel.isDebug) { + if (isEnabledByBuildConfig) { val threadPolicy = StrictMode.ThreadPolicy.Builder() .detectAll() .penaltyLog() @@ -67,9 +67,6 @@ object StrictModeManager { }, false) } - private const val MANUFACTURE_HUAWEI: String = "HUAWEI" - private const val MANUFACTURE_ONE_PLUS: String = "OnePlus" - /** * There are certain manufacturers that have custom font classes for the OS systems. * These classes violates the [StrictMode] policies on startup. As a workaround, we create diff --git a/app/src/main/java/org/mozilla/fenix/components/Components.kt b/app/src/main/java/org/mozilla/fenix/components/Components.kt index 24846cc4d5..8bcbce4202 100644 --- a/app/src/main/java/org/mozilla/fenix/components/Components.kt +++ b/app/src/main/java/org/mozilla/fenix/components/Components.kt @@ -19,6 +19,7 @@ import mozilla.components.support.migration.state.MigrationStore import org.mozilla.fenix.BuildConfig import org.mozilla.fenix.Config import org.mozilla.fenix.HomeActivity +import org.mozilla.fenix.StrictModeManager import org.mozilla.fenix.components.metrics.AppStartupTelemetry import org.mozilla.fenix.ext.settings import org.mozilla.fenix.utils.ClipboardHandler @@ -126,6 +127,7 @@ class Components(private val context: Context) { val performance by lazy { PerformanceComponent() } val push by lazy { Push(context, analytics.crashReporter) } val wifiConnectionMonitor by lazy { WifiConnectionMonitor(context as Application) } + val strictMode by lazy { StrictModeManager(Config) } val settings by lazy { Settings(context) } diff --git a/app/src/test/java/org/mozilla/fenix/StrictModeManagerTest.kt b/app/src/test/java/org/mozilla/fenix/StrictModeManagerTest.kt index 62ec64d280..5c5601c9dc 100644 --- a/app/src/test/java/org/mozilla/fenix/StrictModeManagerTest.kt +++ b/app/src/test/java/org/mozilla/fenix/StrictModeManagerTest.kt @@ -11,10 +11,8 @@ import io.mockk.confirmVerified import io.mockk.every import io.mockk.impl.annotations.MockK import io.mockk.mockk -import io.mockk.mockkObject import io.mockk.mockkStatic import io.mockk.slot -import io.mockk.unmockkObject import io.mockk.unmockkStatic import io.mockk.verify import org.junit.After @@ -26,44 +24,49 @@ import org.mozilla.fenix.helpers.FenixRobolectricTestRunner @RunWith(FenixRobolectricTestRunner::class) class StrictModeManagerTest { + private lateinit var debugManager: StrictModeManager + private lateinit var releaseManager: StrictModeManager + @MockK(relaxUnitFun = true) private lateinit var fragmentManager: FragmentManager @Before fun setup() { MockKAnnotations.init(this) mockkStatic(StrictMode::class) - mockkObject(Config) + + // These tests log a warning that mockk couldn't set the backing field of Config.channel + // but it doesn't seem to impact their correctness so I'm ignoring it. + val debugConfig: Config = mockk { every { channel } returns ReleaseChannel.Debug } + debugManager = StrictModeManager(debugConfig) + + val releaseConfig: Config = mockk { every { channel } returns ReleaseChannel.Release } + releaseManager = StrictModeManager(releaseConfig) } @After fun teardown() { unmockkStatic(StrictMode::class) - unmockkObject(Config) } @Test fun `test enableStrictMode in release`() { - every { Config.channel } returns ReleaseChannel.Release - StrictModeManager.enableStrictMode(false) - + releaseManager.enableStrictMode(false) verify(exactly = 0) { StrictMode.setThreadPolicy(any()) } verify(exactly = 0) { StrictMode.setVmPolicy(any()) } } @Test fun `test enableStrictMode in debug`() { - every { Config.channel } returns ReleaseChannel.Debug - StrictModeManager.enableStrictMode(false) - + debugManager.enableStrictMode(false) verify { StrictMode.setThreadPolicy(any()) } verify { StrictMode.setVmPolicy(any()) } } @Test - fun `test changeStrictModePolicies`() { + fun `test changeStrictModePolicies in debug`() { val callbacks = slot() - StrictModeManager.attachListenerToDisablePenaltyDeath(fragmentManager) + debugManager.attachListenerToDisablePenaltyDeath(fragmentManager) verify { fragmentManager.registerFragmentLifecycleCallbacks(capture(callbacks), false) } confirmVerified(fragmentManager)