Support roboTest without roboScript (sanityRobo) by pawelpasterz · Pull Request #177 · runningcode/fladle

pawelpasterz · 2020-09-25T07:51:55Z

Fixes #165

Flank supports roboTest without roboScript (sanityRobo) Flank/flank#1108

Sanity robo should be ONLY launched when there is NO, robo-script, test, additional-app-test-apks. This reflects gcloud's behavior

TO BE UPDATED

To run sanity robo test add -PsanityRobo to gradle command

./gradlew runFlank -PsanityRobo

With this property, fladle should skip appending config yaml file with robo-directives, robo-script, test, additional-app-test-apks
Example:

Run ./gradlew -p sample-kotlin writeConfigProps -PsanityRobo from project root
generated config yaml should look (no test)

gcloud:
  app: [path]
  device:
  - model: Pixel2
    version: 26
  - model: Nexus5
    version: 23

  use-orchestrator: true
  auto-google-login: false
  record-video: true
  performance-metrics: true
  timeout: 15m
  environment-variables:
    clearPackageData: true
  test-targets:
  - class com.osacky.flank.gradle.sample.ExampleInstrumentedTest#seeView
  num-flaky-test-attempts: 1

flank:
  smart-flank-gcs-path: gs://test-lab-yr9w6qsdvy45q-iurp80dm95h8a/flank/test_app_android.xml
  keep-file-path: false
  ignore-failed-tests: false
  disable-sharding: false
  smart-flank-disable-upload: false
  legacy-junit-result: false
  full-junit-result: false
  output-style: single

pawelpasterz · 2020-09-25T08:11:11Z

Hey @runningcode please, take a look and let me know what do you think

Thanks!

runningcode

Hey @pawelpasterz. Thanks very much for doing this. A lot of people have been asking for this feature.
I also really appreciate the thorough tests.

pawelpasterz · 2020-09-28T12:44:13Z

As discussed with @runningcode, changing approach a bit

