WordPress-Android icon indicating copy to clipboard operation
WordPress-Android copied to clipboard

[Tooling] Fix raw screenshots automation

Open AliSoftware opened this issue 3 years ago • 6 comments

Fixing various issues in the screenshot automation.

Warning: This PR still depends on https://github.com/wordpress-mobile/release-toolkit/pull/409 so we will need to land that one in release-toolkit, then update the fastlane/Pluginfile after we created a new toolkit release, before we can merge this one here.

See also this update comment summarizing the changes once I considered the PR ready for review.

Raw Screenshots

  • [x] 👷 Completely reimplement the logic to launch emulators using Ruby and Fastlane
    • [x] Implement code to install the correct system-image depending on the API level we need + the current machine's architecture (Intel vs M1), using sdkmanager
    • [x] Re-implement the logic to create an AVD from a device, api level and system-image, using avdmanager instead of the copy-device.sh + fixup-android-emulator.sh shell scripts
    • [x] Clean up and get rid of the fastlane/emulators/*.ini and associated scripts
    • [x] Re-Implement the logic to kill previous emulators and launch a new emulator given an AVD, with the proper parameters
    • [x] Move the new code from fastlane/helper/android_emulator_helper.rb to the release-toolkit once it's confirmed to be stable --> https://github.com/wordpress-mobile/release-toolkit/pull/409
    • [x] Re-Implement the screenshots lane (taking the raw screenshots) and make it call the built-in fastlane action (capture_android_screenshots) directly now instead of our local fastlane/action/take_android_emulator_screenshots.rb one
  • [x] ⬆️ Update Screengrab library to latest version
  • [x] 🔧 Fix the AndroidManifest.xml permissions for debug
  • [x] 🕵️ Investigate and fix crash on WPLocaleTestRule
    • [x] 🐛 Fix NPE crash with the Context not yet initialized in the AppInitializer.kt singleton and WordPress.kt singleton by the time the test rule is applied — with the help of @ovitrif (see those commits)
    • [x] 🔧 Align our custom implementation of WPLocaleTestRule with the new driving logic used by latest fastlane/screengrab version
    • [x] ⛓️ Use a RuleChain of fastlane's LocaleTestRule + our own WPLocaleTestRule to apply one on top of the other — and ensure fastlane's own internal logic in their LocaleTestRule implementation is called and not shadowed by our own rewriting of it as before
  • [x] 🕵️ 🐛 Investigate why passing fr-FR doesn't change the locale (but fr does).
    • According to fastlane's doc, we're supposed to pass fr-FR, de-DE, etc to the locale parameter, and that will end up being the name of the locale folder used in fastlane/metadata/.
    • But in practice it seems that Screengrab's LocaleUtil.localeFromString expects something else (is it an Android-like de-rDE / zh-rCN format? Or is it failing to find the locale for some other reason?). And if we pass a different locale code which works (like just fr instead of fr-FR) and changes the UI appropriately… then the fastlane/* locale folder that will be created will be incorrect…
    • ⚠️ This might be tricky to debug, since setting a -e testLocale Instrumentation Argument (from "Run/Debug Configurations" window) from Android Studio doesn't seem to work (the test locale that is read from the test and the LocaleTestRule returns null when running the test from Android Studio), and the passing of the testLocale only works when invoking the test using am instrument -e testLocale … from the command line (like fastlane's screengrab does)… but when running form the CLI we can't debug to inspect how the test locale is (incorrectly) parsed. And all the parsing methods that might be at fault are private to the Screengrab binary library, so that doesn't help debugging via some test code either.
  • [ ] 🕵️ Investigate why the JPScreenshotTest crashes on ZenDesk initialization (see below)
    • ➡️ Tracked separately https://github.com/wordpress-mobile/WordPress-Android/issues/17130
  • [x] 🐛 Check the case of zh-CN which seems to prompt if we want to "Allow Google Pinyin input method" when the app starts, which interrupts the UI Test (see screenshot below). Can we use adb shell to force-disable this prompt?
  • [ ] 👷 Make the raw screenshots be saved in a separate temporary folder for WordPress vs Jetpack (see FIXMEs in 6eec4cfbbf5b155ae770956a176902cea481fbff)
    • ➡️ Kept for a future refinement PR
  • [x] Test again the whole pipeline of running fastlane screenshots for multiple devices and multiple locales and ensure it switches from one to another as expected (i.e. that there's no leak across test run boundaries) and saves the PNGs in the expected folders

Promo Screenshots

I will address those ones in a _separate_ PR, to keep that one focused only on fixing the raw screenshots.
  • [ ] Test the lane (not completely sure what's still broken vs left to be done since I didn't get that far last time given raw screenshots, which are an input for this, were still broken)
  • [ ] Update the screenshots.json to fix the device models and screenshot names being referenced
  • [ ] Update the device frames and background in playstoreres to match newer device models
  • [ ] Circle back with team Kāhu and Design team to update the compositing coordinates and config in screenshots.json with the new promo copies and their expected positioning/layout/rendering.

Other

  • [ ] Update SCREENSHOT_DEVICE_SETUP.md docs ➡️ Will be done in dedicated PR
  • [x] Circle back with Annmarie and the Kāhu team to decide if it's expected that, given how the UI tests are written, some raw screenshots are asked to be taken in light mode and other screens switch to dark mode before doing the screenshot. ➡️ Tracked separately in https://github.com/wordpress-mobile/WordPress-Android/issues/17133

Crash Log and issue details from items above (now solved)

ZenDesk Crash when trying to run the JPScreenshotTest on "phone" device (Pixel 3) in zh-CN
$ /Users/olivierhalligon/Library/Android/sdk/platform-tools/adb -s emulator-5554 shell am instrument --no-window-animation -w \
-e testLocale zh-CN \
--no-hidden-api-checks \
-e appendTimestamp false \
-e class org.wordpress.android.ui.screenshots.JPScreenshotTest \
com.jetpack.android.test/org.wordpress.android.WordPressTestRunner
[19:58:54]: ▸ org.wordpress.android.ui.screenshots.JPScreenshotTest:
[19:58:54]: ▸ Process crashed while executing jPScreenshotTest(org.wordpress.android.ui.screenshots.JPScreenshotTest):
[19:58:54]: ▸ java.lang.IllegalArgumentException: Zendesk needs to be setup before this method can be called
[19:58:54]: ▸ 	at org.wordpress.android.support.ZendeskHelper.enablePushNotifications(ZendeskHelper.kt:197)
[19:58:54]: ▸ 	at org.wordpress.android.push.GCMRegistrationIntentService.sendRegistrationToken(GCMRegistrationIntentService.java:86)
[19:58:54]: ▸ 	at org.wordpress.android.push.GCMRegistrationIntentService.lambda$onHandleWork$0$org-wordpress-android-push-GCMRegistrationIntentService(GCMRegistrationIntentService.java:53)
[19:58:54]: ▸ 	at org.wordpress.android.push.GCMRegistrationIntentService$$ExternalSyntheticLambda0.onComplete(Unknown Source:2)
[19:58:54]: ▸ 	at com.google.android.gms.tasks.zzi.run(com.google.android.gms:play-services-tasks@@18.0.1:1)
[19:58:54]: ▸ 	at android.os.Handler.handleCallback(Handler.java:873)
[19:58:54]: ▸ 	at android.os.Handler.dispatchMessage(Handler.java:99)
[19:58:54]: ▸ 	at android.os.Looper.loop(Looper.java:193)
[19:58:54]: ▸ 	at android.app.ActivityThread.main(ActivityThread.java:6669)
[19:58:54]: ▸ 	at java.lang.reflect.Method.invoke(Native Method)
[19:58:54]: ▸ 	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:493)
[19:58:54]: ▸ 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:858)
[19:59:09]: ▸ INSTRUMENTATION_RESULT: shortMsg=Process crashed.
🖼️ Running Screenshots in zh-CN interrupts the UI tests with an alert from the system asking if we want to enable Google Pinyin input method image image

To Test

ℹ️ See https://github.com/wordpress-mobile/release-toolkit/pull/409 for a more detailed set of instructions for testing this.

  • Run bundle exec fastlane screenshots app:wordpress, go take a very long walk while it runs, and check that all the expected raw screenshots are generated in fastlane/screenshots/raw for all locales and both device types
  • Run bundle exec fastlane screenshots app:jetpack, go take a very long walk while it runs, and check that all the expected raw screenshots are generated in fastlane/screenshots/raw for all locales and both device types

Optionally, during iterative testing, you can limit the runs to a subset of devices and locales, e.g. bundle exec fastlane screenshots app:wordpress device:phone locale:ar,fr-FR,zh-CN to avoid running it for all Mag16 and both devices for each app.

Though ultimately you will need to test all locales and both devices to be sure that:

  • The device codes are correct and thus the screenshots show the UI in the expected language for each locale (instead of using English, or using the locale from the previous run of the emulator…)
  • The device models used are as expected, and that those devices exist and can have their system-image be downloaded, their AVD be created, their emulator launched, and the permissions needed by fastlane/screengrab are set on those devices without any error (e.g. without them needing root access, or failing to get a permission for that emulator model and system image…)

AliSoftware avatar Aug 26 '22 18:08 AliSoftware

WordPress📲 You can test these changes on WordPress by downloading wordpress-installable-build-pr17096-8f36ef0.apk
💡 Scan this QR code with your Android phone to download and install the APK directly on it.
AppWordPress
Build FlavorJalapeno
Build TypeDebug
Commit8f36ef09c98f218dd9bcabfd85cc24988e7dcff7
Note: This installable build uses the JalapenoDebug build flavor, and does not support Google Login.

wpmobilebot avatar Aug 26 '22 19:08 wpmobilebot

Jetpack📲 You can test these changes on Jetpack by downloading jetpack-installable-build-pr17096-8f36ef0.apk
💡 Scan this QR code with your Android phone to download and install the APK directly on it.
AppJetpack
Build FlavorJalapeno
Build TypeDebug
Commit8f36ef09c98f218dd9bcabfd85cc24988e7dcff7
Note: This installable build uses the JalapenoDebug build flavor, and does not support Google Login.

wpmobilebot avatar Aug 26 '22 19:08 wpmobilebot

Update: Ready for Review

All critical/blocking tooling bugs around the screenshots lane (raw screenshots) have been solved now. So I think that PR is ready to review and land now 🎉

Warning: This PR still depends on https://github.com/wordpress-mobile/release-toolkit/pull/409 so we will need to land that one in release-toolkit, then update the fastlane/Pluginfile after we created a new toolkit release, before we can merge this one here.


Remaining, non-blocking tooling refinements for a future PR

Those will be done in separate PRs.

  • Make fastlane screenshots app:<app> generate the raw screenshots in different folders for WordPress vs Jetpack
  • See if we can avoid the red messages that appear in the logs when fastlane tries to delete old screenshots for the device's sdcard and can't find any. This will likely need to be fixed inside fastlane's code itself
  • Update SCREENSHOT_DEVICE_SETUP.md docs
  • Test, and fix if needed, the promo_screenshots lane now that we have working raw screenshots that we can use as input

Remaining non-tooling fixes (UI test failures in Android) — fixed separately

The are also things that will need fixing, but on the Android code of the UI tests themselves (ZenDesk crash, Hilt flaky crash, light vs dark mode…).

  • Those have been gathered under this parent GitHub issue
  • All those are not tooling-related, but Android-code-related (UI test not passing); so they will be fixed separately.
  • I made a call for help internally [ref: pcdRpT-MB-p2] and @AjeshRPai responded to the call; he's currently already working on some of those (for example https://github.com/wordpress-mobile/WordPress-Android/pull/17166)

AliSoftware avatar Sep 16 '22 23:09 AliSoftware

Warnings
:warning: This PR is assigned to a milestone which is closing in less than 4 days Please, make sure to get it merged by then or assign it to a later expiring milestone
:warning: PR has more than 300 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by :no_entry_sign: dangerJS

Hello @ParaskP7 anf thanks a lot for testing this PR!

Could you indicate which configuration you tested? M1 vs Intel and which way you installed the Android command line tools?


As for the crash you reported when testing with Jetpack, thanks for that, but this is a known one, already tracked here. 🙃

As it is not related to the tooling itself but is instead a flaky issue with Hilt only on the JPScreenshotTest.java test and Android code itself, this will be investigated separately (likely by @AjeshRPai) later 🙂

AliSoftware avatar Sep 20 '22 13:09 AliSoftware

👋 @AliSoftware !

Could you indicate which configuration you tested? M1 vs Intel and which way you installed the Android command line tools?

Sure, I tested with Mac on Intel chip and installed the Android command line via the AS (SDK Manager).

As for the crash you reported when testing with Jetpack, thanks for that, but this is https://github.com/wordpress-mobile/WordPress-Android/issues/17129. 🙃

Great, yes, I was thinking that's the case, but wanted to note that anyway since in the testing instructions you are mentioning that we can also test with the relevant Jetpack fastlane command (then go for a long walk, etc.). I just wasn't sure... thank's for confirming that for me! 🙇

As it is not related to the tooling itself but is instead a flaky issue with Hilt only on the JPScreenshotTest.java test and Android code itself, this will be investigated separately (likely by @AjeshRPai) later 🙂

👍

ParaskP7 avatar Sep 20 '22 13:09 ParaskP7

Recalling the apps-infra team for review now that the release-toolkit 5.6.0 has been shipped and this PR can point to it — which should now make it ready to merge once approved. 🙏 🙇

AliSoftware avatar Sep 27 '22 18:09 AliSoftware

Just resync'd the PR with trunk, to fix conflicts that appeared as trunk continued to evolve since the month this PR has been waiting for review.

@wordpress-mobile/apps-infrastructure Can someone add their review please, so we can finally merge it before it diverges again? This is blocking my progress on the rest of my current project Thread 😢

AliSoftware avatar Oct 28 '22 18:10 AliSoftware