Implement lazy properties by pawelpasterz · Pull Request #176 · runningcode/fladle

pawelpasterz · 2020-09-23T06:12:02Z

This will fix #92

What was done?

Implement all remaining properties to use lazy API.
small refactor

What was not done?

there is a test failing (locally) ConfigurationCacheTest#runFlank (it is also falling on the master branch)
update with latest flank's command
enable to run robo test without robo script
refactor of YamlWriterTest

I decided to left above as they are since the scope of this PR is rather big

pawelpasterz · 2020-09-23T06:12:27Z

Hey, @runningcode please take a look, thanks!

runningcode

Looking good. just some small logic changes I noticed. I haven't finished my review though.

runningcode · 2020-09-23T08:32:42Z

    }
    check(base.debugApk.isPresent) { "debugApk must be specified" }
-    check(base.instrumentationApk.isPresent xor !base.roboScript.isNullOrBlank()) {
+    check(base.instrumentationApk.isPresent xor base.roboScript.isPresent) {


the previous logic checked if the roboScript was not null and not blank. Should we check that here as well?

TBH I based my implementation on instrumentationApk check, but yeah, for now we should check for path being blank as well.

FYI, this will change with #165

runningcode · 2020-09-23T08:34:01Z

        appendln("  test: ${base.instrumentationApk.get()}")
      }
-      appendln(deviceString)
+      if (config.devices.isPresentAndNotEmpty) appendln(createDeviceString(config.devices.get()))


Is the device a required property in flank? What would happen if it were omitted?

I believe the previous logic would alway add it, this logic only add it if it is present. Why the change?

Actually not, we can omit devices and NexusLowRes will be taken as default. TBH the only required properties (from flank's perspective) are app apk and test apk. I have in my mind to change it in fladle but in separate PR

But I can change implementation to reflect the previous one, and change it with another PR

Makes sense. What version of the API/Android SDK does Flank use? I want to ensure that there are no unexpected changes introduced with this change.

Makes sense. What version of the API/Android SDK does Flank use? I want to ensure that there are no unexpected changes introduced with this change.

28

https://github.com/Flank/flank/blob/master/test_runner/src/main/kotlin/ftl/config/FtlConstants.kt#L47-L48

runningcode

Two more questions.
There are a lot of large customers of fladle, and I just want to make sure not to introduce any unexpected changes.

runningcode · 2020-09-24T15:26:53Z

        appendln("  test: ${base.instrumentationApk.get()}")
      }
-      appendln(deviceString)
+      if (config.devices.isPresentAndNotEmpty) appendln(createDeviceString(config.devices.get()))


Makes sense. What version of the API/Android SDK does Flank use? I want to ensure that there are no unexpected changes introduced with this change.

runningcode

Alright, we're getting pretty close. Sorry its a big PR and its taking me a while to get through. :)
Here's another round of comments.

runningcode · 2020-09-25T10:14:03Z

              extension.debugApk.set([email protected])
            }