sanityRobo won't be able to set via gradle property
Sanity robo run will ONLY be launched when ALL conditions are met:
a) no instrumentationApk, roboScript, roboDirectives, additionalTestApks are set
b) sanityRobo is explicitly set true (this will turn off autodetection feature for apks/scripts
```
fladle {
    sanityRobo = true
}
```
If sanityRobo = true and at least one of instrumentationApk, roboScript, roboDirectives, additionalTestApks is set as well, an error will be thrown

runningcode

i haven't finished looking but just some comments! looks pretty good. i will make some suggestions on the error messages in my next pass.

runningcode · 2020-09-29T13:38:32Z

      serviceAccountCredentials = objects.fileProperty().convention(serviceAccountCredentials),
+      debugApk = objects.property<String>().convention(debugApk),
+      instrumentationApk = objects.property<String>().convention(instrumentationApk),
+      sanityRobo = objects.property<Boolean>().convention(false),


shouldn't we be passing the value from the base here?

I think we shouldn't. Assuming we have sanityRobo = true in the base config that would propagate this to the additional configs. My idea was, every config has it's own sanityRobo value (default false) that is independent of the rest. I am thinking about the case when user want runFlank to be sanity robo test and all the rest (runFlank[name1], runFlank[name2]) are 'normal' test runs

Of course, I can change it to behave otherwise.

You make a good point here. I think we want consistency though. All other values are inherited. We shouldn't assume that users will not try to have multiple different sanityRobo configs which inherit from the base.
Let's pass the value from the base here as well.

runningcode

Very nice. I went through all of it. I had a few questions about the test cases and error messages but otherwise the overall approach looks like what we want!

runningcode · 2020-09-30T12:11:43Z

 tasks.withType(Test::class.java).configureEach {
  // Test fixtures are stored in here so we should re-run tests if the test projects change.
  inputs.dir("src/test/resources")
+  maxParallelForks = Runtime.getRuntime().availableProcessors()


can we make this change separately? How does it affect the performance of the tests?

can we make this change separately?

sure

How does it affect the performance of the tests?

On my machine, ./gradlew test took 1m 41s (without parallelism) and 1m 10s (with parallelism)
So it's not something crucial (IMO) but a nice-to-have feature.

runningcode · 2020-09-30T12:15:29Z

+
+private fun FladleConfigImpl.checkAndValidateConfig() {
+  if (sanityRobo.getOrElse(false)) when {
+    roboDirectives.isNotPresentOrEmpty -> throwAdditionalConfigError("roboDirectives", name)


why is the ordering different for the property validation?
can we make both methods the same and just replace an empty base config name "" with base?

Although base could be the name of the configuration. I don't have a better idea though.

Sure, nice catch. This is old implementation before I've added some new properties to FladleConfig, thanks!

runningcode · 2020-09-30T12:16:44Z

-      if (base.instrumentationApk.isPresent) {
-        appendln("  test: ${base.instrumentationApk.get()}")
+      appendln("  app: ${config.debugApk.get()}")
+      // We don't want to print instrumentation apks if sanityRobo == true


shouldn't we throw an error here if sanityRobo is true?

Sanity run validation if the user sets instrumentationApk explicitly and this occurs before findDebugAndInstrumentationApk (autodetection ) is invoked (in FlankPluginDelegate). The autodetection feature sets app apk and test apk only for base config and then those values are used for inner (or would should I name those additional ?) configs.

So actually this is applicable only for case when base config is intended to be sanity robo run and the rest 'normal'

Logic could be simplified if we would agree that base config can't be sanity robo

runningcode · 2020-09-30T12:20:56Z

  override val fullJunitResult: Property<Boolean>
-) : FladleConfig
+) : FladleConfig {
+  fun sanityRoboRun() {


I like this method. Its a good compromise!
Can we rename it to clearPropertiesForSanityRobo()? And add a bit of javadoc explaining which fields are being cleared?

runningcode · 2020-09-30T12:21:59Z

  }

-  internal fun writeAdditionalProperties(config: FladleConfig): String = buildString {
+  internal fun writeAdditionalProperties(config: FladleConfig, printRobo: Boolean = true): String = buildString {


instead of having a boolean passed in to this function, maybe we can just separate it in to two functions? I think that would make the code more readable.
i have an intrinsic dislike of boolean parameters to functions 😆

Actually, I can remove this logic entirely since now we have sanityRobo in FladleConfig

runningcode · 2020-09-30T12:29:59Z

+  }
+
+  @Test
+  fun `test auto configuration with sanityRobo set (base config)`() {


very nice test. thank you!

pawelpasterz · 2020-10-01T06:34:32Z

Hey @runningcode , ready for another review round :)

runningcode

Alright, looking very good. Another round of comments and hopefully we can merge this very soon.

runningcode · 2020-10-01T19:14:02Z

      // Must be done afterEvaluate otherwise extension values will not be set.
      project.dependencies.add(FLADLE_CONFIG, "${base.flankCoordinates.get()}:${base.flankVersion.get()}")

+      checkIfSanityAndValidateConfigs(base)


let's move this to be the first line in createTasksForConfig
that way the next line can be removed as well.

runningcode · 2020-10-01T19:17:13Z

      serviceAccountCredentials = objects.fileProperty().convention(serviceAccountCredentials),
+      debugApk = objects.property<String>().convention(debugApk),
+      instrumentationApk = objects.property<String>().convention(instrumentationApk),
+      sanityRobo = objects.property<Boolean>().convention(false),


You make a good point here. I think we want consistency though. All other values are inherited. We shouldn't assume that users will not try to have multiple different sanityRobo configs which inherit from the base.
Let's pass the value from the base here as well.

runningcode · 2020-10-01T19:20:05Z

+  }
+  is FladleConfigImpl -> config.checkAndValidateConfig(config.name) { option, name ->
+    "Incorrect [$name] configuration. [$option] can't be used together with sanityRobo. " +
+      "If you want to launch robo test run without robo script place only clearPropertiesForSanityRobo() into [$name] configuration"


Suggested change

"If you want to launch robo test run without robo script place only clearPropertiesForSanityRobo() into [$name] configuration"

"To configure sanityRobo, add clearPropertiesForSanityRobo() into [$name] configuration"

I'm changing the wording here a bit because a robo test run could also be with the robo directives. In that case, these instructions could be confusing. Let's refer specifically to sanityRobo here.

runningcode · 2020-10-01T19:22:21Z

+    "Incorrect [$name] configuration. [$option] can't be used together with sanityRobo. " +
+      "If you want to launch robo test run without robo script place only clearPropertiesForSanityRobo() into [$name] configuration"
+  }
+  else -> throw GradleException("Unable to check for sanity, check config type")


Let's throw an IllegalStateException and add the config's class name (or just toString() to the message.
This will help with debugging down the line.

runningcode · 2020-10-01T19:23:03Z

+
+private fun FladleConfig.checkAndValidateConfig(name: String = "base", message: (String, String) -> String) {
+  if (sanityRobo.getOrElse(false)) when {
+    roboDirectives.isNotPresentOrEmpty -> throw GradleException(message("roboDirectives", name))


I think an IllegalArgumentException might be better here.

runningcode · 2020-10-01T19:24:19Z

+}
+
+private fun FladleConfig.checkAndValidateConfig(name: String = "base", message: (String, String) -> String) {
+  if (sanityRobo.getOrElse(false)) when {


We should just get the default value since it is already set to false. Adding a getOrElse(false) here is like adding a second default value.

Suggested change

if (sanityRobo.getOrElse(false)) when {

if (sanityRobo.get()) when {

runningcode · 2020-10-01T19:58:03Z

I'm going to merge and fix the rest of the comments on master so we can release this!

pawelpasterz force-pushed the enable-robo-test-without-roboscript branch from 17f39ee to 4978a4b Compare September 25, 2020 08:03

pawelpasterz mentioned this pull request Sep 25, 2020

Enable to run robo test without roboScript on fladle Flank/flank#1144

Closed

runningcode requested changes Sep 25, 2020

View reviewed changes

pawelpasterz force-pushed the enable-robo-test-without-roboscript branch from 31ea174 to 779ca80 Compare September 25, 2020 10:01

runningcode reviewed Sep 25, 2020

View reviewed changes

Comment thread buildSrc/src/test/java/com/osacky/flank/gradle/integration/SanityRoboCheck.kt Outdated

Comment thread buildSrc/src/main/java/com/osacky/flank/gradle/YamlWriter.kt Outdated

pawelpasterz force-pushed the enable-robo-test-without-roboscript branch from 0671480 to 4cfe399 Compare September 28, 2020 12:33

pawelpasterz marked this pull request as draft September 29, 2020 10:44

pawelpasterz force-pushed the enable-robo-test-without-roboscript branch 2 times, most recently from ac2ba00 to a3ef5d3 Compare September 29, 2020 13:09

runningcode reviewed Sep 29, 2020

View reviewed changes

pawelpasterz force-pushed the enable-robo-test-without-roboscript branch from 8dee5d4 to f726143 Compare September 30, 2020 10:42

runningcode reviewed Sep 30, 2020

View reviewed changes

pawelpasterz added 11 commits October 1, 2020 08:23

Implement sanityRobo

cbb00ae

Update doc

abed478

PR review changes

3095ac1

Refactor sanity check test

79b0e11

Remove gradle properties

e4dd6ee

After rebase - resolve problems

750ae1b

Add validation

f08f456

Refactor

1a6c8b6

Add test for autoconfig sanity run

e4b1e1f

Update test

cd22c9b

Another round of refactor

65213bd

pawelpasterz force-pushed the enable-robo-test-without-roboscript branch from f602526 to 65213bd Compare October 1, 2020 06:33

Linting

82cd138

runningcode reviewed Oct 1, 2020

View reviewed changes

runningcode marked this pull request as ready for review October 1, 2020 19:30

runningcode approved these changes Oct 1, 2020

View reviewed changes

runningcode merged commit 4fa1199 into runningcode:master Oct 1, 2020

	"If you want to launch robo test run without robo script place only clearPropertiesForSanityRobo() into [$name] configuration"
	"To configure sanityRobo, add clearPropertiesForSanityRobo() into [$name] configuration"

	if (sanityRobo.getOrElse(false)) when {
	if (sanityRobo.get()) when {

Uh oh!

Conversation

pawelpasterz commented Sep 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TO BE UPDATED

Uh oh!

pawelpasterz commented Sep 25, 2020

Uh oh!

runningcode left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pawelpasterz commented Sep 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

runningcode left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pawelpasterz Sep 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

runningcode left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pawelpasterz Oct 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pawelpasterz Oct 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pawelpasterz commented Oct 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

runningcode left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pawelpasterz commented Sep 25, 2020 •

edited

Loading

pawelpasterz commented Sep 28, 2020 •

edited

Loading

pawelpasterz Sep 29, 2020 •

edited

Loading

pawelpasterz Oct 1, 2020 •

edited

Loading

pawelpasterz Oct 1, 2020 •

edited

Loading

pawelpasterz commented Oct 1, 2020 •

edited

Loading