-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Android CI improvements, improves #2892 #2903
Conversation
This should help to see why "Mockito cannot mock this class" happens.
…en in many of the examples.
…e infamous ternary workaround.
Codecov ReportBase: 85.65% // Head: 85.64% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #2903 +/- ##
============================================
- Coverage 85.65% 85.64% -0.02%
Complexity 2847 2847
============================================
Files 325 325
Lines 8623 8623
Branches 1060 1060
============================================
- Hits 7386 7385 -1
Misses 964 964
- Partials 273 274 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
…tps://github.com/search?q=api-level+emulator-build+33&type=code To expand on "too old": the version 7425822 mentioned in the issue spams 30MBs of vqParseGuestToHostRequestLocked:734 {src_port=1495992989 dst_port=4294967295} unexpected dst_port to the GitHub Actions logs.
… somehow not sharing variables, so just use files.
This reverts commit 5074da7.
This reverts commit a7771cc.
@@ -121,8 +117,54 @@ jobs: | |||
with: | |||
arch: x86_64 | |||
api-level: ${{ matrix.android-api }} | |||
target: ${{ matrix.android-image-type }} | |||
script: ./gradlew :androidTest:connectedCheck --no-daemon --no-build-cache | |||
# Workaround for https://issuetracker.google.com/issues/267458959 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks massive, is it worth the complexity? (vs building on 26
only)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
26 can have the same issues... it's just how emulators work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, most of the complexity is to get the logcat displayed and saved, which is necessary if you want to diagnose issues. This failure looks really ugly: https://github.com/mockito/mockito/actions/runs/4106511182/jobs/7085124984#step:4:674, and the fact that this can happen on this very simple project, means that it can happen randomly for anyone in the world using mockito. So the more info we can get to diagnose failures, the better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is indeed quite a bit of complexity for running a couple of Android tests. That said, if CI is more stable with this, then let's merge this as-is. Thanks for the investigation!
@TWiStErRob we are affected by the intermittent |
followup on #2893 and #2899, fixes #2892
Changes
emulator-build
for reproducibility and hopefully stability.emulator-options
to make the emulator more stable.Investigation details
My first idea (finding) was cold-boot, but apparently that's default behavior via
--no-snapshot
.Anyway, I had a look about recent failures, and found the below and made some change in hopes of a more stable CI:
Error: Timeout waiting for emulator to boot.
Job
Stumbled upon this issue instrumentation tests uses: reactivecircus/android-emulator-runner@v2 failing - Error: Timeout waiting for emulator to boot. ReactiveCircus/android-emulator-runner#160 in one of the comments in the example CIs. I guess I agree that locking in a known-good version is better than be at the mercy of unstable "stable" releases. Google has blindsided us with a "no-repro" at https://issuetracker.google.com/issues/191799887.
Fix: added explicit
emulator-build
Unknown failure: cmd: Can't find service: package
Job
Strange error, it looks like the emulator is in a bad state. This could happen because the GitHub Actions runners might be shared between different jobs, and the emulator might be in a bad state when we try to use it. This sounds weird, because I'm sure GHA ensures a clean state, but not sure about it. Anyway, I haven't found any other suggestion than to kill the emulator and cold-boot it. Which is achieved by the default
-no-snapshot
flag. The only option I see here is to retry somehow.Fix: N/A so far, 🤞
emulator-build
helps.Unknown failure: cmd: Failure calling service package: Broken pipe (32)
Job
Not a clue what's happening here, let's assume it's the "same" problem as above.
Retry might help in this case, but it's quite complex, not sure that we want to adopt this.
https://github.com/kiwix/kiwix-android/blob/develop/contrib/instrumentation.sh
Fix: N/A so far, 🤞
emulator-build
helps.Mockito cannot mock this class: interface org.mockitousage.androidtest.BasicInterface.
Job
Undiagnosable as per Set up Android Github Action pipeline. Fixes #2892 #2893 (comment). What we know: the emulator started, the Gradle build completed, and app deployed, and tests are discovered. At this point it's usually the production/test code that's wrong, but not reproducible locally.
Fix: Added artifact for the test results.
Note: I went through all referenced CIs here:
https://github.com/ReactiveCircus/android-emulator-runner#who-is-using-android-emulator-runner
to find some ideas on how to improve the Mockito CI.
Most of them used verbatim runners as we've set up, but some (major ones) used some extra options.
Additional findings, that I ignored for now:
Source: slackhq/keeper@ae8638a in Updates and add enableL8RuleSharing feature slackhq/keeper#58
Checklist
including project members to get a better picture of the change
commit is meaningful and help the people that will explore a change in 2 years
Fixes #<issue number>
in the description if relevantFixes #<issue number>
if relevant