Skip to content
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

Add support for robo-directives & robo-script options #709

Merged
merged 11 commits into from
Apr 23, 2020

Conversation

jan-goral
Copy link
Contributor

@jan-goral jan-goral commented Apr 8, 2020

Fixes #706

Checklist

  • Documented
  • Unit tested
  • release_notes.md updated

@jan-goral jan-goral self-assigned this Apr 8, 2020
@jan-goral jan-goral added this to the May 2020 milestone Apr 8, 2020
@jan-goral jan-goral changed the title Add robo for robo-directives & robo-script options Add support for robo-directives & robo-script options Apr 9, 2020
@jan-goral jan-goral force-pushed the 706-robo-script-beta-option branch from 63265ed to 00f6817 Compare April 9, 2020 23:51
@codecov-io
Copy link

codecov-io commented Apr 9, 2020

Codecov Report

Merging #709 into master will decrease coverage by 1.26%.
The diff coverage is 66.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #709      +/-   ##
============================================
- Coverage     79.48%   78.22%   -1.27%     
- Complexity      666      676      +10     
============================================
  Files           113      125      +12     
  Lines          2696     2787      +91     
  Branches        385      402      +17     
============================================
+ Hits           2143     2180      +37     
- Misses          305      348      +43     
- Partials        248      259      +11     

@jan-goral jan-goral marked this pull request as ready for review April 10, 2020 00:06
@jan-goral jan-goral marked this pull request as draft April 10, 2020 07:09
@jan-goral
Copy link
Contributor Author

There is a little bit more work to do here. Android instrumentation tests and robo tests are mutually exclusive, they cannot be specified both. It requires additional changes how args are validated and test matrix build.

## The path may be in the local filesystem or in Google Cloud Storage using gs:// notation.
## You can guide the Robo test to perform specific actions by recording a Robo Script in Android Studio and then specifying this argument.
## Learn more at https://firebase.google.com/docs/test-lab/robo-ux-test#scripting.
# robo-script: path_to_robo_script
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice work! Remember to update readme.md as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated bunch of config options in README.md and flank.yml because both of them had lack of some updates.

@bootstraponline
Copy link
Contributor

For new features from gcloud, it's good to test the yaml syntax with gcloud CLI.

  • gcloud firebase test android run flank.yml:gcloud

I'd test and confirm that robo-directives and robo-script works well both from Flank and gcloud.

@jan-goral jan-goral force-pushed the 706-robo-script-beta-option branch from 00f6817 to 401b93b Compare April 14, 2020 03:38
@jan-goral jan-goral marked this pull request as ready for review April 21, 2020 11:21
@jan-goral
Copy link
Contributor Author

For new features from gcloud, it's good to test the yaml syntax with gcloud CLI.

* `gcloud firebase test android run flank.yml:gcloud`

I'd test and confirm that robo-directives and robo-script works well both from Flank and gcloud.

Thanks for advise, that was helpful to keep yml config consistent with gcloud. I tested robo-directives and robo-script with Flank and gcloud firebase test android run flank.yml:gcloud. Both seems to work well.

@@ -29,7 +29,7 @@ abstract class BaseInstrumentedTest {
val result: Boolean = when (BuildConfig.FLAVOR_type) {
"success" -> true
"error" -> false
"flaky" -> SystemClock.uptimeMillis() % 2 == 0L
"flaky" -> Random.nextBoolean()
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@jan-goral jan-goral requested a review from pawelpasterz April 22, 2020 10:17
@@ -104,9 +107,21 @@ class AndroidArgs(
"Option num-uniform-shards cannot be specified along with max-test-shards. Use only one of them"
)

if (!(isRoboTest xor isInstrumentationTest)) throw FlankFatalError(
"Option test xor robo-directives xor robo-script must be specified"
Copy link
Contributor

Choose a reason for hiding this comment

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

How about xor -> or?
Maybe we could use sth like One of bla bla bla. Just thinking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Flank is for programmers not end users, I guess every programmer knows what xor does mean. Anyway if you propose something better than One of bla bla bla ;), I can replace.

Copy link
Contributor

Choose a reason for hiding this comment

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

we have a community of QA engineers using Firebase Test Lab, they may not be familiar with what xor means 🙂

get() = if (input.isNotBlank()) input else "\"\""
}

fun List<String>.parseRoboDirectives() = map {
Copy link
Contributor

Choose a reason for hiding this comment

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

List<String>#parseRoboDirectives and Map<...>#parseRoboDirectives are only used inside AndroidArgs class. How about moving those there and make private?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put those mappers here because AndroidArgs has over 200 lines and do to much.

@@ -23,7 +25,7 @@ Example:
object MatrixResultsReport : IReport {
override val extension = ".txt"

private val percentFormat by lazy { DecimalFormat("#0.00") }
private val percentFormat by lazy { DecimalFormat("#0.00", DecimalFormatSymbols(Locale.US)) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change? Previous implementation takes default locale

public DecimalFormat(String pattern) {
        // Always applyPattern after the symbols are set
        this.symbols = DecimalFormatSymbols.getInstance(Locale.getDefault(Locale.Category.FORMAT));
        applyPattern(pattern, false);
}

I am just curious

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because defaults on some locales change . into , which breaks 3 unit tests. That fix is required because in commit 18b72b5 maven( url = "https://maven-central.storage-download.googleapis.com/repos/central/data/" ) was changed to jcenter() I guess.

Copy link
Contributor

Choose a reason for hiding this comment

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

I moved to jcenter since the old URL was causing build failures locally and is non-standard.

I think this code is related to printing the cost in US dollar so having that locale makes sense.

): AndroidTestConfig = when {
isInstrumentationTest -> createInstrumentationConfig(
uploadedApks = uploadedApks,
testShards = testShards!!
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO we should avoid !! operators. I think we should check and throw an error with detailed info. I know that now we are 100% sure (are we?) that null value will never happen here. But this could be useful during refactor or other changes.

)
isRoboTest -> createRoboConfig(
uploadedApks = uploadedApks,
runGcsPath = runGcsPath!!
Copy link
Contributor

Choose a reason for hiding this comment

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

As previous comment

@jan-goral jan-goral requested a review from pawelpasterz April 22, 2020 21:13
@jan-goral jan-goral merged commit b9596bc into master Apr 23, 2020
@jan-goral jan-goral deleted the 706-robo-script-beta-option branch April 23, 2020 12:45
@jan-goral jan-goral added the New Option Used to track PR with new configuration option (useful when updating fladle) label Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New Option Used to track PR with new configuration option (useful when updating fladle)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for robo-script FTL beta option
4 participants