-            if (extension.roboScript == null && !extension.instrumentationApk.isPresent) {
+            if (!extension.roboScript.isPresent && !extension.instrumentationApk.isPresent) {


in order to maintain the same logic as before, i think this should be:

Suggested change

if (!extension.roboScript.isPresent && !extension.instrumentationApk.isPresent) {

if (extension.roboScript.orNull == null && !extension.instrumentationApk.isPresent) {

runningcode · 2020-09-25T10:14:39Z

-  override var devices: List<Map<String, String>> = listOf(mapOf("model" to "NexusLowRes", "version" to "28"))
+  override val useOrchestrator: Property<Boolean> = objects.property<Boolean>().convention(false)
+  override val autoGoogleLogin: Property<Boolean> = objects.property<Boolean>().convention(false)
+  override val devices: ListProperty<Map<String, String>> = objects.listProperty<Map<String, String>>().convention(listOf(mapOf("model" to "NexusLowRes", "version" to "28")))


thanks for keeping this here. perhaps we can remove it in a separate PR.

runningcode · 2020-09-25T10:17:56Z

-      resultsBucket = resultsBucket,
-      keepFilePath = keepFilePath,
-      resultsDir = resultsDir,
+      useOrchestrator = objects.property<Boolean>().convention(useOrchestrator),


thanks for changing all of these! 👍

runningcode · 2020-09-25T10:18:27Z

    }
    check(base.debugApk.isPresent) { "debugApk must be specified" }
-    check(base.instrumentationApk.isPresent xor !base.roboScript.isNullOrBlank()) {
+    check(base.instrumentationApk.isPresent xor !base.roboScript.orNull.isNullOrBlank()) {


runningcode · 2020-09-25T10:19:01Z

        appendln("  test: ${base.instrumentationApk.get()}")
      }
-      appendln(deviceString)
+      if (config.devices.isPresentAndNotEmpty) appendln(createDeviceString(config.devices.get()))


i think this isPresentAndNotEmpty can be removed now.

runningcode · 2020-09-25T10:23:44Z

-    appendln("  output-style: ${config.outputStyle.get()}")
+    appendProperty(config.testShards, name = "max-test-shards")
+    appendProperty(config.shardTime, name = "shard-time")
+    appendProperty(config.repeatTests, name = "num-test-runs")


nice cleanup here

runningcode · 2020-09-25T10:40:06Z

+  private fun <T> StringBuilder.appendListProperty(
+    prop: ListProperty<T>,


In this case, I believe the list property must always be of type string. Can we restrict the API in that way in order to prevent it from being abused or having unintended effects?

Suggested change

private fun <T> StringBuilder.appendListProperty(

prop: ListProperty<T>,

private fun StringBuilder.appendListProperty(

prop: ListProperty<String>,

runningcode · 2020-09-25T10:41:11Z


-    config.roboScript?.let {
-      appendln("  robo-script: $it")
+  private fun <T, K> StringBuilder.appendMapProperty(


In this case, I believe the map property must always be of type string. Can we restrict the API in that way in order to prevent it from being abused or having unintended effects?

Suggested change

private fun <T, K> StringBuilder.appendMapProperty(

private fun StringBuilder.appendMapProperty(

runningcode · 2020-09-26T19:41:54Z

Thanks @pawelpasterz for doing this! i know it was a lot of work!

pawelpasterz added 4 commits September 22, 2020 21:35

Initial commit

267b370

Implement nex properties

222127e

Implement rest propoerties

e163baa

Refactor

221a800

runningcode reviewed Sep 23, 2020

View reviewed changes

Comment thread build.gradle Outdated

runningcode reviewed Sep 23, 2020

View reviewed changes

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

pawelpasterz added 2 commits September 23, 2020 08:56

Update build.gradle and documentation

d5cf08b

Remove unnecessary annotation

9c50454

pawelpasterz mentioned this pull request Sep 23, 2020

Implement lazy properties Flank/flank#1143

Closed

runningcode requested changes Sep 23, 2020

View reviewed changes

Refactor

98313a6

runningcode requested changes Sep 24, 2020

View reviewed changes

runningcode reviewed Sep 25, 2020

View reviewed changes

runningcode approved these changes Sep 26, 2020

View reviewed changes

runningcode merged commit 041c8c3 into runningcode:master Sep 26, 2020

	if (!extension.roboScript.isPresent && !extension.instrumentationApk.isPresent) {
	if (extension.roboScript.orNull == null && !extension.instrumentationApk.isPresent) {

		private fun <T> StringBuilder.appendListProperty(
		prop: ListProperty<T>,

	private fun <T, K> StringBuilder.appendMapProperty(
	private fun StringBuilder.appendMapProperty(

Uh oh!

Conversation

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

Uh oh!

Uh oh!

pawelpasterz commented Sep 23, 2020

Uh oh!

Uh oh!

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!

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 Sep 23, 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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

runningcode commented Sep 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pawelpasterz commented Sep 23, 2020 •

edited

Loading

pawelpasterz Sep 23, 2020 •

edited

Loading