Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
bd7d602
Add Android Firebase Action + Helper
jkmassel Apr 5, 2022
f866e66
Move Firebase Device out of the Helper
jkmassel Apr 6, 2022
848c0ae
Move log file parsing out of the FirebaseHelper
jkmassel May 2, 2022
928613f
Add CHANGELOG note
jkmassel May 2, 2022
c52bcff
Cleanup
jkmassel May 2, 2022
e49b930
Apply suggestions from code review
jkmassel May 17, 2022
66f3e65
Fix tests
jkmassel May 17, 2022
372ad20
Don’t authenticate in constructor
jkmassel May 17, 2022
0deed47
Verify gcloud binary on FirebaseTestRunner.new
jkmassel May 17, 2022
77e180f
Make CI happy
jkmassel May 17, 2022
5ac5225
Remove the explicit verify_has_gloud_binary call
jkmassel May 17, 2022
c659f02
Simplify more_details_url
jkmassel May 17, 2022
23870c0
Simplify raw_results_paths
jkmassel May 17, 2022
7ac5002
Adjust `require`
jkmassel May 17, 2022
9dc580e
Add docs
jkmassel May 17, 2022
ff89bd0
Fix broken success? method
jkmassel May 17, 2022
d7b73c5
Fix `passed?`
jkmassel May 24, 2022
c6a8f2c
Fix Google Cloud Storage gem pinning
jkmassel May 24, 2022
8e16c7b
Apply suggestions from code review
jkmassel May 24, 2022
14eed21
Fix tests
jkmassel May 24, 2022
bd7395e
Simplify parameters
jkmassel May 24, 2022
3267537
Indentation Fix
jkmassel May 24, 2022
b99126a
Fix an oops
jkmassel May 24, 2022
1c1a8bc
Re-add default_value_dynamic
jkmassel May 24, 2022
8ab2031
Fix unclear errors when gcloud binary missing
jkmassel May 24, 2022
9ba5d34
Escape all the things
jkmassel May 24, 2022
5d15b3a
Improve Firebase Test Runner Errors
jkmassel May 24, 2022
85b0df0
Remove constants
jkmassel May 24, 2022
5b46778
Fix lint issues
jkmassel May 25, 2022
fdb177c
Merge remote-tracking branch 'origin/trunk' into add/firebase-tests
jkmassel Jun 1, 2022
839be18
Merge remote-tracking branch 'origin/trunk' into add/firebase-tests
jkmassel Jul 1, 2022
f1e3128
Fix rubocop error
jkmassel Jul 1, 2022
d9536de
Document the `env_name`
jkmassel Jul 1, 2022
08bc2c0
Remove key file environment variable setting
jkmassel Jul 1, 2022
7826e23
Improve argument documentation
jkmassel Jul 1, 2022
1b789ce
Avoid double escaping
jkmassel Jul 1, 2022
46e3825
Apply suggestions from code review
jkmassel Jul 1, 2022
913578a
Fix crashes
AliSoftware Jul 4, 2022
20ae178
Check params early to avoid late prompts
AliSoftware Jul 11, 2022
febebef
Add Action Validation Tests
jkmassel Jul 15, 2022
75a8d41
Fix params
jkmassel Jul 15, 2022
8e7c274
Add Firebase Login Action
jkmassel Jul 16, 2022
fb136d0
Remove login logic from Firebase_Test Action
jkmassel Jul 16, 2022
6db3296
Put back key file parameter
jkmassel Jul 16, 2022
a660c60
Make FirebaseTestRunner static
jkmassel Jul 16, 2022
495b038
Manually specify the project_id field
jkmassel Jul 16, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ Metrics/ClassLength:
Max: 300

Metrics/MethodLength:
Max: 100
Max: 150

