Instrumentation test coroutines #7207

ouchadam · 2022-09-22T10:20:18Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Updates all of the sdk instrumentation tests to use suspending functions, replacing CountdownLatch usages

Motivation and context

To help improve test stability and time taken to execute

Before: 23m 43s
After: 6m 34s

72% reduction in runtime, saving around 17 minutes~

Screenshots / GIFs

Before	After

|

Tests

Run the ./gradlew matrix-sdk-android:connectedAndroidTest, or all the matrix-sdk-android androidTest tests from within Android Studio

Tested devices

Physical
Emulator
OS version(s): 31

ouchadam · 2022-09-22T13:12:40Z

for reviewers, enabling hide whitespace will provide a much better diff and the PR should be merged as a squash (the commits were used as checkpoints to ensure all tests continued to pass during the refactors)

ouchadam · 2022-09-22T15:56:10Z

vector-app/src/androidTest/java/im/vector/app/CantVerifyTest.kt

@@ -34,6 +35,7 @@ import java.util.UUID

 @RunWith(AndroidJUnit4::class)
 @LargeTest
+@Ignore // Temporary ignore whilst confirming other tests


I'm currently investigating re-enabling this and the VoiceRecorderLTests tests

CantVerifyTest is fixed by #7210
VoiceRecorderLTests is fixed by #7226

…lper

…ng version

- replaces some more runBlocking/periodic lateches

… there's only 1 variant

- fixes double trigger on the resume which error

sonarqubecloud · 2022-09-26T14:04:40Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

ouchadam · 2022-09-26T14:16:20Z

matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/account/ChangePasswordTest.kt

