Add Firebase Test Lab Support #355

jkmassel · 2022-04-18T05:42:49Z

Adds Firebase Test Lab support to allow for running instrumented/connected tests.

Can be tested alongside woocommerce/woocommerce-android#6422

The Firebase Helper had a lot of pieces doing a variety of things – splitting this into model objects makes it easier to change and test.

oguzkocer · 2022-05-03T16:05:09Z

@AliSoftware If you have the bandwidth, can I leave this review to you? It's very heavy on Ruby/fastlane, so I think your input would be much more valuable than mine. Having said that, I don't mind reviewing it at all and have the bandwidth for it, so just let me know if you prefer I review it.

AliSoftware · 2022-05-03T16:33:22Z

@AliSoftware If you have the bandwidth, can I leave this review to you? It's very heavy on Ruby/fastlane, so I think your input would be much more valuable than mine. Having said that, I don't mind reviewing it at all and have the bandwidth for it, so just let me know if you prefer I review it.

@oguzkocer I can review the code in GitHub to check for good Ruby and Fastlane implementation / patterns, but I very likely won't have time to test it (i.e. try running the action in one of our client app to validate it works in practice). Can I let you do that test while I review the code itself?

oguzkocer · 2022-05-03T16:42:13Z

@AliSoftware Sure thing - it looks like we have a test PR in WCAndroid, I'll look into that.

AliSoftware

Left a couple of 💄 nits and suggestions, as well as 🧠 improvement suggestions on the action doc, perfs and API… but also found a blocker with what I think is an 💥 incorrect implementation of one of the methods

👍 Notes and positive feedback
💄 Ruby Codestyle nits. Not a blocker for merging this PR
🧠 Optimization suggestions, Improvements on the action
💥 Actual blockers / issues that require fixing before we merge

The 💥 blocker is why I'm flagging this as "Request Changes".

PS: Once the 💥 is addressed, if you really need this to land ASAP to progress on adopting it in client projects, and don't have time to address the 💄 and 🧠 in a timely manner, we could consider landing the PR, make the client apps point to trunk, and then you could follow-up with a separate PR to address the 💄 + 🧠 before we do a formal 4.2 release.

AliSoftware · 2022-05-03T16:13:48Z

fastlane-plugin-wpmreleasetoolkit.gemspec

  spec.add_dependency 'chroma', '0.2.0'
  spec.add_dependency 'activesupport', '~> 5'
+
+  # `google-cloud-storage` is required by fastlane, but we pin it in case it's not in the future


lib/fastlane/plugin/wpmreleasetoolkit/actions/android/android_firebase_test.rb

lib/fastlane/plugin/wpmreleasetoolkit/models/firebase_test_runner.rb

spec/firebase_device_spec.rb

spec/firebase_test_lab_result_spec.rb

spec/firebase_test_runner_spec.rb

spec/test-data/firebase/failed-firebase-test-lab-run.log

Co-authored-by: Olivier Halligon <[email protected]>

lib/fastlane/plugin/wpmreleasetoolkit/models/firebase_device.rb

spec/firebase_test_runner_spec.rb

AliSoftware

