-
-
Notifications
You must be signed in to change notification settings - Fork 48
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
Update current flank flags #88
Conversation
Hey @runningcode , please take a look and let me know what do you think. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! thanks for the update. I just had some questions on the documentation but otherwise looks great.
You can also update the CHANGELOG.md
if you please and add a link to your contribution and github profile.
assertTrue(properties.contains(" network-profile: LTE")) | ||
} | ||
|
||
private fun emptyExtension(block: FlankGradleExtension.() -> Unit) = FlankGradleExtension(project).apply(block) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice extension function! :)
@@ -24,5 +24,16 @@ data class FladleConfigImpl( | |||
override var resultsBucket: String? = null, | |||
override var keepFilePath: Boolean = false, | |||
override var resultsDir: String?, | |||
override var additionalTestApks: Map<String, List<String>> = emptyMap() | |||
override var additionalTestApks: Map<String, List<String>> = emptyMap(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i just realized this is bad design on my part that we have to specify these twice :(. i'll see if i can clean this up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about small refactor but decided to keep it separate from each other. I'd like to contribute more into fladle and next PR would be probably refactor of some pieces of code (if I may ofc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that would be lovely. and yes, its a good idea to keep these PRs separate.
README.md
Outdated
@@ -184,6 +203,38 @@ Keeps the full path of downloaded files from a Google Cloud Storage bucket. Requ | |||
### resultsDir | |||
The name of a unique Google Cloud Storage object within the results bucket where raw test results will be stored. The default is a timestamp with a random suffix. | |||
|
|||
### runTimeout | |||
The max time this test run can execute before it is cancelled. s (seconds), m (minutes), h (hours) suffixes are acceptable (default: unlimited). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a little example here? is it like 1h2m2s
or 1h 2m 2s
? or just 1h
?
Disables sharding. Useful for parameterized tests. (default: false) | ||
|
||
### smartFlankDisableUpload | ||
Disables smart flank JUnit XML uploading. Useful for preventing timing data from being updated. (default: false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is "smart flank"? Can we link to docs in flank here?
A list of device-path: file-path pairs that indicate the device paths to push files to the device before starting tests, and the paths of files to push. Device paths must be under absolute, whitelisted paths (${EXTERNAL_STORAGE}, or ${ANDROID_DATA}/local/tmp). Source file paths may be in the local filesystem or in Google Cloud Storage (gs://…). | ||
|
||
### networkProfile | ||
The name of the network traffic profile, for example LTE, HSPA, etc, which consists of a set of parameters to emulate network conditions when running the test (default: no network shaping; see available profiles listed by the `flank test network-profiles list` command). This feature only works on physical devices. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, i like the examples and the command to fetch other supported values.
ced8aa0
to
163e587
Compare
Hey @runningcode thanks for review and comments. I implemented changes, please verify. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! I'm going to wait until an updated version of Flank is released with these features before merging, is that okay? Otherwise people might look at the readme and try to use features that don't work with any released version.
README.md
Outdated
@@ -184,6 +203,44 @@ Keeps the full path of downloaded files from a Google Cloud Storage bucket. Requ | |||
### resultsDir | |||
The name of a unique Google Cloud Storage object within the results bucket where raw test results will be stored. The default is a timestamp with a random suffix. | |||
|
|||
### runTimeout | |||
The max time this test run can execute before it is cancelled. s (seconds), m (minutes), h (hours) suffixes are acceptable, mixes like 1h45m are currently not supported (default: unlimited). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
||
### smartFlankDisableUpload | ||
Disables smart flank JUnit XML uploading. Useful for preventing timing data from being updated. (default: false) | ||
[What is Smart Flank?](https://github.com/Flank/flank/blob/master/docs/smart_flank.md) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Definitely, I will keep this PR updated if any new flag will appear in flank |
163e587
to
b71ecfc
Compare
b71ecfc
to
4dcf594
Compare
Hey! I was looking in to merging this, but when testing with Flank 8.1.0 i ran in to this issue which caused flank to hang:
Are we adding a flag that doesn't work with the old version of flank? |
@@ -25,5 +25,16 @@ data class FladleConfigImpl( | |||
override var resultsBucket: String?, | |||
override var keepFilePath: Boolean, | |||
override var resultsDir: String?, | |||
override var additionalTestApks: Map<String, List<String>> | |||
override var additionalTestApks: Map<String, List<String>>, | |||
override var runTimeout: String? = null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, i updated the rest of this class, but you can remove the redundant default specification from this file!
Yes, there are few flags there are not supported in flank 8.1.0. I'll update README with that information |
Co-Authored-By: Nelson Osacky <nelson@osacky.com>
Co-Authored-By: Nelson Osacky <nelson@osacky.com>
Co-Authored-By: Nelson Osacky <nelson@osacky.com>
Co-Authored-By: Nelson Osacky <nelson@osacky.com>
Co-Authored-By: Nelson Osacky <nelson@osacky.com>
21de81f
to
6ecdb1e
Compare
* Each key should be the Android resource name of a target UI element and each value should be the text input for that element. | ||
* Values are only permitted for text type elements, so no value should be specified for click and ignore type elements. | ||
*/ | ||
var roboDirectives: List<List<String>> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ohhh. do we want this to be a List<Triple>
?
Or is it better represented as Map<String, List<String>>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about that but I wanted to allow user to skip 3rd parameter.
roboDirectives = [
["click", "button1", ""],
["ignore", "button2"], <-- 3rd skipped
["text", "field1", "my text"],
]
Triple
will force user to always provide three strings. But I am open to suggestions
if (config.roboDirectives.isNotEmpty()) { | ||
appendln(" robo-directives:") | ||
config.roboDirectives.forEach { | ||
val value = it.getOrNull(2).let { stringValue -> if (stringValue.isNullOrBlank()) "\"\"" else stringValue } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it.getOrNull(2)
returns null, does it crash on .let
? should it be ?.let
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting fact it worked! But I think you are right. I'll change it to be more robust
Looking good. I'll test this tomorrow now that I figured out the issue that was causing the crash for me! Also, I won't block this PR since it already is quite large, but I just wanted to also give you a heads up: From now on, I would like new extensions in fladle to use Gradle's lazy property API. This will allow for lazy evaluation and faster configuration times. I started doing some migration here: f92bd12#diff-670e340c5b646cd261359b8466643022R40 Anyways, don't worry about it. I'll do the migration for you once I merge this PR. Let's figure out that robo directives api first! |
@pawelpasterz now that flank is released, let's double check all the flags are supported. I think |
Hey, I just tested this out. I'm going to merge to master and release a snapshot! Thanks! |
Hey @runningcode, have you had a chance to push these exciting changes to a snapshot release? I don't think |
There may be an older
More info here: https://docs.gradle.org/current/userguide/declaring_repositories.html#maven_repository_filtering |
Hey, I've deleted the |
To be honest I'm having trouble figuring out the correct coordinates for the sonatype snapshots repository. I think it's |
I believe the URL is this: |
New flags added