From 3ab6b197a9718ce381f673e9acdb9358c4e479fd Mon Sep 17 00:00:00 2001 From: Jacky Wang Date: Tue, 15 Oct 2024 16:43:05 +0800 Subject: [PATCH] Fix flaky tests due to shared system property The system property is shared within JVM, change system property in a test case might break test cases in another test class. To address the issue, introduce SystemProperty helper class to back up and restore the system properties in tests. Bug: 373177618 Flag: EXEMPT Test only Test: atest SettingsRoboTests Change-Id: I15539ce5ac425f35571d796baa25f259df1b601f --- tests/robotests/Android.bp | 5 +- .../FingerprintEnrollEnrollingTest.java | 11 +++- .../settings/display/DisplayScreenTest.kt | 16 ++--- .../settings/testutils/SystemProperty.kt | 64 +++++++++++++++++++ 4 files changed, 86 insertions(+), 10 deletions(-) create mode 100644 tests/robotests/testutils/com/android/settings/testutils/SystemProperty.kt diff --git a/tests/robotests/Android.bp b/tests/robotests/Android.bp index 84c84b0cf50..1cac3634cc0 100644 --- a/tests/robotests/Android.bp +++ b/tests/robotests/Android.bp @@ -109,7 +109,10 @@ android_robolectric_test { java_library { name: "Settings-robo-testutils", - srcs: ["testutils/**/*.java"], + srcs: [ + "testutils/**/*.java", + "testutils/**/*.kt", + ], libs: [ "Robolectric_all-target_upstream", "Settings-core", diff --git a/tests/robotests/src/com/android/settings/biometrics/fingerprint/FingerprintEnrollEnrollingTest.java b/tests/robotests/src/com/android/settings/biometrics/fingerprint/FingerprintEnrollEnrollingTest.java index 8f983de8068..df2ab455c5b 100644 --- a/tests/robotests/src/com/android/settings/biometrics/fingerprint/FingerprintEnrollEnrollingTest.java +++ b/tests/robotests/src/com/android/settings/biometrics/fingerprint/FingerprintEnrollEnrollingTest.java @@ -63,12 +63,14 @@ import android.widget.TextView; import com.android.settings.R; import com.android.settings.testutils.FakeFeatureFactory; +import com.android.settings.testutils.SystemProperty; import com.android.settings.widget.RingProgressBar; import com.airbnb.lottie.LottieAnimationView; import com.airbnb.lottie.LottieTask; import com.google.android.setupdesign.GlifLayout; +import org.junit.After; import org.junit.Before; import org.junit.Ignore; import org.junit.Test; @@ -111,15 +113,23 @@ public class FingerprintEnrollEnrollingTest { private final int[] mSfpsStageThresholds = new int[]{0, 9, 13, 19, 25}; private final int[] mUdfpsStageThresholds = new int[]{0, 13, 17, 22}; + private final SystemProperty mSystemProperty = new SystemProperty(); + private FingerprintEnrollEnrolling mActivity; private Context mContext; @Before public void setUp() { MockitoAnnotations.initMocks(this); + mSystemProperty.override("robolectric.createActivityContexts", "true"); FakeFeatureFactory.setupForTest(); } + @After + public void tearDown() { + mSystemProperty.close(); + } + @Test public void fingerprintUdfpsEnrollSuccessProgress_shouldNotVibrate() { initializeActivityFor(TYPE_UDFPS_OPTICAL); @@ -645,7 +655,6 @@ public class FingerprintEnrollEnrollingTest { } private void createActivity() { - System.setProperty("robolectric.createActivityContexts", "true"); final Bundle savedInstanceState = new Bundle(); savedInstanceState.putInt(KEY_STATE_PREVIOUS_ROTATION, Surface.ROTATION_90); diff --git a/tests/robotests/src/com/android/settings/display/DisplayScreenTest.kt b/tests/robotests/src/com/android/settings/display/DisplayScreenTest.kt index 6a7c2388622..61c3b192917 100644 --- a/tests/robotests/src/com/android/settings/display/DisplayScreenTest.kt +++ b/tests/robotests/src/com/android/settings/display/DisplayScreenTest.kt @@ -21,9 +21,9 @@ import androidx.test.ext.junit.runners.AndroidJUnit4 import com.android.internal.widget.LockPatternUtils import com.android.settings.flags.Flags import com.android.settings.testutils.FakeFeatureFactory +import com.android.settings.testutils.SystemProperty import com.android.settingslib.preference.CatalystScreenTestCase import com.google.common.truth.Truth.assertThat -import org.junit.Ignore import org.junit.Test import org.junit.runner.RunWith import org.mockito.ArgumentMatchers.anyInt @@ -66,16 +66,16 @@ class DisplayScreenTest : CatalystScreenTestCase() { assertThat(preferenceScreenCreator.isAvailable(contextWrapper)).isFalse() } - @Ignore("robolectric.createActivityContexts cause other test failure") override fun migration() { // avoid UnsupportedOperationException when getDisplay from context - System.setProperty("robolectric.createActivityContexts", "true") + SystemProperty("robolectric.createActivityContexts", "true").use { + val lockPatternUtils = + mock { on { isSecure(anyInt()) } doReturn true } + FakeFeatureFactory.setupForTest().securityFeatureProvider.stub { + on { getLockPatternUtils(any()) } doReturn lockPatternUtils + } - val lockPatternUtils = mock { on { isSecure(anyInt()) } doReturn true } - FakeFeatureFactory.setupForTest().securityFeatureProvider.stub { - on { getLockPatternUtils(any()) } doReturn lockPatternUtils + super.migration() } - - super.migration() } } diff --git a/tests/robotests/testutils/com/android/settings/testutils/SystemProperty.kt b/tests/robotests/testutils/com/android/settings/testutils/SystemProperty.kt new file mode 100644 index 00000000000..9c2574e52f9 --- /dev/null +++ b/tests/robotests/testutils/com/android/settings/testutils/SystemProperty.kt @@ -0,0 +1,64 @@ +/* + * Copyright (C) 2024 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.settings.testutils + +/** + * Helper class to override system properties. + * + * [System.setProperty] changes the static state in the JVM, which is shared by all tests. Hence, + * there is chance that test cases are dependent/interfered due to system property unexpectedly. + * This helper class backs up the old properties when invoking [override] and restore the old + * properties in [close] to avoid flaky testing. + */ +class SystemProperty(overrides: Map = mapOf()) : AutoCloseable { + private val oldProperties = mutableMapOf() + + constructor(key: String, value: String?) : this(mapOf(key to value)) + + init { + override(overrides) + } + + fun override(key: String, value: String?) = override(mapOf(key to value)) + + fun override(overrides: Map) { + // back up system properties for the overrides + for (key in overrides.keys) { + // only back up the oldest property + if (!oldProperties.containsKey(key)) { + oldProperties[key] = System.getProperty(key) + } + } + overrides.overrideProperties() + } + + override fun close() { + // restore the backed up properties + oldProperties.overrideProperties() + oldProperties.clear() + } + + private fun Map.overrideProperties() { + for ((key, value) in this) { + if (value != null) { + System.setProperty(key, value) + } else { + System.clearProperty(key) + } + } + } +}