Metrics/ModuleLength:
Max: 300
Expand Down
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ _None_
If provided, that identifier will be added as an `a8c-src-lib` XML attribute to the `<string>` nodes being updated with strings from said library.
This can be useful to help identify where each string come from in the resulting, merged `strings.xml`. [#351]
* Add the option for `an_localize_libs` to set the `tools:ignore="UnusedResources"` XML attribute for each string being merged from a library. [#354]
* Add the ability to run Firebase Test Lab tests [#355]

### Bug Fixes

Expand Down
3 changes: 2 additions & 1 deletion Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ PATH
chroma (= 0.2.0)
diffy (~> 3.3)
git (~> 1.3)
google-cloud-storage (~> 1)
jsonlint (~> 0.3)
nokogiri (~> 1.11)
octokit (~> 4.18)
Expand Down Expand Up @@ -427,4 +428,4 @@ DEPENDENCIES
yard

BUNDLED WITH
2.2.33
2.3.8
4 changes: 4 additions & 0 deletions fastlane-plugin-wpmreleasetoolkit.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ Gem::Specification.new do |spec|
spec.add_dependency 'parallel', '~> 1.14'
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

spec.add_dependency 'google-cloud-storage', '~> 1'

# Some of the upstream code uses `BigDecimal.new` which version 2.0 of the
# `bigdecimal` gem removed. Until we'll find the time to identify the
# dependencies and see if we can move them to something compatible with
Expand Down
2 changes: 1 addition & 1 deletion lib/fastlane/plugin/wpmreleasetoolkit.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ module Fastlane
module Wpmreleasetoolkit
# Return all .rb files inside the "actions" and "helper" directory
def self.all_classes
Dir[File.expand_path('**/{actions,helper}/**/*.rb', File.dirname(__FILE__))]
Dir[File.expand_path('**/{actions,helper,models}/**/*.rb', File.dirname(__FILE__))]
end
end
end
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,186 @@
require 'securerandom'

module Fastlane
module Actions
module SharedValues
FIREBASE_TEST_RESULT = :FIREBASE_TEST_LOG_FILE
FIREBASE_TEST_LOG_FILE_PATH = :FIREBASE_TEST_LOG_FILE_PATH
end

class AndroidFirebaseTestAction < Action
def self.run(params)
# Preflight – ensure the system is set up correctly
Fastlane::FirebaseTestRunner.verify_has_gcloud_binary

# Log in to Firebase (and validate credentials)
test_runner = Fastlane::FirebaseTestRunner.new(key_file: params[:key_file])

# Set up the log file and output directory
FileUtils.mkdir_p(params[:results_output_dir])
Fastlane::Actions.lane_context[:FIREBASE_TEST_LOG_FILE_PATH] = File.join(params[:results_output_dir], 'output.log')

device = Fastlane::FirebaseDevice.new(
model: params[:model],
version: params[:version],
locale: params[:locale],
orientation: params[:orientation]
)

result = test_runner.run_tests(
apk_path: params[:apk_path],
test_apk_path: params[:test_apk_path],
device: device,
type: params[:type]
)

# Download all of the outputs from the job to the local machine
test_runner.download_result_files(
result: result,
destination: params[:results_output_dir],
project_id: params[:project_id],
key_file_path: params[:key_file]
)

FastlaneCore::UI.test_failure! "Firebase Tests failed – more information can be found at #{result.more_details_url}" unless result.success?

UI.success 'Firebase Tests Complete'
end

#####################################################
# @!group Documentation
#####################################################

def self.description
'Runs the specified tests in Firebase Test Lab'
end

def self.details
description
end

def self.available_options
run_uuid = SecureRandom.uuid

[
FastlaneCore::ConfigItem.new(
key: :project_id,
env_name: 'GCP_PROJECT',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming there is a rationale behind choosing that specific env_name rather than following the more common naming pattern for config items like ANDROID_FIREBASE_TEST_PROJECT_ID… like, this is a typical environment variable that is usually set in client projects for other reasons / tools to pick up?

If that's the rationale (this env var name being some standard used by other stuff as well), then you should probably add a # comment to mention it, so that if there's a reason then we'd not tempted to normalize that env_name later and break things.

If it's not (i.e. it's just that the client app you want to test this action with first happens to have that env var set, but for legacy reasons with some old tooling which won't be relevant anymore, and thus no hard reason to keep that name), you might as well change this to try to follow our usual naming conventions of the env var (even if that means that you'll explicitly pass project_id: ENV['GCP_PROJECT'] at call site for this client app, or rename the env var in the client repo)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in d9536de

description: 'The Project ID to test in',
type: String
),
FastlaneCore::ConfigItem.new(
key: :key_file,
env_name: 'GOOGLE_APPLICATION_CREDENTIALS',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same remark here. In particular, I find GOOGLE_APPLICATION_CREDENTIALS way too generic. If you see such env var defined on a project without more context, how would you guess that it's supposed to be a token, or path to a file, or a password… while FL_ANDROID_FIREBASE_TEST_KEY_FILE would be more in line with fastlane conventions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense – I'm not sure this one needs an env var since this is always provided by some sort of secrets implementation anyway.

Removed in 08bc2c0.

description: 'The key file used to authorize with Google Cloud',
type: String,
verify_block: proc do |value|
next if File.file? value

UI.user_error!("Invalid key file path: #{value}")
end
),
FastlaneCore::ConfigItem.new(
key: :apk_path,
description: 'The application APK',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit vague, especially when you try to understand the difference between apk_path and test_apk_path… could we provide more info here (even if it's just a link referring to the FTL documentation, or gcloud's documentation) to make more sense at what we're supposed to pass here vs in test_apk_path?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 7826e23 – I added a bit more documentation, there's really nothing to link to from Google's side, they don't explain it either – a user of this action would need to know that Android testing uses two APKs for UI testing – one for the application code and one for the test code.

This is pretty basic UI Testing knowledge, so I'm not sure it's worth documenting to that level, but hopefully the replacement is at least more clear about the argument expectations?

Copy link
Contributor

@AliSoftware AliSoftware Jul 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes agreed, no need to go into documenting to that level.

In fact I learned a bit more about how Android instrumentation testing works recently (i.e. way after I made that comment above) and indeed learned that in Android UI testing requires 2 separate APKs(†)… and now that I know and played a bit more with instrumentation tests, this is self-explanatory and just a fact of how Android instrumentation tests work indeed; so the updated docs you made in 7826e23 are enough 👍

(†) And now that I think about it it kinda makes sense and is similar to iOS after all (where the App bundle and the Test bundle are separate build products), just less visible in iOS so I never really thought about it that way before and why I was confused by the 2 APKs in Android at first back then 🙃 . Present me is less confused now. 😂

type: String,
verify_block: proc do |value|
next if File.file? value

UI.user_error!("Invalid application APK: #{value}")
end
),
FastlaneCore::ConfigItem.new(
key: :test_apk_path,
description: 'The test APK',
type: String,
verify_block: proc do |value|
next if File.file? value

UI.user_error!("Invalid test APK: #{value}")
end
),
FastlaneCore::ConfigItem.new(
key: :model,
description: 'The device model to run',
type: String,
verify_block: proc do |value|
model_names = Fastlane::FirebaseDevice.valid_model_names
next if model_names.include? value

UI.user_error!("Invalid Model Name: #{value}. Valid Model Names: #{model_names}")
end
),
FastlaneCore::ConfigItem.new(
key: :version,
description: 'The device version to run',
type: Integer,
verify_block: proc do |value|
version_numbers = Fastlane::FirebaseDevice.valid_version_numbers
next if version_numbers.include? value

UI.user_error!("Invalid Version Number: #{value}. Valid Verison Numbers: #{version_numbers}")
end
),
FastlaneCore::ConfigItem.new(
key: :locale,
description: 'The locale code to run in',
type: String,
default_value: 'en',
verify_block: proc do |value|
locale_codes = Fastlane::FirebaseDevice.valid_locales
next if locale_codes.include? value

UI.user_error!("Invalid Locale: #{value}. Valid Locales: #{locale_codes}")
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
next if orientations.include? value

UI.user_error!("Invalid Orientation: #{value}. Valid Orientations: #{orientations}")
end
),
FastlaneCore::ConfigItem.new(
key: :type,
description: 'Which type of test are we running?',
type: String,
default_value: 'instrumentation',
verify_block: proc do |value|
types = Fastlane::FirebaseTestRunner::VALID_TEST_TYPES
next if types.include? value

UI.user_error!("Invalid Test Type: #{value}. Valid Types: #{types}")
end
),
FastlaneCore::ConfigItem.new(
key: :test_run_id,
description: 'A unique ID used to identify this test run',
type: String,
default_value: run_uuid,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tbh this is not super idiomatic to have a dynamic value set as default_value here, because that means that each time you run fastlane action android_firebase_test to get the documentation about the action, you'll get a different UUID shown in the documentation for the default value.

I'd suggest instead to remove default_value: run_uuid here, as well as the one in results_output_dir, and as well as the run_uuid = at the top of this self.available_options, and instead move that logic inside self.run

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in bd7395e and tested here to ensure that everything works fine with no adjustment to the calling conventions.

default_value_dynamic: true
),
FastlaneCore::ConfigItem.new(
key: :results_output_dir,
description: 'Where should we store the results of this test run?',
type: String,
default_value: File.join(Dir.tmpdir(), run_uuid),
default_value_dynamic: true
),
]
end

def self.authors
['Automattic']
end

def self.is_supported?(platform)
platform == :android
end
end
end
end
53 changes: 53 additions & 0 deletions lib/fastlane/plugin/wpmreleasetoolkit/models/firebase_device.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
module Fastlane
class FirebaseDevice
attr_reader :model, :version, :locale, :orientation
Comment on lines +1 to +3
Copy link
Contributor

@AliSoftware AliSoftware May 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I really like that this was represented as a model class. We don't have many model classes in our release-toolkit right now so it's rare to see a good reason for having them, but this one was a good fit for it indeed 👌

I'm not a fan of this being defined directly at the root of the Fastlane namespace (i.e. what if in the future fastlane itself adds such a FirebaseDevice class?) Even though I know all our other existing classes in models/ already follow this pattern, that doesn't mean it was a good choice back then 😅 But maybe that's a refactor for another time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I worried about this for a moment as well – IMHO if that happens at some point we can address it then? I didn't want to bury it too deeply since it's a pain to reference a deeply-nested model object?

Copy link
Contributor

@AliSoftware AliSoftware May 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I think we could also keep the idea in the back of our head for later to introduce a module WPMRT namespace for models at some point.

I worried about it for this old WIP as well, which might be even more at risk of clashing with something provided by fastlane given its more generic name… so maybe at that point create that WPMRT namespace then. Given yours is not introduced as public API (iinm), changing the internal namespace and thus fully-qualified name of that class later would not be a breaking change (unlike the LocaleHelper PR which is planned to be exposed publicly) so that's ok to keep it as is for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changing the internal namespace and thus fully-qualified name of that class later would not be a breaking change

Yeah exactly – this was what had me thinking it'd make sense to leave it for the moment 🙂

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing we could consider at least doing though: put all the Firebase-related models under a models/firebase subdirectory, and use Fastlane::Firebase::Device instead of Fastlane::FirebaseDevice (and similar for the 2 others). (I think Rubymine can do such a refactor automatically accross the codebase…?)

Since those are models that all go together hand in hand that would make it clear and neat, and would make us ready for if we add more, unrelated model files for other unrelated things in the future, avoinding to put everything in the same bowl.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that:


@@locale_data = nil
@@model_data = nil
@@version_data = nil

def initialize(model:, version:, orientation:, locale: 'en')
raise 'Invalid Model' unless FirebaseDevice.valid_model_names.include? model
raise 'Invalid Version' unless FirebaseDevice.valid_version_numbers.include? version
raise 'Invalid Locale' unless FirebaseDevice.valid_locales.include? locale
raise 'Invalid Orientation' unless FirebaseDevice.valid_orientations.include? orientation

@model = model
@version = version
@locale = locale
@orientation = orientation
end

def to_s
"model=#{@model},version=#{@version},locale=#{@locale},orientation=#{@orientation}"
end

def self.valid_model_names
JSON.parse(model_data).map { |device| device['codename'] }
end

def self.valid_version_numbers
JSON.parse(version_data).map { |version| version['apiLevel'].to_i }
end

def self.valid_locales
JSON.parse(locale_data).map { |locale| locale['id'] }
end

def self.valid_orientations
%w[portrait landscape]
end

def self.locale_data
@@locale_data || Fastlane::Actions.sh('gcloud firebase test android locales list --format="json"', log: false)
end

def self.model_data
@@model_data || Fastlane::Actions.sh('gcloud firebase test android models list --format="json"', log: false)
end

def self.version_data
@@version_data || Fastlane::Actions.sh('gcloud firebase test android versions list --format="json"', log: false)
end
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
module Fastlane
class FirebaseTestLabResult
def initialize(log_file_path:)
raise "No log file found at path #{log_file_path}" unless File.file? log_file_path

@path = log_file_path
end

# Scan the log file to for indications that the Test Run failed
def success?
File.readlines(@path).none? { |line| !line.include? 'Failed' }
Copy link
Contributor

@AliSoftware AliSoftware May 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💥 Double negation in "None of the lines don't include" feels a bit hard to parse and reason about… and in fact seems wrong to me and the opposite of what we want?

We should consider that we have a success if none of the line include "Failed", right?

Suggested change
File.readlines(@path).none? { |line| !line.include? 'Failed' }
File.readlines(@path).none? { |line| line.include? 'Failed' }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @oguzkocer FYI as you're testing this PR with a real-world usage in a client app, and this code/issue might lead to you seeing results opposite to what you'd expect

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent catch there – this is addressed in ff89bd0.

end

# Parse the log for the "More details are available..." URL
def more_details_url
File.readlines(@path)
.map { |line| URI.extract(line) }
.flatten
.compact
.filter { |string| string.include? 'matrices' }
.first
end

# Parse the log for the Google Cloud Storage Bucket URL
def raw_results_paths
uri = File.readlines(@path)
.map { |line| URI.extract(line) }
.flatten
.compact
.map { |string| URI(string) }
.filter { |u| u.scheme == 'gs' }
.first

return nil if uri.nil?

return {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also surprised here that rubocop doesn't complain about that return being useless (Ruby has implicit return)…

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely weird, agreed!

bucket: uri.host,
prefix: uri.path.delete_prefix('/').chomp('/')
}
end
end
end
Loading