@@ -43,9 +43,7 @@ class ChangePasswordTest : InstrumentedTest {
        val session = commonTestHelper.createAccount(TestConstants.USER_ALICE, SessionTestParams(withInitialSync = false))

        // Change password
-        commonTestHelper.runBlockingTest {


the runCryptoTest & runSessionTest are now suspending which means runBlockingTest is the equivalent of running in the original scope

ouchadam · 2022-09-26T14:17:53Z

matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/common/CommonTestHelper.kt

-                    testHelper.cleanUpOpenedSessions()
+            return runTest(dispatchTimeoutMs = TestConstants.timeOutMillis) {
+                try {
+                    withContext(Dispatchers.Default) {


we use the Default context to allow Timeouts to work correctly, the original runTest dispatcher uses a test scheduler which requires the scheduler timings to be manually incremented, otherwise timing based suspensions like delay/timeout complete instantly (which we don't want!)

ouchadam · 2022-09-26T14:22:21Z

matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/common/TestExtensions.kt

+ * limitations under the License.
+ */
+
+package org.matrix.android.sdk.common


reusable test functions, more functions could lifted from the testHelper now that the helpers are mostly stateless~

ouchadam · 2022-09-26T14:25:48Z

matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/internal/crypto/UnwedgingTest.kt

@@ -173,25 +141,18 @@ class UnwedgingTest : InstrumentedTest {
                bobSession.cryptoService().getMyDevice().identityKey()!!
        )
        olmDevice.clearOlmSessionCache()
-        Thread.sleep(6_000)


we should avoid any thread sleeps and instead rely on dynamic updates from retryPeriodic, waitFor, suspendCancellableCoroutine

ouchadam · 2022-09-26T14:26:58Z

matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/internal/crypto/UnwedgingTest.kt


-        messagesReceivedByBob.size shouldBe 1
+        messagesReceivedByBob.size shouldBeEqualTo 1


shouldBe uses reference equality === whereas shouldBeEqualTo uses value equality ==

bmarty · 2022-09-27T12:24:40Z

...ndroid/src/androidTest/java/org/matrix/android/sdk/internal/crypto/E2EShareKeysConfigTest.kt

@@ -135,7 +126,7 @@ class E2EShareKeysConfigTest : InstrumentedTest {
    }

    @Test
-    fun ifSharingDisabledOnAliceSideBobShouldNotShareAliceHistoty() = runCryptoTest(context()) { cryptoTestHelper, commonTestHelper ->
+    fun ifSharingDisabledOnAliceSideBobShouldNotShareAliceHistory() = runCryptoTest(context()) { cryptoTestHelper, commonTestHelper ->


thought I'd fix it whilst I was in the area 😄

bmarty

Awesome work, LGTM. Some documentation above fun in https://github.com/vector-im/element-android/blob/3e46263bef7d480b441477ecf98589d79610e873/matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/common/TestExtensions.kt could help, but I can add it later, to not block this PR from being merged.

ouchadam · 2022-09-27T12:36:42Z

@bmarty agreed! we could also lift some of the TestHelper functions out as extensions/file level functions as well

ouchadam marked this pull request as ready for review September 22, 2022 13:11

ouchadam requested review from a team and onurays and removed request for a team September 22, 2022 13:12

ouchadam changed the title ~~[WIP] Instrumentation test coroutines~~ Instrumentation test coroutines Sep 22, 2022

Base automatically changed from feature/adm/configurable-sync-timeout to develop September 22, 2022 14:40

ouchadam force-pushed the feature/adm/coroutine-instrumentation branch from 955ac8d to ad8786d Compare September 22, 2022 14:41

ouchadam commented Sep 22, 2022

View reviewed changes

ouchadam force-pushed the feature/adm/coroutine-instrumentation branch from ad8786d to 1db0ef4 Compare September 23, 2022 10:14

ouchadam changed the base branch from develop to feature/bma/ignore_flaky_test September 23, 2022 10:14

bmarty force-pushed the feature/bma/ignore_flaky_test branch from 9c42b87 to 97b3b23 Compare September 23, 2022 12:09

Base automatically changed from feature/bma/ignore_flaky_test to develop September 23, 2022 13:10

ouchadam force-pushed the feature/adm/coroutine-instrumentation branch from 1db0ef4 to 3f83afb Compare September 23, 2022 13:26

ouchadam added the PR-Large PR with more than 500 updated lines label Sep 23, 2022

ouchadam added 15 commits September 26, 2022 14:07

adding test helpers when using coroutines with live data

bb381ca

replacing latched live data waiting with coroutines

b4dde44

adding suspending test account creation

4552f43

removing unused helpers

6b9e5a2

converting test helper to use coroutine based waits

743667b

adding suspending message sending and top level suspending runTest he…

79fd5bf

…lper

creating wrapper function for testing coroutine timeouts

f90667a

using suspending periodic retries for the E2E share history tests

e3fb362

replacing direct usages of withTimeout with the deferred result readi…

e59cadc

…ng version

allowing the live data suspending wrapper to clean up on cancellation

7f0955b

replacing send message test periodic retries with suspending versions

6ebda6b

forcing periodic retries to block the test thread

71d18bb

using suspending message sending and removing duplicated retry rule

24a7ea9

using default context when running tests to allow delays to execute

4004263

- replaces some more runBlocking/periodic lateches

replacing latches with suspending versions

e39faa8

ouchadam added 18 commits September 26, 2022 14:07

making all test createAccount usages suspending

75becaf

making all runCryptoTest suspending

9d3fdda

renaming all suspending test runners back to their original names now…

20f7fc0

… there's only 1 variant

using suspending text message sending for all instrumentation tests

087be63

making all test doSync calls suspending instead of latch

a9361c7

removing no longer needed test runBlocking or launch

ae51bfe

replacing remaining period wait with latch with suspending version

5ed9e75

replacing remaining latched wait usages with suspends

707f3bf

removing no longer need runBlocking

53e8e25

renaming function for better context

2c338bd

removing unused scope

99d01b2

disabling test retries to help identify flaky tests

e09f9bc

including waiting for the room topic to be updated

2c446ce

replacing latches in keysbackup tests

ff012bd

ensuring the verification listener is removed before resuming

aef44a3

- fixes double trigger on the resume which error

removing unused imports and re-enabling retry test rule

bf153e2

using correct license for matrix sdk class

3e46263

adding changelog entry

c595475

ouchadam force-pushed the feature/adm/coroutine-instrumentation branch from 3f83afb to c595475 Compare September 26, 2022 13:08

ouchadam commented Sep 26, 2022

View reviewed changes

bmarty reviewed Sep 27, 2022

View reviewed changes

bmarty approved these changes Sep 27, 2022

View reviewed changes

ouchadam merged commit fad0206 into develop Sep 27, 2022

ouchadam deleted the feature/adm/coroutine-instrumentation branch September 27, 2022 12:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Instrumentation test coroutines #7207

Instrumentation test coroutines #7207

ouchadam commented Sep 22, 2022 •

edited

Loading

ouchadam commented Sep 22, 2022

ouchadam Sep 22, 2022

ouchadam Sep 26, 2022

sonarqubecloud bot commented Sep 26, 2022

ouchadam Sep 26, 2022

ouchadam Sep 26, 2022 •

edited

Loading

ouchadam Sep 26, 2022

ouchadam Sep 26, 2022

ouchadam Sep 26, 2022

bmarty Sep 27, 2022

ouchadam Sep 27, 2022

bmarty left a comment

ouchadam commented Sep 27, 2022


		messagesReceivedByBob.size shouldBe 1
		messagesReceivedByBob.size shouldBeEqualTo 1

Instrumentation test coroutines #7207

Instrumentation test coroutines #7207

Conversation

ouchadam commented Sep 22, 2022 • edited Loading

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

ouchadam commented Sep 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Sep 26, 2022

Choose a reason for hiding this comment

ouchadam Sep 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

ouchadam commented Sep 27, 2022

ouchadam commented Sep 22, 2022 •

edited

Loading

ouchadam Sep 26, 2022 •

edited

Loading