Fix: Instrumented tests are executed in Drone CI again#16612
Fix: Instrumented tests are executed in Drone CI again#16612alperozturk96 merged 13 commits intomasterfrom
Conversation
|
|
||
| debug { | ||
| enableUnitTestCoverage = project.hasProperty("coverage") | ||
| enableAndroidTestCoverage = project.hasProperty("coverage") |
There was a problem hiding this comment.
@tobiasKaminsky with this, Drone now actually runs the instrumented tests again. See https://drone.nextcloud.com/nextcloud/android/28585/1/3 as a proof of failure, i.e. broken tests on master which were never detected.
I'll go ahead addressing the test failures. But there is several other things I noticed:
- We should have Drone only run the instrumentation tests.
Currently, drone CI runs everything for which coverage is enabled. Why this is chosen as filter, I don't know. But what this means is that Drone wastes time on the unit tests which we already run elsewhere.- Unit tests were the only tests Drone ran since Chore: Improve Gradle Configuration #15859. Another weird side-effect of that PR.
Easiest for saving drone time would be to disable the coverage for unit tests in the line above, but I guess this is not desired?
- Actually, what about the coverage reporting? It doesn't appear anymore. It looks broken, in several ways: - I am not seeing the coverage report on any PR I have recently checked. Don't know when this stopped. Many of the pulls on Codecov just show errors: https://app.codecov.io/github/nextcloud/android/pulls
- The unit test job seems to still upload to Codecov, but prints out wrong links in the log, e.g.https://app.codecov.io/github/***/android/commit/2c1438fba2fda500d3ec2e330d2aa62284ef8e2c (log) .
- You need to replace the
***withnextcloud, then it works. Is this redaction of secrets at play - if so, I surely hope, no secret token has simply "nextcloud" as value?
- Coverage reporting should also be extended by the instrumentation tests. I don't see them uploading anything
There was a problem hiding this comment.
regarding
We should have Drone only run the instrumentation tests
This seems to be the case already 🤔 I don't know how this works, but adding coverage to the androidTests somehow removed the unitTests automatically. Quite unexpected, but the outcome is the desired one, i.e. Drone is not executing unit tests anymore.
|
|
||
| if [ $TYPE = "IT" ]; then | ||
| FOLDER=app/build/reports/androidTests/connected/flavors/gplay | ||
| FOLDER=app/build/reports/androidTests/connected/debug/flavors/gplay |
There was a problem hiding this comment.
@tobiasKaminsky these adjustments are needed so the report files are actually found. But reporting still is broken, which you can see e.g. in the log of https://drone.nextcloud.com/nextcloud/android/28585/1/3:
It does upload the report to https://www.kaminsky.me/nc-dev/android-integrationTests/28585-IT-stable-10-44 but it fails to create the expected Github comment notifying the developer of this.
The error is:
{
"message": "Must have admin rights to Repository.",
"documentation_url": "https://docs.github.com/rest/issues/comments#create-an-issue-comment",
"status": "403"
}
There was a problem hiding this comment.
@tobiasKaminsky I am not sure what to do about this, and about the discussion thread above, where I also pinged you.
ffaabe4 to
f0ee559
Compare
app/src/androidTest/java/com/nextcloud/utils/UploadDateTests.kt
Outdated
Show resolved
Hide resolved
c468198 to
ca5f5d0
Compare
|
@tobiasKaminsky Could you please share your feedback regarding script changes? |
ca5f5d0 to
89436bf
Compare
alperozturk96
left a comment
There was a problem hiding this comment.
Thank you so much 👍
app/src/androidTest/java/com/nextcloud/utils/AutoRenameTests.kt
Outdated
Show resolved
Hide resolved
f24f4ae to
1793be8
Compare
1793be8 to
25e9a97
Compare
alperozturk96
left a comment
There was a problem hiding this comment.
Hey, I updated the PR. Could you please check these tests?
Thank you for the swift reply.
com.******.utils.UploadDateTests > getRelativeDateTimeStringReturnsFutureAsAbsoluteWhenShowFutureIsFalse[android(AVD) - 16] FAILED
org.junit.ComparisonFailure: expected:<Mar 10, 2026 [0]8:14:50 AM> but was:<Mar 10, 2026 []8:14:50 AM>
at org.junit.Assert.assertEquals(Assert.java:117)
android(AVD) - 16 Tests 339/569 completed. (0 skipped) (6 failed)
com.owncloud.android.datamodel.UploadStorageManagerTest > largeTest[android(AVD) - 16] FAILED
java.lang.AssertionError
at org.junit.Assert.fail(Assert.java:87)
The Drone CI job is running ./gradlew createGplayDebugCoverageReport This only executes tests with coverage. In 5fd2e29, the line testCoverageEnabled = project.hasProperty("coverage") was replaced with enableUnitTestCoverage = project.hasProperty("coverage") which only left the unit tests with coverage Enabling the coverage for androidTest (the instrumented tests) makes Drone run them again. Closes #16350 Signed-off-by: Philipp Hasper <vcs@hasper.info>
The drone tests simply ended with the log line: stable-IT test failed, but no output was generated. Maybe a preliminary stage failed. The reason was that the script didn't use the correct folder Signed-off-by: Philipp Hasper <vcs@hasper.info>
1. SetOnlineStatusBottomSheetIT: Checked for the wrong view, at the wrong point in time 2. SetStatusMessageBottomSheetIT: Fragment transactions are scheduled asynchronously, so you couldn't set the status inside the same scenario where show() is shown. Now, the setPredefinedStatus() method instead pre-stores the status and only updates the adapter if available Signed-off-by: Philipp Hasper <vcs@hasper.info>
The test-stable failed in the skipAutoRenameWhenWCFDisabled test, while test-master succeeded. This is due to the different server versions. At the time of writing, stable is using version 30 and master is using version 32. The WCF handling differs per version, as detailed in the OCCapabilityExtensions.kt file. Hence the test needs to adjust the server version. While we are on it, we are testing the three different cases Signed-off-by: Philipp Hasper <vcs@hasper.info>
The tests themselves ran through, but immediately after the android process crashes. Bluntly mocking the whole System class causes problems everywhere. Even limiting the mocking to a single method still causes the crashes. The issue is that mocking System.currentTimeMillis() on Android is fundamentally unsafe, even with a function reference. The mock interferes with framework internals (WorkManager, schedulers, etc.) that run after the test completes, causing the process crash. Mocking the System class must be avoided. Signed-off-by: Philipp Hasper <vcs@hasper.info>
The android utility used underneath, DateUtils.getRelativeDateTimeString, documents its minResolution parameter like this: minResolution – the minimum elapsed time (in milliseconds) to report when showing relative times. For example, a time 3 seconds in the past will be reported as "0 minutes ago" if this is set to MINUTE_IN_MILLIS. So our way of using SECOND_IN_MILLIS was wrong Signed-off-by: Philipp Hasper <vcs@hasper.info>
Signed-off-by: Philipp Hasper <vcs@hasper.info>
Signed-off-by: alperozturk96 <alper_ozturk@proton.me>
Signed-off-by: alperozturk96 <alper_ozturk@proton.me>
Signed-off-by: alperozturk96 <alper_ozturk@proton.me>
Signed-off-by: alperozturk96 <alper_ozturk@proton.me>
Signed-off-by: Philipp Hasper <vcs@hasper.info>
The issue was that uploadsStorageManager.uploadDao.insertOrReplace() generates a new uploadId, as documented in UploadEntityKt.toUploadEntity(), leading to a failure of contains(uploads, storedUploads[i]). In the test, we need to compare the objects without comparing the uploadId Signed-off-by: Philipp Hasper <vcs@hasper.info>
7aeb131 to
0bb255f
Compare
|
@alperozturk96 I fixed the wrong timestamp string. As I only develop after work, I never noticed that the hour of day does not have a trailing zero 😄 Also, I fixed the next failure, which was of In general, it looks like that the tests cannot complete, not even upload the test failure report, because they are running into a timeout. Maybe not executing the unit tests (see my thread above) will buy us the necessary time |
|
blue-Light-Screenshot test failed, but no output was generated. Maybe a preliminary stage failed. |
|
APK file: https://www.kaminsky.me/nc-dev/android-artifacts/16612.apk |
|
@alperozturk96 thank you for the collaboration on this. Was it merged now on purpose? Because - see my previous message - the Drone runs are now consistently failing. They run into the 1 hour timeout. Did you plan to address this in a follow-up MR? Also @tobiasKaminsky I think the open discussions above are still relevant: |
Hey this happens for master as well and since these changes not harming something else I think it's okay to merge. We can fix other issues in different PRs. Thank you for your contribution. |
sorry to be annoying but no, the tests running into an 1 hour timeout is only happening since merging this here. The last merge before that, in #16622, had an almost successful Drone run: https://drone.nextcloud.com/nextcloud/android/28739/2/1 . You can see it took only 9 minutes for test-stable and 20 minutes for test-master. The one failure in test-master I would say is a spurious failure only. Since merging the PR here, the drone tests finally test what they suppose to test. But they timeout, so they are still not useful. I wonder how to address this 🤔 |
The Drone CI job is running ./gradlew createGplayDebugCoverageReport This only executes tests with coverage.
In 5fd2e29 (PR #15859), the line
was replaced with
which only left the unit tests with coverage
Enabling the coverage for androidTest (the instrumented tests) makes Drone run them again.
Closes #16350
🏁 Checklist