Approving to get it land and move forward, but there is still the problem of only supporting only one {model,version,locale,orientation} tuple per call, while a couple of use cases we have (including screenshots) will need to support a matrix of up to… 64 combinations ({2 devices: phone+tablet} * {2 orientations} * {16 locales}) to be triggered in one run. And supporting multiple devices in one call of the action (which I'll need for [a4] of my Project Thread, btw) will require a breaking change in the API of this action 😒 so not sure we should ship a new version of the release-toolkit before making this API more flexible…?

AliSoftware · 2022-07-04T15:24:39Z

lib/fastlane/plugin/wpmreleasetoolkit/actions/android/android_firebase_test.rb

+          FastlaneCore::ConfigItem.new(
+            key: :model,
+            description: 'The device model to use to run the test',
+            type: String,
+            verify_block: proc do |value|
+              FirebaseTestRunner.verify_has_gcloud_binary!
+              model_names = Fastlane::FirebaseDevice.valid_model_names
+              UI.user_error!("Invalid Model Name: #{value}. Valid Model Names: #{model_names}") unless model_names.include?(value)
+            end
+          ),
+          FastlaneCore::ConfigItem.new(
+            key: :version,
+            description: 'The Android version (API Level) to use to run the test',
+            type: Integer,
+            verify_block: proc do |value|
+              FirebaseTestRunner.verify_has_gcloud_binary!
+              version_numbers = Fastlane::FirebaseDevice.valid_version_numbers
+              UI.user_error!("Invalid Version Number: #{value}. Valid Version Numbers: #{version_numbers}") unless version_numbers.include?(value)
+            end
+          ),
+          FastlaneCore::ConfigItem.new(
+            key: :locale,
+            description: 'The locale code to use when running the test',
+            type: String,
+            default_value: 'en',
+            verify_block: proc do |value|
+              FirebaseTestRunner.verify_has_gcloud_binary!
+              locale_codes = Fastlane::FirebaseDevice.valid_locales
+              UI.user_error!("Invalid Locale: #{value}. Valid Locales: #{locale_codes}") unless locale_codes.include?(value)
+            end
+          ),
+          FastlaneCore::ConfigItem.new(
+            key: :orientation,
+            description: 'Which orientation to run the device in',
+            type: String,
+            default_value: 'portrait',
+            verify_block: proc do |value|
+              orientations = Fastlane::FirebaseDevice.valid_orientations
+              UI.user_error!("Invalid Orientation: #{value}. Valid Orientations: #{orientations}") unless orientations.include?(value)
+            end
+          ),


I just realized something: this API means that we will only be able to pass a single {device,model,version,locale} tuple, as opposed to a list/matrix of devices. But in a couple of client apps we need CircleCI to run on multiple devices (Typically here to run the screenshots for WPAndroid).

Given how long this PR has been waiting I'm still inclined to accept this limitation for the first iteration… but this means that if/when we'll want to add support to trigger a FTL run for multiple devices at once, that will incur a breaking change in the API of this action 😒

Note: We might be able to make-do with the limitation in the meantime, by having separate Buildkite steps in our client pipeline for each {device,locale} we want to trigger… and then create a dependency between them all. But that won't be as optimal as a single trigger though, as that will mean uploading the 2 APK files to Firebase… as many times as we have devices we want to test on — so waste of CI time, bandwidth and FTL quota. Especially for the case of screenshots, when we want to run them on at least {2 devices}x{16 locales}, which will mean 64 APK uploads and 32 separate Buildkite Steps (!) and FTL runs (and seemingly unrelated when looking at the FTL UI, as opposed to being represented as a run matrix). Clearly not sustainable.

So I still think that we should fix this in a future iteration, at least to support the case of screenshots.

One quick possibility to fix this issue would be to have locale changed to locales and accept an array. That would reduce the number of API calls to gcloud from 64 to only 4 (each having 16 --device parameters, one for each locale) and reduce the number of APK uploads by 16 and have the tests related to the same device+orientation be under a common matrix under FTL… not as good as a single FTL matrix but still way better than the current situation.

AliSoftware · 2022-07-04T15:28:32Z

lib/fastlane/plugin/wpmreleasetoolkit/models/firebase_device.rb

+module Fastlane
+  class FirebaseDevice
+    attr_reader :model, :version, :locale, :orientation


Note that:

The models will be exposed to the public now thanks to https://github.com/wordpress-mobile/release-toolkit/pull/355/files#diff-ea7a49d236e63ebdbd0e50b21527ce2fa9887c7666ce0145eb9b6b6d1d393542R7

Which means it will be a breaking change to switch their API

Though it could be a good thing to make them public if we end up changing the API of this action to accept multiple devices (like the FTL API does, and as we'll need it for various cases in our client apps in practice) and want the action's API to accept an array of Firebase::Device… that again will need a breaking change in API in the next iteration tho

AliSoftware · 2022-07-04T16:34:26Z

lib/fastlane/plugin/wpmreleasetoolkit/actions/android/android_firebase_test.rb

+            verify_block: proc do |value|
+              FirebaseTestRunner.verify_has_gcloud_binary!
+              model_names = Fastlane::FirebaseDevice.valid_model_names
+              UI.user_error!("Invalid Model Name: #{value}. Valid Model Names: #{model_names}") unless model_names.include?(value)


The user_error doesn't seem to show (and thus list the available models) when run on CI and we pass an invalid model — see example here. It just says the parameter has been passed invalid value but the message above doesn't show on the logs, so that is not super helpful 😒

| [2022-07-04T16:30:17Z] [16:30:17]: ▸ BUILD SUCCESSFUL in 12m 56s | [2022-07-04T16:30:17Z] [16:30:17]: ▸ 94 actionable tasks: 86 executed, 8 from cache | [2022-07-04T16:30:17Z] [16:30:17]: ▸ Publishing build scan... | [2022-07-04T16:30:17Z] [16:30:17]: ▸ https://gradle.a8c.com/s/wz445ksdkrrek | [2022-07-04T16:30:19Z] [16:30:19]: Error setting value 'Nexus5' for option 'model' | [2022-07-04T16:30:19Z] [16:30:19]: You passed invalid parameters to 'android_firebase_test'. | [2022-07-04T16:30:19Z] [16:30:19]: Check out the error below and available options by running `fastlane action android_firebase_test` | [2022-07-04T16:30:19Z] +-------------------------------------+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ | [2022-07-04T16:30:19Z] \| Lane Context \| | [2022-07-04T16:30:19Z] +-------------------------------------+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ | [2022-07-04T16:30:19Z] \| DEFAULT_PLATFORM \| android \| | [2022-07-04T16:30:19Z] \| PLATFORM_NAME \| android \| | [2022-07-04T16:30:19Z] \| LANE_NAME \| android build_and_instrumented_test \| | [2022-07-04T16:30:19Z] \| GRADLE_ALL_APK_OUTPUT_PATHS \| ["/var/lib/buildkite-agent/builds/ci-android-i-04a64e2244229b61c-1/automattic/woocommerce-android/WooCommerce/build/outputs/apk/vanilla/debug/WooCommerce-vanilla-debug.apk", "/var/lib/buildkite-agent/builds/ci-android-i-04a64e2244229b61c-1/automattic/woocommerce-android/WooCommerce/build/outputs/apk/androidTest/vanilla/debug/WooCommerce-vanilla-debug-androidTest.apk"] \| | [2022-07-04T16:30:19Z] \| GRADLE_ALL_AAB_OUTPUT_PATHS \| [] \| | [2022-07-04T16:30:19Z] \| GRADLE_ALL_OUTPUT_JSON_OUTPUT_PATHS \| ["/var/lib/buildkite-agent/builds/ci-android-i-04a64e2244229b61c-1/automattic/woocommerce-android/WooCommerce/build/outputs/apk/vanilla/debug/output-metadata.json", "/var/lib/buildkite-agent/builds/ci-android-i-04a64e2244229b61c-1/automattic/woocommerce-android/WooCommerce/build/outputs/apk/androidTest/vanilla/debug/output-metadata.json"] \| | [2022-07-04T16:30:19Z] \| GRADLE_ALL_MAPPING_TXT_OUTPUT_PATHS \| [] \| | [2022-07-04T16:30:19Z] \| GRADLE_APK_OUTPUT_PATH \| /var/lib/buildkite-agent/builds/ci-android-i-04a64e2244229b61c-1/automattic/woocommerce-android/WooCommerce/build/outputs/apk/androidTest/vanilla/debug/WooCommerce-vanilla-debug-androidTest.apk \| | [2022-07-04T16:30:19Z] \| GRADLE_OUTPUT_JSON_OUTPUT_PATH \| /var/lib/buildkite-agent/builds/ci-android-i-04a64e2244229b61c-1/automattic/woocommerce-android/WooCommerce/build/outputs/apk/androidTest/vanilla/debug/output-metadata.json \| | [2022-07-04T16:30:19Z] +-------------------------------------+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ | [2022-07-04T16:30:19Z] [16:30:19]: Shell command exited with exit status 1 instead of 0. | [2022-07-04T16:30:20Z] | [2022-07-04T16:30:20Z] +------+------------------------------------------------------+-------------+ | [2022-07-04T16:30:20Z] \| fastlane summary \| | [2022-07-04T16:30:20Z] +------+------------------------------------------------------+-------------+ | [2022-07-04T16:30:20Z] \| Step \| Action \| Time (in s) \| | [2022-07-04T16:30:20Z] +------+------------------------------------------------------+-------------+ | [2022-07-04T16:30:20Z] \| 1 \| default_platform \| 0 \| | [2022-07-04T16:30:20Z] \| 2 \| is_ci \| 0 \| | [2022-07-04T16:30:20Z] \| 3 \| assembleVanillaDebug assembleVanillaDebugAndroidTest \| 777 \| | [2022-07-04T16:30:20Z] +------+------------------------------------------------------+-------------+ | [2022-07-04T16:30:20Z] | [2022-07-04T16:30:20Z] [16:30:20]: fastlane finished with errors | [2022-07-04T16:30:20Z] | [2022-07-04T16:30:20Z] [!] Shell command exited with exit status 1 instead of 0. | [2022-07-04T16:30:20Z] 🚨 Error: The command exited with status 1 | [2022-07-04T16:30:20Z] user command error: exit status 1

Actually removing my approval because while testing with woocommerce/woocommerce-android#6422 it failed and I couldn't make the action work. Investigating to check if it's an issue on the action or on the test PR itself.

AliSoftware

💥 When trying to run the command in the WCAndroid test branch manually from my Mac, I got the following crash:

[18:41:40]: wrong number of arguments (given 2, expected 1)
+------+------------------------------------------------------+-------------+
|                             fastlane summary                              |
+------+------------------------------------------------------+-------------+
| Step | Action                                               | Time (in s) |
+------+------------------------------------------------------+-------------+
| 1    | default_platform                                     | 0           |
| 2    | is_ci                                                | 0           |
| 3    | check_for_toolkit_updates                            | 1           |
| 4    | assembleVanillaDebug assembleVanillaDebugAndroidTest | 262         |
| 💥   | android_firebase_test                                | 0           |
+------+------------------------------------------------------+-------------+

[18:41:40]: fastlane finished with errors

Looking for related GitHub issues on fastlane/fastlane...

➡️  Couldn't upload testers from CSV because of ArgumentError
    https://github.com/fastlane/fastlane/issues/19671 [closed] 6 💬
    3 weeks ago

➡️  DELIVER (2.150.0.rc3/4) always ask attribute whatsNew
    https://github.com/fastlane/fastlane/issues/16700 [closed] 102 💬
    6 weeks ago

➡️  upload_to_play_store fails with error wrong number of arguments (given 1, expected 0)
    https://github.com/fastlane/fastlane/issues/18698 [closed] 9 💬
    18 Jul 2021

and 29 more at: https://github.com/fastlane/fastlane/search?q=wrong%20number%20of%20arguments%20%28given%202%2C%20expected%201%29&type=Issues&utf8=✓

🔗  You can ⌘ + double-click on links to open them directly in your browser.
bundler: failed to load command: fastlane (/Users/olivierhalligon/Documents/Dev/apps/woocommerce-android/vendor/bundle/ruby/2.7.0/bin/fastlane)
Traceback (most recent call last):
{…snip…}
	 2: from /Users/olivierhalligon/Documents/Dev/apps/woocommerce-android/vendor/bundle/ruby/2.7.0/gems/fastlane-2.205.2/fastlane/lib/fastlane/runner.rb:263:in `block (2 levels) in execute_action'
	 1: from /Users/olivierhalligon/Documents/Dev/apps/woocommerce-android/vendor/bundle/ruby/2.7.0/bundler/gems/release-toolkit-46e3825cc9e8/lib/fastlane/plugin/wpmreleasetoolkit/actions/android/android_firebase_test.rb:14:in `run'
/Users/olivierhalligon/Documents/Dev/apps/woocommerce-android/vendor/bundle/ruby/2.7.0/gems/fastlane-2.205.2/fastlane_core/lib/fastlane_core/configuration/configuration.rb:214:in `fetch': \e[31m[!] wrong number of arguments (given 2, expected 1)\e[0m (ArgumentError)

Seems that the run_uuid = params.fetch(:test_run_id, SecureRandom.uuid) is the one creating the crash — and indeed FastlaneCore::Configuration#fetch doesn't accept a second parameter. One more reason why you should have added a Unit Test for run_described_fastlane_action like I previously suggested above, which would have caught this early in tests.

AliSoftware · 2022-07-04T17:38:44Z

Ok I fixed the crashes above, but woocommerce/woocommerce-android#6422 is still not passing, claiming Error setting value 'Nexus5' for option 'model', You passed invalid parameters to 'android_firebase_test'. and then telling that Shell command exited with exit status 1 instead of 0..

It feels strange that the error doesn't say which models would be valid values. I'm starting to wonder if the action doesn't fail to find the gcloud library on CI, especially since the action manages to run when I run the lane from my local Mac, and since @jkmassel mentioned here that the fix would require the new AMI to work… but I have no idea if it was deployed yet or not. 🤷

Though even if e.g. gcloud wasn't installed on the CI machine, I'd expect the action to print the message "The gcloud binary isn't available on this machine" rather than exiting without any clear message…

lib/fastlane/plugin/wpmreleasetoolkit/actions/android/android_firebase_test.rb

spec/firebase_login_spec.rb

mokagio

I'm not sure which commit did it, but the instrumented tests that were failing are now pass.

I also followed the link in the logs to Firebase Test Lab to verify the build was there and green. 👌

mokagio · 2022-07-18T00:37:22Z

lib/fastlane/plugin/wpmreleasetoolkit/actions/android/android_firebase_test.rb

      def self.run(params)
        validate_options(params)

+        UI.user_error!('You must be logged in to Firebase prior to calling this action. Use the `FirebaseLogin` Action to log in if needed.') unless Fastlane::FirebaseAccount.authenticated?


Nitpick: Have you considered writing the action in the syntax a user would use to call it from the CLI, which is where this error would pop up?

This is a good idea – there's more area for improvement here for sure (for one, right now there's currently only one way to call this action, but gcloud supports a few different authentication mechanisms).

I'd like to land this PR to unblock some rather urgent fixes, but there's a bunch of other work we can do in this area for sure! (see the add/gradle-output-parsing branch for one such example)

Resolved by adding explicit input param validation

jkmassel force-pushed the add/firebase-tests branch from 572b8c0 to de20940 Compare May 2, 2022 20:35

jkmassel added 3 commits May 2, 2022 14:48

Add Android Firebase Action + Helper

bd7d602

Move Firebase Device out of the Helper

f866e66

Move log file parsing out of the FirebaseHelper

848c0ae

The Firebase Helper had a lot of pieces doing a variety of things – splitting this into model objects makes it easier to change and test.

jkmassel force-pushed the add/firebase-tests branch from de20940 to 848c0ae Compare May 2, 2022 20:48

Add CHANGELOG note

928613f

jkmassel force-pushed the add/firebase-tests branch 2 times, most recently from fb0ce52 to c878fe5 Compare May 2, 2022 22:47

Cleanup

c52bcff

jkmassel force-pushed the add/firebase-tests branch from c878fe5 to c52bcff Compare May 2, 2022 22:48

jkmassel marked this pull request as ready for review May 2, 2022 22:51

jkmassel requested review from AliSoftware, oguzkocer and wpmobilebot May 2, 2022 22:51

AliSoftware requested changes May 3, 2022

View reviewed changes

AliSoftware modified the milestones: Next Major Release, Next Minor Release May 6, 2022

jkmassel and others added 3 commits May 17, 2022 12:48

Apply suggestions from code review

e49b930

Co-authored-by: Olivier Halligon <[email protected]>

Fix tests

66f3e65

Don’t authenticate in constructor

372ad20