-
-
Notifications
You must be signed in to change notification settings - Fork 30.9k
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
gh-116622: Add Android test script #121595
Conversation
@freakboy3742: Please review, and test that it works on your own machine. Instructions are in README.md. Some of the actual tests are currently failing, but I'll deal with that in a separate PR. |
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.
Mostly makes sense; and once I got it working, it did exactly what it said it would.
A couple of quirks I hit working through these instructions:
- The docs make no mention of
JAVA_HOME
; I got an error until I set that as well. I didn't need to havejava
on the path as the instructions suggest. - I can see there's an earlier reference to building x86_64, but I wasn't expecting that as a dependency for testing - especially in terms of maintaining a fast testing cycle. Ideally, the
test
target should only build the current architecture by default. - I had a few difficulties getting the test suite to run. On my first attempts, with both the
--managed
and--connected
options, I got the following in the console:
...
> Task :app:createDebugAndroidTestApkListingFileRedirect UP-TO-DATE
> Task :app:targetSdkSetup
> Task :app:targetSdkDebugAndroidTest
Serial: emulator-5556
Boot completed
but then no further output for 5 minutes. From some debugging, it looks like it was getting stuck interrogating for a UID of the test package; it's getting stuck in the loop polling adb shell pm
. It might be worth adding a safety catch to this loop so that it will actually stop (eventually) if something goes wrong.
- I'm not sure if the delay was due to a weird state in my emulator (It was behaving really weird, so I can't rule that out), or due to a "first run" delay with Gradle. I know from the Briefcase experience that the first Android project run can take ~10 mins to download the emulator images; and the way that the wrapper script is eating console output means even the (woefully inadequate) status bars that Gradle provides aren't visible.
- However, after a reboot, the test suite started fine with
--connected
with the Briefcase default emulator. However - it got to 127/478, and then crashed with an OOM error. I guess there's a need for guidance on what the minimum emulator specification is. - The
--managed
option was successful; it created an emulator, and started it without any problems; 4 test failures in the overall suite (test_android test_capi test_pathlib test_posixpath), but not for reasons that seem to be related to the runner itself.
I've dropped a couple of other comments inline.
In summary: most of the issues I'm seeing can be addressed with documentation, or a little more verbosity in the test runner script (or both).
One other minor style inconsistency - there's a couple of files with no trailing newline; they all seem to be Java-ecosystem files.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
I think either of them will work; I've updated the README to say that.
Good point – this has since been fixed in #122487.
From the name of the task, this must have been
I wasn't able to reproduce this. What did the error look like? And are you sure you're using the default emulator from the current version of Briefcase? I created one with the Briefcase main branch, and it shows the following settings in Android Studio:
Fixed. |
I've now added a |
I haven't been able to reproduce the OOM issue I saw last time; I think I'll have to chalk it up to the emulator being in an odd state (which it was, AFAICT). One UX note - When I ran this time with a warm, connected emulator, there was a delay of several minutes in the test startup. The delay was after the On second startup, the delay was only a couple of seconds - so I'm guessing the delay was gradle downloading something with no feedback to the user. It's not clear to me what might be downloaded at that point (or what point is triggering the download), but another "this might take a few minutes" log message might be called for. Managed emulators are another matter:
I get no error messages, but no test suite either. |
…n in non-verbose mose
Aside from the HACL issue above, the script is now working on macOS ARM64, Windows x86_64 and Linux x86_64. As usual, Linux was the most awkward, so I'll leave a few notes here for future reference (on Debian Bookworm):
|
I did some experimentation with
|
In one particular random order I also hit the OOM error you mentioned above. Increasing the emulator RAM from 2 GB to 4 GB was enough to allow it to pass, so I've added a note about this to the README. This occurred on commit 7a3d674, with arguments Log
Based on these issues with |
I've got one more error handling improvement to push, and then this should be ready to merge. |
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.
Looks good to me; passes every test run I've been able to throw at it, with appropriate messaging when things are slow due to initial installs etc.
Thanks @mhsmith for the PR, and @freakboy3742 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13. |
Sorry, @mhsmith and @freakboy3742, I could not cleanly backport this to
|
Thanks @mhsmith for the PR, and @freakboy3742 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13. |
Adds a script for running the test suite on Android emulator devices. Starting with a fresh install of the Android Commandline tools; the script manages installing other requirements, starting the emulator (if required), and retrieving results from that emulator. (cherry picked from commit f84cce6) Co-authored-by: Malcolm Smith <smith@chaquo.com>
GH-123061 is a backport of this pull request to the 3.13 branch. |
gh-116622: Add Android test script (GH-121595) Adds a script for running the test suite on Android emulator devices. Starting with a fresh install of the Android Commandline tools; the script manages installing other requirements, starting the emulator (if required), and retrieving results from that emulator. (cherry picked from commit f84cce6) Co-authored-by: Malcolm Smith <smith@chaquo.com>
|
As far as I can make out, this buildbot failure is unrelated to this PR; the same test has failed on other PRs recently (eg #122998). |
Adds a script for running the test suite on Android emulator devices. Starting with a fresh install of the Android Commandline tools; the script manages installing other requirements, starting the emulator (if required), and retrieving results from that emulator.
This PR builds on #117878 by adding an
android.py test
command, which can be used to run the Android testbed either in a buildbot or interactively during development.This initial version uses two threads: one to run Gradle and one to run
adb logcat
. But it has a number of bugs around interleaved output and unclean shutdowns. So I'm currently refactoring it to use asyncio instead.@freakboy3742: FYI. I'll request a review after I've done the refactor.