From 4312a968519c27932ec685bab5d25988834b24fa Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Mon, 24 Jul 2023 12:41:02 +0200 Subject: [PATCH 1/5] Cleanup: there are no androidTest on those modules. --- features/analytics/impl/build.gradle.kts | 2 -- features/createroom/impl/build.gradle.kts | 2 -- features/login/impl/build.gradle.kts | 2 -- features/logout/impl/build.gradle.kts | 2 -- features/messages/impl/build.gradle.kts | 1 - features/onboarding/impl/build.gradle.kts | 2 -- features/preferences/impl/build.gradle.kts | 2 -- features/rageshake/impl/build.gradle.kts | 2 -- features/roomlist/impl/build.gradle.kts | 2 -- libraries/permissions/impl/build.gradle.kts | 2 -- tests/uitests/build.gradle.kts | 1 - 11 files changed, 20 deletions(-) diff --git a/features/analytics/impl/build.gradle.kts b/features/analytics/impl/build.gradle.kts index 3bf58ab636..8356bb38bb 100644 --- a/features/analytics/impl/build.gradle.kts +++ b/features/analytics/impl/build.gradle.kts @@ -52,6 +52,4 @@ dependencies { testImplementation(projects.libraries.matrix.test) testImplementation(projects.features.analytics.test) testImplementation(projects.features.analytics.impl) - - androidTestImplementation(libs.test.junitext) } diff --git a/features/createroom/impl/build.gradle.kts b/features/createroom/impl/build.gradle.kts index 8bb343c10a..fe2970dd80 100644 --- a/features/createroom/impl/build.gradle.kts +++ b/features/createroom/impl/build.gradle.kts @@ -66,7 +66,5 @@ dependencies { testImplementation(projects.libraries.mediaupload.test) testImplementation(projects.libraries.usersearch.test) - androidTestImplementation(libs.test.junitext) - ksp(libs.showkase.processor) } diff --git a/features/login/impl/build.gradle.kts b/features/login/impl/build.gradle.kts index f9bbb390e6..4a4c6756aa 100644 --- a/features/login/impl/build.gradle.kts +++ b/features/login/impl/build.gradle.kts @@ -61,6 +61,4 @@ dependencies { testImplementation(libs.test.turbine) testImplementation(projects.libraries.matrix.test) testImplementation(projects.tests.testutils) - - androidTestImplementation(libs.test.junitext) } diff --git a/features/logout/impl/build.gradle.kts b/features/logout/impl/build.gradle.kts index 88d8282875..464695e169 100644 --- a/features/logout/impl/build.gradle.kts +++ b/features/logout/impl/build.gradle.kts @@ -48,6 +48,4 @@ dependencies { testImplementation(libs.test.truth) testImplementation(libs.test.turbine) testImplementation(projects.libraries.matrix.test) - - androidTestImplementation(libs.test.junitext) } diff --git a/features/messages/impl/build.gradle.kts b/features/messages/impl/build.gradle.kts index 7488820f7d..b5edaec78e 100644 --- a/features/messages/impl/build.gradle.kts +++ b/features/messages/impl/build.gradle.kts @@ -78,6 +78,5 @@ dependencies { testImplementation(projects.libraries.mediapickers.test) testImplementation(libs.test.mockk) - androidTestImplementation(libs.test.junitext) ksp(libs.showkase.processor) } diff --git a/features/onboarding/impl/build.gradle.kts b/features/onboarding/impl/build.gradle.kts index 0952835d46..0f97a0ffeb 100644 --- a/features/onboarding/impl/build.gradle.kts +++ b/features/onboarding/impl/build.gradle.kts @@ -48,6 +48,4 @@ dependencies { testImplementation(libs.test.truth) testImplementation(libs.test.turbine) testImplementation(projects.libraries.matrix.test) - - androidTestImplementation(libs.test.junitext) } diff --git a/features/preferences/impl/build.gradle.kts b/features/preferences/impl/build.gradle.kts index f183f7f1fa..60b928e0bb 100644 --- a/features/preferences/impl/build.gradle.kts +++ b/features/preferences/impl/build.gradle.kts @@ -68,6 +68,4 @@ dependencies { testImplementation(projects.features.analytics.test) testImplementation(projects.features.analytics.impl) testImplementation(projects.tests.testutils) - - androidTestImplementation(libs.test.junitext) } diff --git a/features/rageshake/impl/build.gradle.kts b/features/rageshake/impl/build.gradle.kts index 137a3bd070..3283e3f37a 100644 --- a/features/rageshake/impl/build.gradle.kts +++ b/features/rageshake/impl/build.gradle.kts @@ -56,6 +56,4 @@ dependencies { testImplementation(libs.test.mockk) testImplementation(projects.libraries.matrix.test) testImplementation(projects.features.rageshake.test) - - androidTestImplementation(libs.test.junitext) } diff --git a/features/roomlist/impl/build.gradle.kts b/features/roomlist/impl/build.gradle.kts index 302e54fe09..f5a08ba860 100644 --- a/features/roomlist/impl/build.gradle.kts +++ b/features/roomlist/impl/build.gradle.kts @@ -71,6 +71,4 @@ dependencies { testImplementation(projects.features.networkmonitor.test) testImplementation(projects.tests.testutils) testImplementation(projects.features.leaveroom.fake) - - androidTestImplementation(libs.test.junitext) } diff --git a/libraries/permissions/impl/build.gradle.kts b/libraries/permissions/impl/build.gradle.kts index 31a4e4bbb1..9808986f8f 100644 --- a/libraries/permissions/impl/build.gradle.kts +++ b/libraries/permissions/impl/build.gradle.kts @@ -57,7 +57,5 @@ dependencies { testImplementation(libs.test.turbine) testImplementation(projects.libraries.matrix.test) - androidTestImplementation(libs.test.junitext) - ksp(libs.showkase.processor) } diff --git a/tests/uitests/build.gradle.kts b/tests/uitests/build.gradle.kts index 729899c4f8..1881822691 100644 --- a/tests/uitests/build.gradle.kts +++ b/tests/uitests/build.gradle.kts @@ -42,7 +42,6 @@ dependencies { testImplementation(libs.test.junit) testImplementation(libs.test.parameter.injector) testImplementation(projects.libraries.designsystem) - androidTestImplementation(libs.test.junitext) ksp(libs.showkase.processor) kspTest(libs.showkase.processor) From 83d47957e55a453e8a6a6583bd5b90f8211c4924 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Mon, 24 Jul 2023 12:44:50 +0200 Subject: [PATCH 2/5] Ignore this file. --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 70599626ce..0b61f0aaaf 100644 --- a/.gitignore +++ b/.gitignore @@ -38,6 +38,7 @@ captures/ # IntelliJ *.iml .idea/.name +.idea/androidTestResultsUserPreferences.xml .idea/assetWizardSettings.xml .idea/compiler.xml .idea/deploymentTargetDropDown.xml From 94bc2ce53da1660f8d3b116cd24d42b9fdc5b383 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Mon, 24 Jul 2023 15:01:48 +0200 Subject: [PATCH 3/5] getOrPut is not thread safe, so ensure that no multiple instance will be created per data store (#950) --- .../pushstore/impl/DefaultUserPushStoreFactory.kt | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/libraries/pushstore/impl/src/main/kotlin/io/element/android/libraries/pushstore/impl/DefaultUserPushStoreFactory.kt b/libraries/pushstore/impl/src/main/kotlin/io/element/android/libraries/pushstore/impl/DefaultUserPushStoreFactory.kt index a84fe2ea69..16100fd858 100644 --- a/libraries/pushstore/impl/src/main/kotlin/io/element/android/libraries/pushstore/impl/DefaultUserPushStoreFactory.kt +++ b/libraries/pushstore/impl/src/main/kotlin/io/element/android/libraries/pushstore/impl/DefaultUserPushStoreFactory.kt @@ -41,11 +41,13 @@ class DefaultUserPushStoreFactory @Inject constructor( // We can have only one class accessing a single data store, so keep a cache of them. private val cache = mutableMapOf() override fun create(userId: SessionId): UserPushStore { - return cache.getOrPut(userId) { - UserPushStoreDataStore( - context = context, - userId = userId - ) + return synchronized(cache) { + cache.getOrPut(userId) { + UserPushStoreDataStore( + context = context, + userId = userId + ) + } } } From a2975ec094cc1d9073c8e8f084a388fc02be23ae Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Mon, 24 Jul 2023 15:28:36 +0200 Subject: [PATCH 4/5] Add a test to cover fix of #950 --- libraries/pushstore/impl/build.gradle.kts | 14 +++++ .../impl/DefaultUserPushStoreFactoryTest.kt | 56 +++++++++++++++++++ .../session-storage/test/build.gradle.kts | 26 +++++++++ .../test/observer/NoOpSessionObserver.kt | 25 +++++++++ 4 files changed, 121 insertions(+) create mode 100644 libraries/pushstore/impl/src/androidTest/kotlin/io/element/android/libraries/pushstore/impl/DefaultUserPushStoreFactoryTest.kt create mode 100644 libraries/session-storage/test/build.gradle.kts create mode 100644 libraries/session-storage/test/src/main/kotlin/io/element/android/libraries/sessionstorage/test/observer/NoOpSessionObserver.kt diff --git a/libraries/pushstore/impl/build.gradle.kts b/libraries/pushstore/impl/build.gradle.kts index dca5c82a4d..5946e77694 100644 --- a/libraries/pushstore/impl/build.gradle.kts +++ b/libraries/pushstore/impl/build.gradle.kts @@ -20,6 +20,11 @@ plugins { android { namespace = "io.element.android.libraries.push.pushstore.impl" + + defaultConfig { + testInstrumentationRunner = "androidx.test.runner.AndroidJUnitRunner" + testInstrumentationRunnerArguments["clearPackageData"] = "true" + } } anvil { @@ -43,4 +48,13 @@ dependencies { testImplementation(libs.coroutines.test) testImplementation(projects.libraries.matrix.test) testImplementation(projects.services.appnavstate.test) + + androidTestImplementation(libs.coroutines.test) + androidTestImplementation(libs.test.core) + androidTestImplementation(libs.test.junit) + androidTestImplementation(libs.test.truth) + androidTestImplementation(libs.test.runner) + androidTestImplementation(projects.libraries.sessionStorage.test) + + coreLibraryDesugaring(libs.android.desugar) } diff --git a/libraries/pushstore/impl/src/androidTest/kotlin/io/element/android/libraries/pushstore/impl/DefaultUserPushStoreFactoryTest.kt b/libraries/pushstore/impl/src/androidTest/kotlin/io/element/android/libraries/pushstore/impl/DefaultUserPushStoreFactoryTest.kt new file mode 100644 index 0000000000..c87c772ddf --- /dev/null +++ b/libraries/pushstore/impl/src/androidTest/kotlin/io/element/android/libraries/pushstore/impl/DefaultUserPushStoreFactoryTest.kt @@ -0,0 +1,56 @@ +/* + * Copyright (c) 2023 New Vector Ltd + * + * 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 io.element.android.libraries.pushstore.impl + +import androidx.test.platform.app.InstrumentationRegistry +import io.element.android.libraries.matrix.api.core.SessionId +import io.element.android.libraries.pushstore.api.UserPushStore +import io.element.android.libraries.sessionstorage.test.observer.NoOpSessionObserver +import kotlinx.coroutines.runBlocking +import org.junit.Test +import kotlin.concurrent.thread + +/** + * Note: to clear the emulator, invoke: + * adb uninstall io.element.android.libraries.push.pushstore.impl.test + */ +class DefaultUserPushStoreFactoryTest { + + /** + * Ensure that creating UserPushStore is thread safe. + */ + @Test + fun testParallelCreation() { + val context = InstrumentationRegistry.getInstrumentation().targetContext.applicationContext + val sessionId = SessionId("@alice:server.org") + val userPushStoreFactory = DefaultUserPushStoreFactory(context, NoOpSessionObserver()) + var userPushStore1: UserPushStore? = null + val thread1 = thread { + userPushStore1 = userPushStoreFactory.create(sessionId) + } + var userPushStore2: UserPushStore? = null + val thread2 = thread { + userPushStore2 = userPushStoreFactory.create(sessionId) + } + thread1.join() + thread2.join() + runBlocking { + userPushStore1!!.areNotificationEnabledForDevice() + userPushStore2!!.areNotificationEnabledForDevice() + } + } +} diff --git a/libraries/session-storage/test/build.gradle.kts b/libraries/session-storage/test/build.gradle.kts new file mode 100644 index 0000000000..0c8de84669 --- /dev/null +++ b/libraries/session-storage/test/build.gradle.kts @@ -0,0 +1,26 @@ +/* + * Copyright (c) 2023 New Vector Ltd + * + * 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. + */ +plugins { + id("io.element.android-library") +} + +android { + namespace = "io.element.android.libraries.sessionstorage.test" +} + +dependencies { + implementation(projects.libraries.sessionStorage.api) +} diff --git a/libraries/session-storage/test/src/main/kotlin/io/element/android/libraries/sessionstorage/test/observer/NoOpSessionObserver.kt b/libraries/session-storage/test/src/main/kotlin/io/element/android/libraries/sessionstorage/test/observer/NoOpSessionObserver.kt new file mode 100644 index 0000000000..03487a3701 --- /dev/null +++ b/libraries/session-storage/test/src/main/kotlin/io/element/android/libraries/sessionstorage/test/observer/NoOpSessionObserver.kt @@ -0,0 +1,25 @@ +/* + * Copyright (c) 2023 New Vector Ltd + * + * 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 io.element.android.libraries.sessionstorage.test.observer + +import io.element.android.libraries.sessionstorage.api.observer.SessionListener +import io.element.android.libraries.sessionstorage.api.observer.SessionObserver + +class NoOpSessionObserver : SessionObserver { + override fun addListener(listener: SessionListener) = Unit + override fun removeListener(listener: SessionListener) = Unit +} From a6c96af7316bc3ee6655ec470f3c387279278cfe Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Mon, 24 Jul 2023 21:53:19 +0200 Subject: [PATCH 5/5] Use ConcurrentHashMap to manage synchronization. --- .../pushstore/impl/DefaultUserPushStoreFactory.kt | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/libraries/pushstore/impl/src/main/kotlin/io/element/android/libraries/pushstore/impl/DefaultUserPushStoreFactory.kt b/libraries/pushstore/impl/src/main/kotlin/io/element/android/libraries/pushstore/impl/DefaultUserPushStoreFactory.kt index 16100fd858..8c85dca80c 100644 --- a/libraries/pushstore/impl/src/main/kotlin/io/element/android/libraries/pushstore/impl/DefaultUserPushStoreFactory.kt +++ b/libraries/pushstore/impl/src/main/kotlin/io/element/android/libraries/pushstore/impl/DefaultUserPushStoreFactory.kt @@ -26,6 +26,7 @@ import io.element.android.libraries.pushstore.api.UserPushStore import io.element.android.libraries.pushstore.api.UserPushStoreFactory import io.element.android.libraries.sessionstorage.api.observer.SessionListener import io.element.android.libraries.sessionstorage.api.observer.SessionObserver +import java.util.concurrent.ConcurrentHashMap import javax.inject.Inject @SingleIn(AppScope::class) @@ -39,15 +40,13 @@ class DefaultUserPushStoreFactory @Inject constructor( } // We can have only one class accessing a single data store, so keep a cache of them. - private val cache = mutableMapOf() + private val cache = ConcurrentHashMap() override fun create(userId: SessionId): UserPushStore { - return synchronized(cache) { - cache.getOrPut(userId) { - UserPushStoreDataStore( - context = context, - userId = userId - ) - } + return cache.getOrPut(userId) { + UserPushStoreDataStore( + context = context, + userId = userId + ) } }