Skip to content

Improve test handling in CI #6025

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
michaelkaye merged 12 commits into from
May 18, 2022
Merged

Improve test handling in CI #6025

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
e06682d
@Ignore all tests currently failing in CI
michaelkaye May 11, 2022
012b20f
changelog.d
michaelkaye May 11, 2022
010be91
Fix threading UI test failure by adding a sleep 30s.
michaelkaye May 12, 2022
78140af
Crypto tests are failing due to slow initialSync. Increase timeout by…
michaelkaye May 13, 2022
70682b4
Increase timeout. Log timeout.
michaelkaye May 13, 2022
fa26e2a
Revert "@Ignore all tests currently failing in CI"
michaelkaye May 13, 2022
4ced6ca
Fix linting error.
michaelkaye May 13, 2022
8950aa3
Address repeated initializatin of WorkManagerImpl in #5887
michaelkaye May 16, 2022
096cf92
Rather than ignore them, put tests on a retry loop.
michaelkaye May 16, 2022
a19c1d6
Make test consistent wiht assert message.
michaelkaye May 16, 2022
1f89cfb
Lint fixes
michaelkaye May 16, 2022
868c33a
Correct var -> val for @get:Rules
michaelkaye May 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/6025.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
@Ignore a number of tests that are currently failing in CI.
7 changes: 5 additions & 2 deletions matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/common/CommonTestHelper.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ class CommonTestHelper(context: Context) {
val session = (registrationResult as RegistrationResult.Success).session
session.open()
if (sessionTestParams.withInitialSync) {
syncSession(session, 60_000)
syncSession(session, 120_000)
}
return session
}
Expand Down Expand Up @@ -378,7 +378,10 @@ class CommonTestHelper(context: Context) {
* @throws InterruptedException
*/
fun await(latch: CountDownLatch, timeout: Long? = TestConstants.timeOutMillis) {
assertTrue(latch.await(timeout ?: TestConstants.timeOutMillis, TimeUnit.MILLISECONDS))
assertTrue(
"Timed out after " + timeout + "ms waiting for something to happen. See stacktrace for cause.",
latch.await(timeout ?: TestConstants.timeOutMillis, TimeUnit.MILLISECONDS)
)
}

suspend fun retryPeriodicallyWithLatch(latch: CountDownLatch, condition: (() -> Boolean)) {
Expand Down
2 changes: 1 addition & 1 deletion matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/common/TestConstants.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ object TestConstants {
const val TESTS_HOME_SERVER_URL = "http://10.0.2.2:8080"

// Time out to use when waiting for server response.
private const val AWAIT_TIME_OUT_MILLIS = 60_000
private const val AWAIT_TIME_OUT_MILLIS = 120_000

// Time out to use when waiting for server response, when the debugger is connected. 10 minutes
private const val AWAIT_TIME_OUT_WITH_DEBUGGER_MILLIS = 10 * 60_000
Expand Down
10 changes: 8 additions & 2 deletions matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/common/TestMatrix.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ import android.os.Handler
import android.os.Looper
import androidx.lifecycle.ProcessLifecycleOwner
import androidx.work.Configuration
import androidx.work.WorkManager
import androidx.work.impl.WorkManagerImpl
import androidx.work.impl.utils.taskexecutor.WorkManagerTaskExecutor
import com.zhuinden.monarchy.Monarchy
import org.matrix.android.sdk.BuildConfig
import org.matrix.android.sdk.api.MatrixConfiguration
Expand Down Expand Up @@ -66,7 +67,12 @@ internal class TestMatrix(context: Context, matrixConfiguration: MatrixConfigura
.setExecutor(Executors.newCachedThreadPool())
.setWorkerFactory(matrixWorkerFactory)
.build()
WorkManager.initialize(appContext, configuration)
val delegate = WorkManagerImpl(
context,
configuration,
WorkManagerTaskExecutor(configuration.taskExecutor)
)
WorkManagerImpl.setDelegate(delegate)
uiHandler.post {
ProcessLifecycleOwner.get().lifecycle.addObserver(backgroundDetectionObserver)
}
Expand Down
4 changes: 4 additions & 0 deletions ...dk-android/src/androidTest/java/org/matrix/android/sdk/internal/crypto/CryptoStoreTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,11 @@ import org.junit.Assert.assertEquals
import org.junit.Assert.assertNotEquals
import org.junit.Assert.assertNull
import org.junit.Before
import org.junit.Rule
import org.junit.Test
import org.junit.runner.RunWith
import org.matrix.android.sdk.InstrumentedTest
import org.matrix.android.sdk.common.RetryTestRule
import org.matrix.android.sdk.internal.crypto.model.OlmSessionWrapper
import org.matrix.android.sdk.internal.crypto.store.IMXCryptoStore
import org.matrix.android.sdk.internal.util.time.DefaultClock
Expand All @@ -37,6 +39,8 @@ private const val DUMMY_DEVICE_KEY = "DeviceKey"
@RunWith(AndroidJUnit4::class)
class CryptoStoreTest : InstrumentedTest {

@get:Rule var rule = RetryTestRule(3)
Copy link
Member

@bmarty bmarty May 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be val instead of val (= immutable). Applicable to all the occurrences on this PR

Copy link
Contributor Author

@michaelkaye michaelkaye May 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Have updated these.


private val cryptoStoreHelper = CryptoStoreHelper()
private val clock = DefaultClock()

Expand Down
4 changes: 4 additions & 0 deletions ...dk-android/src/androidTest/java/org/matrix/android/sdk/internal/crypto/E2eeSanityTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import org.amshove.kluent.fail
import org.amshove.kluent.internal.assertEquals
import org.junit.Assert
import org.junit.FixMethodOrder
import org.junit.Rule
import org.junit.Test
import org.junit.runner.RunWith
import org.junit.runners.JUnit4
Expand Down Expand Up @@ -57,6 +58,7 @@ import org.matrix.android.sdk.api.session.room.send.SendState
import org.matrix.android.sdk.api.session.room.timeline.TimelineSettings
import org.matrix.android.sdk.common.CommonTestHelper
import org.matrix.android.sdk.common.CryptoTestHelper
import org.matrix.android.sdk.common.RetryTestRule
import org.matrix.android.sdk.common.SessionTestParams
import org.matrix.android.sdk.common.TestConstants
import org.matrix.android.sdk.common.TestMatrixCallback
Expand All @@ -67,6 +69,8 @@ import java.util.concurrent.CountDownLatch
@LargeTest
class E2eeSanityTests : InstrumentedTest {

@get:Rule var rule = RetryTestRule(3)

/**
* Simple test that create an e2ee room.
* Some new members are added, and a message is sent.
Expand Down
2 changes: 0 additions & 2 deletions .../src/androidTest/java/org/matrix/android/sdk/internal/crypto/crosssigning/XSigningTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import org.junit.Assert.assertNull
import org.junit.Assert.assertTrue
import org.junit.Assert.fail
import org.junit.FixMethodOrder
import org.junit.Ignore
import org.junit.Test
import org.junit.runner.RunWith
import org.junit.runners.MethodSorters
Expand Down Expand Up @@ -143,7 +142,6 @@ class XSigningTest : InstrumentedTest {
}

@Test
@Ignore("This test will be ignored until it is fixed")
fun test_CrossSigningTestAliceTrustBobNewDevice() {
val cryptoTestData = cryptoTestHelper.doE2ETestWithAliceAndBobInARoom()

Expand Down
4 changes: 4 additions & 0 deletions ...id/src/androidTest/java/org/matrix/android/sdk/internal/crypto/gossiping/KeyShareTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import org.amshove.kluent.internal.assertEquals
import org.junit.Assert
import org.junit.Assert.assertNull
import org.junit.FixMethodOrder
import org.junit.Rule
import org.junit.Test
import org.junit.runner.RunWith
import org.junit.runners.MethodSorters
Expand All @@ -43,6 +44,7 @@ import org.matrix.android.sdk.api.session.room.model.create.CreateRoomParams
import org.matrix.android.sdk.api.session.room.timeline.getLastMessageContent
import org.matrix.android.sdk.common.CommonTestHelper
import org.matrix.android.sdk.common.CryptoTestHelper
import org.matrix.android.sdk.common.RetryTestRule
import org.matrix.android.sdk.common.SessionTestParams
import org.matrix.android.sdk.common.TestConstants

Expand All @@ -51,6 +53,8 @@ import org.matrix.android.sdk.common.TestConstants
@LargeTest
class KeyShareTests : InstrumentedTest {

@get:Rule var rule = RetryTestRule(3)

@Test
fun test_DoNotSelfShareIfNotTrusted() {
val commonTestHelper = CommonTestHelper(context())
Expand Down
8 changes: 6 additions & 2 deletions ...id/src/androidTest/java/org/matrix/android/sdk/internal/crypto/gossiping/WithHeldTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import androidx.test.ext.junit.runners.AndroidJUnit4
import androidx.test.filters.LargeTest
import org.junit.Assert
import org.junit.FixMethodOrder
import org.junit.Rule
import org.junit.Test
import org.junit.runner.RunWith
import org.junit.runners.MethodSorters
Expand All @@ -38,6 +39,7 @@ import org.matrix.android.sdk.api.session.room.getTimelineEvent
import org.matrix.android.sdk.common.CommonTestHelper
import org.matrix.android.sdk.common.CryptoTestHelper
import org.matrix.android.sdk.common.MockOkHttpInterceptor
import org.matrix.android.sdk.common.RetryTestRule
import org.matrix.android.sdk.common.SessionTestParams
import org.matrix.android.sdk.common.TestConstants

Expand All @@ -46,6 +48,8 @@ import org.matrix.android.sdk.common.TestConstants
@LargeTest
class WithHeldTests : InstrumentedTest {

@get:Rule var rule = RetryTestRule(3)

@Test
fun test_WithHeldUnverifiedReason() {
val testHelper = CommonTestHelper(context())
Expand Down Expand Up @@ -102,7 +106,7 @@ class WithHeldTests : InstrumentedTest {
val type = (failure as MXCryptoError.Base).errorType
val technicalMessage = failure.technicalMessage
Assert.assertEquals("Error should be withheld", MXCryptoError.ErrorType.KEYS_WITHHELD, type)
Assert.assertEquals("Cause should be unverified", WithHeldCode.UNAUTHORISED.value, technicalMessage)
Assert.assertEquals("Cause should be unverified", WithHeldCode.UNVERIFIED.value, technicalMessage)
}

// Let's see if the reply we got from bob first session is unverified
Expand Down Expand Up @@ -143,7 +147,7 @@ class WithHeldTests : InstrumentedTest {
val type = (failure as MXCryptoError.Base).errorType
val technicalMessage = failure.technicalMessage
Assert.assertEquals("Error should be withheld", MXCryptoError.ErrorType.KEYS_WITHHELD, type)
Assert.assertEquals("Cause should be unverified", WithHeldCode.UNAUTHORISED.value, technicalMessage)
Assert.assertEquals("Cause should be unverified", WithHeldCode.UNVERIFIED.value, technicalMessage)
}

testHelper.signOutAndClose(aliceSession)
Expand Down
20 changes: 20 additions & 0 deletions vector/src/androidTest/java/im/vector/app/ui/robot/ElementRobot.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,26 @@ class ElementRobot {
.perform(ViewActions.closeSoftKeyboard(), click())
}
}
// at this point we are in a race with the app restarting. The steps that happen are:
// - (initially) app has started, app has initial synched
// - (restart) app has strted, app has not initial synched
// - (racey) app shows some UI but overlays with initial sync ui
// - (initial sync finishes) app has started, has initial synched

// We need to wait for the initial sync to complete; but we can't
// use waitForHome() like login does.

// waitForHome() -- does not work because we have already fufilled the initialSync
// so we can racily have an IllegalStateException that we have transitioned from busy -> idle
// but never having sent the signal.

// So we need to not start waiting for an initial sync until we have restarted
// then we do need to wait for the sync to complete.

// Which is convoluted especially as it involves the app state refreshing
// so; in order to make this be more stable
// I hereby cheat and write:
Thread.sleep(30_000)
Copy link
Contributor Author

@michaelkaye michaelkaye May 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be clear: i feel bad about this but i experimented with various options for ~1day and none were more likely to work than this - handling the restart requirement just makes it hard to test with.

Copy link
Contributor

@ouchadam ouchadam May 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems fair, it sounds like we're missing registering the sync as an IdlingResource in order for espresso to be able to wait for the sync to finish

there's a helper for withRetry if we wanted to poll for the home appearing instead of a blanket 30 second sleep (assuming the original check wasn't providing false positives)

fun waitForHome() {
  // each attempt has a 500ms delay
  withRetry(times = 60)
  ... original logic
  }
}

Copy link
Contributor Author

@michaelkaye michaelkaye May 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that waitForHome() is using an idling resource on initial sync; but it's idle at two points: Immediately (+/- thread racing) after we return from getting espresso to click the toggle, and then it's busy for 10-15s until after the app has restarted and done the initial sync.

}
else -> {
}
Expand Down