Skip to content
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.

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

Trying to fix integration tests #4842

Merged
merged 11 commits into from
Jan 12, 2022

Conversation

ariskotsomitopoulos
Copy link
Contributor

@ariskotsomitopoulos ariskotsomitopoulos commented Jan 3, 2022

Fixing Integration test

The main problem is that GitHub actions is that it has limited resources and is not meant for that kind of jobs. In order to properly do a robust 100% solution we should welcome another CI/CD tool like Jenkins and trigger the build from here

For further improvements we should change the actual implementation of the integration tests along with the shared resources

  • Synapse server can now start, it was broken
  • Fix/Improve running them on GitHub
  • Mark the not working tests with @ignore so that they will not run neither local or on github actions
  • Advanced parsing the result to add a user friendly comment on the PR with the results
  • Increase adb ram & also tried caching the adb

On mac slave it works much better and consistent

As you can see here the tests can run and the results are published like below:
Screenshot 2022-01-07 at 17 05 02

@github-actions
Copy link

github-actions bot commented Jan 3, 2022

Unit Test Results

  66 files  ±0    66 suites  ±0   54s ⏱️ +3s
135 tests ±0  135 ✔️ ±0  0 💤 ±0  0 ±0 
418 runs  ±0  418 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 8adeab0. ± Comparison against base commit 1490854.

♻️ This comment has been updated with latest results.

…ery very limited for that)

- Split them in msdk and app test along with multiple smaller steps.
- Mark the not working tests with @ignore so that they will not run neither local or on github actions
- Add user friendly comment on PR to view the results
@ariskotsomitopoulos ariskotsomitopoulos force-pushed the feature/aris/integration_tests_improvement branch from 9221904 to 6a24e02 Compare January 5, 2022 21:45
@element-hq element-hq deleted a comment from github-actions bot Jan 5, 2022
@element-hq element-hq deleted a comment from github-actions bot Jan 5, 2022
@github-actions
Copy link

github-actions bot commented Jan 5, 2022

Matrix SDK

Integration Tests Results:

  • [org.matrix.android.sdk.session]
    passed="21" failures="8" errors="0" skipped="2"
  • [org.matrix.android.sdk.account]
    passed="1" failures="1" errors="0" skipped="0"
  • [org.matrix.android.sdk.internal]
    passed="1" failures="1" errors="0" skipped="0"
  • [org.matrix.android.sdk.ordering]
    passed="1" failures="1" errors="0" skipped="0"
  • [org.matrix.android.sdk.PermalinkParserTest]
    passed="1" failures="1" errors="0" skipped="0"

@ariskotsomitopoulos ariskotsomitopoulos marked this pull request as ready for review January 7, 2022 10:09
@bmarty
Copy link
Member

bmarty commented Jan 7, 2022

We will have to update the GitHub setting to make the actions required:

  • Integration Tests / Matrix SDK - Build Android Tests (pull_request) - it was a temporary GitHub action, which can be removed now
  • Integration Tests / Matrix SDK - Running Integration Tests (28) (pull_request) - now passing!
  • Integration Tests / App - Build Android Tests (pull_request) - not required anymore, maybe because it has been renamed? Anyway it was a temporary GitHub action, which can be removed now)

Also the action Compile Android tests seems stuck.

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Nice work, thanks a lot for working on that.
Some comment/questions.

jobs:
# Build Android Tests [Matrix SDK]
build-android-test-matrix-sdk:
name: Matrix SDK - Build Android Tests
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to keep this action since it is now done when we run the integration test?
It has been added (see the comment here) to ensure that the tests are compiling by making this action "Required" on every PRs.
But now I think we can put the test run "Required".

Thinking more about it, since this action is more reliable than the running of integration test, maybe we can keep it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly thats why I kept it. While its not 100% reliable I think we should keep it

@@ -29,6 +29,7 @@ def vanniktechEmoji = "0.8.0"
def mockk = "1.12.1"
def espresso = "3.4.0"
def androidxTest = "1.4.0"
def androidxOrchestrator = "1.4.1"
Copy link
Member

Choose a reason for hiding this comment

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

Why not just update the value of androidxTest above to 1.4.1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As you can see below only androidxOrchestrator is yet it 1.4.1
Screenshot 2022-01-07 at 17 53 50

Copy link
Member

Choose a reason for hiding this comment

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

You are right, I read a bit fast the title at https://developer.android.com/jetpack/androidx/releases/test#core_141_2

Also confirmed that this should not be a problem to use various version by the sample at https://developer.android.com/jetpack/androidx/releases/test#declaring_dependencies

return
} catch (t: Throwable) {
caughtThrowable = t
Log.e(TAG, description.displayName + ": run " + (i + 1) + " failed")
Copy link
Member

Choose a reason for hiding this comment

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

Maybe also log the exception t?

Copy link
Member

Choose a reason for hiding this comment

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

Also can we use Timber here? (Maybe not)

@@ -39,12 +41,14 @@ import org.matrix.android.sdk.internal.crypto.model.event.WithHeldCode

@RunWith(AndroidJUnit4::class)
@FixMethodOrder(MethodSorters.JVM)
@LargeTest
Copy link
Member

Choose a reason for hiding this comment

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

Where are effectively use this annotation LargeTest (and the other ones)? It's maybe not in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we are not currently using those annotations. The plan is to start using this annotation to our tests when everything is annotated. I have annotated some of the tests but not all of them

@@ -528,7 +547,6 @@ class SASTest : InstrumentedTest {
val aliceDeviceInfoFromBobPOV: CryptoDeviceInfo? = bobSession.cryptoService().getDeviceInfo(aliceSession.myUserId, aliceSession.cryptoService().getMyDevice().deviceId)

// latch wait a bit again
Thread.sleep(1000)
Copy link
Member

Choose a reason for hiding this comment

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

Please also remove the comment above

@@ -31,8 +33,13 @@ import org.matrix.android.sdk.common.CommonTestHelper
import org.matrix.android.sdk.common.CryptoTestHelper
import java.util.concurrent.CountDownLatch

/** !! Not working with the new timeline
* Disabling it until the fix is made
Copy link
Member

Choose a reason for hiding this comment

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

CC @ganfra

@github-actions
Copy link

Ktlint Results

👍 ✅ 👍

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Thanks for the update

@bmarty bmarty merged commit f8afe04 into develop Jan 12, 2022
@bmarty bmarty deleted the feature/aris/integration_tests_improvement branch January 12, 2022 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants