Run android integration tests in emulator#2036
Run android integration tests in emulator#2036rPraml wants to merge 21 commits intomozilla:masterfrom
Conversation
|
@rPraml I see you tested with and without the fix and got the same performance. This actually makes sense - the JIT compilation failure is environment-specific and typically occurs on:
The issue in #1365 was reported on real devices where the interpretLoop method exceeds the JIT threshold. Your emulator environment might have different or relaxed limits, which is why you're This reinforces why the fix is needed - it prevents the issue on affected devices while having no impact on environments that don't hit the threshold (like your emulator). |
|
Yes, until now I could not reproduce the issue.
It seems that this bug is very tricky. Maybe I can retry on an older emulator version (although this would be x86)
And it would be good, to have a test for this to avoid a regression in future releases, as the interpreter loop is code, that will be modified by a lot of people.
@anivar I know, you are busy, but maybe you can provide details on which android device (android + SDK version) you were able to reproduce the issue. Maybe this is reproduceable in the emulator with same sdk (even if it is x86 and not arm) |
9c60cf6 to
1b586ee
Compare
rPraml
left a comment
There was a problem hiding this comment.
This is an initial (and working) solution for how Android tests could be executed in the github-CI pipeline.
The goal is to prevent regressions, which have repeatedly occurred in the past.
Tests can be easily added in the style of the ‘MozillaTestSuite’ as a JavaScript file in src/main/assets/tests, and an app (APK file) can also be built, which can be copied onto a phone to run the tests.
The CI-pipeline uses https://github.com/ReactiveCircus/android-emulator-runner
Additionally, with the budtmo/docker-android:emulator we have an easy to use solution for local debugging
I'd really appreciate community feedback on how we should proceed from here.
it-android.sh
Outdated
| #docker exec -it $CONTAINER_NAME adb install -r -t $APP && \ | ||
| #docker exec -it $CONTAINER_NAME adb shell am start -n com.example.rhino/com.example.rhino.MainActivity | ||
| ./gradlew it-android:connectedAndroidTest | ||
| ./gradlew it-android:installDebug |
There was a problem hiding this comment.
TODO: Would be nice, if someone else can confirm, these scripts will work
| } | ||
|
|
||
| if (!System.getenv("ANDROID_HOME") && !localProperties.get("sdk.dir")) { | ||
| System.out.println("No Android SDK found. Skipping build") |
There was a problem hiding this comment.
Skip android build for users, that do not have a SDK installed
| @@ -0,0 +1,417 @@ | |||
| // Copyright 2008 the V8 project authors. All rights reserved. | |||
There was a problem hiding this comment.
This is a 1:1 copy of assert.js from MozillaTestSuite
it-android/src/androidTest/java/org/mozilla/javascript/android/test/RhinoTest.java
Show resolved
Hide resolved
| new ContextFactory() { | ||
| @Override | ||
| protected boolean hasFeature(Context cx, int featureIndex) { | ||
| if (featureIndex == Context.FEATURE_ENABLE_XML_SECURE_PARSING) return false; |
There was a problem hiding this comment.
Secure-parsing cannot be changed on android-devices
|
@rPraml Thanks for this Android testing infrastructure! The JIT issue from #2018 occurs when template literals generate methods exceeding ~7392KB, causing ART compiler failures on certain Android devices. Key factors:
Suggestions for your framework:
The fix prevents the issue, but automated tests will help catch regressions. |
|
@rPraml Great work on the implementation details! Here's feedback on your specific questions: KVM rules (gradle.yml): Yes, needed for hardware acceleration in GitHub Actions runners. Without it, emulators run extremely slow. Build integration: Run as a separate matrix job, not part of Java 21 build. This allows:
SDK installation script: Keep it. Not everyone has Android SDK locally, and it ensures consistent SDK versions across contributors. API Level 33: While Play Store requires API 33, for testing we should include older versions too: minSdk = 26 // Android 8.0 where JIT issues start
targetSdk = 33Architecture suggestion: Add ARM to your emulator matrix since x86_64 won't catch ARM-specific JIT issues: strategy:
matrix:
api-level: [26, 28, 30, 33]
arch: [x86_64, arm64-v8a]The local Docker approach is excellent for debugging. Overall, this is a solid foundation! |
1a8e208 to
a799cb8
Compare
|
This is great progress -- I know it's a pain to get this in. I think that we should create a separate GitHub Action for Android, rather than putting it in the main tests. That might make it easier to develop, and also it can run in parallel. We still won't merge anything until all the checks have passed. Also, I personally don't use Docker, but Podman, and I wonder if others do too? If so I may try to adapt the script (should just be replacing "docker" with "podman"). |
|
minsdk should be 21, 26 is too high. |
|
CI failure under minSdk 21 is a regression introduced by PR #1785. Switching the minSDK back to 26 is not the correct solution, as this will prevent Android projects with a minSDK of 21 from compiling, resulting in the same failure as the CI. |
|
This task gave me a much better understanding of how Android works under the hood. (For example, I wasn’t aware that the entire rhino.jar is converted by dx into a completely different Dalvik bytecode format and isn’t bundled into the .apk as a regular .jar file.) Summary The goal of this pull request is to provide the infrastructure for Android tests (we are not aiming to fix any bugs at this stage.) ✅ Tests run successfully for API levels 26, 28, 30, and 33 on x86_64 Next Steps
|
|
@rPraml Thank you for this comprehensive Android testing infrastructure! Your deep dive into the dx/Dalvik bytecode conversion and the testing setup is excellent work. I've been thinking about the DexMaker suggestion and wondering if there might be alternative approaches that better align with Rhino's strengths and modern Android development: On DexMaker: Alternative Approaches to Consider:
On the Module Split: Given that Dalvik was replaced by ART in Android 5.0 (2014), perhaps a simpler approach would be a single module with runtime feature detection rather than rhino-jvm/rhino-dalvik? On Minimum API Level: Your testing infrastructure gives us exactly what we need to make these decisions based on real data. The ability to run tests across multiple API levels is invaluable. What do you think about focusing the Android strategy on Rhino's unique advantages (pure Java, no JNI, excellent Java interop) rather than trying to match the performance of V8 or QuickJS? |
gbrail
left a comment
There was a problem hiding this comment.
Thanks! I get where this is going and it is going to help a lot.
So I can understand -- if I run the "install-android-sdk" script, and then run the tests, will it use the SDK to run the tests on Android, or do I need to do the whole docker / podman setup for that?
I'm asking because installing the SDK works great, and the tests work fine in either way, but getting that docker / podman script to work portably across OSse and other things is going to take some work -- I had to muck around to make it work with Windows and MSYS but there are probably five other combinations that people use. Do we get some value out of this if we don't need people to run the container, but if the CI job does?
| @@ -0,0 +1,15 @@ | |||
| #/usr/bin/env bash | |||
There was a problem hiding this comment.
Can you please move this file to the "it-android" directory so that the root directory doesn't get too full?
Could you also make it executable (might need to mess around with Git -- usually if you "chmod a+x" the file Git will figure it out when you first add it).
| @@ -0,0 +1,40 @@ | |||
| #/usr/bin/env bash | |||
There was a problem hiding this comment.
Could you also move this file to it-android and make it executable?
7ae58e3 to
c858a5d
Compare
> Could not resolve all files for configuration
':it-android:debugRuntimeClasspath'.
> Failed to transform rhino-1.8.1-SNAPSHOT.jar (project :rhino) to
match attributes {artifactType=android-dex,
dexing-component-attributes=ComponentSpecificParameters(minSdkVersion=21,
debuggable=true, enableCoreLibraryDesugaring=false,
enableGlobalSynthetics=false, enableApiModeling=false,
dependenciesClassesAreInstrumented=false, asmTransformComponent=null,
useJacocoTransformInstrumentation=false, enableDesugaring=true,
needsClasspath=true, useFullClasspath=false,
componentIfUsingFullClasspath=null), org.gradle.category=library,
org.gradle.dependency.bundling=external, org.gradle.jvm.version=11,
org.gradle.libraryelements=jar, org.gradle.usage=java-runtime}.
> Execution failed for DexingWithClasspathTransform:
/home/runner/work/rhino/rhino/rhino/build/libs/rhino-1.8.1-SNAPSHOT.jar.
> Error while dexing.
Increase the minSdkVersion to 26 or above.
41b31a2 to
385674e
Compare
There was a problem hiding this comment.
This has to be reverted!
|
I'm going to close this PR as I've included all the commits in #2146 and I would like to merge that soon. Thanks! |
The trigger was PR #2018 and #1152, which prompted me to revisit running Android tests in an emulator.
It’s still not fully finished, but I wanted to share the concept anyway.
In short: What does this PR
install-android-sdk.shdownloads the SDK in RHINO_ROOT/android-sdk - if you do not already have one installed
it-android.shstarts an android emulator, the emulator is accessible over http://localhost:6080
it-androidthis project is a minimal android project, with embedded rhino (based on https://github.com/czak/minimal-android-project)
testsand outputs the result in a very minimalistic UI:Open TODOs
@anivar I adapted your performance tests and I get ~297ms for 10000 iterations, with or without your fix. So there is no difference in the emulator. Do we perform the correct test? Or do I still not understand, what the problem is
(What I've understood: On native android (Dalvik/ART), the performance is worse, on a linux JVM, the performance is OK)