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

Support roboTest without roboScript (sanityRobo) #177

13 changes: 13 additions & 0 deletions buildSrc/src/main/java/com/osacky/flank/gradle/FladleConfig.kt
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,19 @@ interface FladleConfig {
@get:Optional
val serviceAccountCredentials: RegularFileProperty

/**
* debugApk and instrumentationApk are [Property<String>] and not [RegularFileProperty] because we support wildcard characters.
*/
@get:Input
@get:Optional
val debugApk: Property<String>
@get:Input
@get:Optional
val instrumentationApk: Property<String>

@get:Input
val sanityRobo: Property<Boolean>

@get:Input
val useOrchestrator: Property<Boolean>

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ data class FladleConfigImpl(
internal val name: String,
override val projectId: Property<String>,
override val serviceAccountCredentials: RegularFileProperty,
override val debugApk: Property<String>,
override val instrumentationApk: Property<String>,
override val sanityRobo: Property<Boolean>,
override val useOrchestrator: Property<Boolean>,
override val autoGoogleLogin: Property<Boolean>,
override val devices: ListProperty<Map<String, String>>,
Expand Down Expand Up @@ -45,4 +48,19 @@ data class FladleConfigImpl(
override val outputStyle: Property<String>,
override val legacyJunitResult: Property<Boolean>,
override val fullJunitResult: Property<Boolean>
) : FladleConfig
) : FladleConfig {
/**
* Prepare config to run sanity robo.
*
* Sets [sanityRobo] property as `true`.
*
* Cleans [instrumentationApk], [additionalTestApks], [roboDirectives], [roboScript] properties.
*/
fun clearPropertiesForSanityRobo() {
sanityRobo.set(true)
additionalTestApks.empty()
instrumentationApk.set("")
roboDirectives.empty()
roboScript.set("")
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@ class FladlePluginDelegate {
// Must be done afterEvaluate otherwise extension values will not be set.
project.dependencies.add(FLADLE_CONFIG, "${base.flankCoordinates.get()}:${base.flankVersion.get()}")

checkIfSanityAndValidateConfigs(base)
Copy link
Owner

Choose a reason for hiding this comment

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

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

base.configs.forEach(::checkIfSanityAndValidateConfigs)

// Only use automatic apk path detection for 'com.android.application' projects.
project.pluginManager.withPlugin("com.android.application") {
if (!base.debugApk.isPresent || !base.instrumentationApk.isPresent) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ open class FlankGradleExtension @Inject constructor(objects: ObjectFactory) : Fl
@get:Input
val flankCoordinates: Property<String> = objects.property(String::class.java).convention("com.github.flank:flank")

override val sanityRobo: Property<Boolean> = objects.property<Boolean>().convention(false)

@get:Input
val flankVersion: Property<String> = objects.property(String::class.java).convention("20.09.3")
// Project id is automatically discovered by default. Use this to override the project id.
Expand Down Expand Up @@ -50,12 +52,9 @@ open class FlankGradleExtension @Inject constructor(objects: ObjectFactory) : Fl
/**
* debugApk and instrumentationApk are [Property<String>] and not [RegularFileProperty] because we support wildcard characters.
*/
@get:Input
@get:Optional
val debugApk: Property<String> = objects.property()
@get:Input
@get:Optional
val instrumentationApk: Property<String> = objects.property()
override val debugApk: Property<String> = objects.property()

override val instrumentationApk: Property<String> = objects.property()

override val directoriesToPull: ListProperty<String> = objects.listProperty()

pawelpasterz marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -115,6 +114,9 @@ open class FlankGradleExtension @Inject constructor(objects: ObjectFactory) : Fl
name = it,
projectId = objects.property<String>().convention(projectId),
serviceAccountCredentials = objects.fileProperty().convention(serviceAccountCredentials),
debugApk = objects.property<String>().convention(debugApk),
instrumentationApk = objects.property<String>().convention(instrumentationApk),
sanityRobo = objects.property<Boolean>().convention(false),
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@pawelpasterz pawelpasterz Sep 29, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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.

useOrchestrator = objects.property<Boolean>().convention(useOrchestrator),
autoGoogleLogin = objects.property<Boolean>().convention(autoGoogleLogin),
devices = objects.listProperty<Map<String, String>>().convention(devices),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package com.osacky.flank.gradle

import org.gradle.api.GradleException
import org.gradle.api.provider.ListProperty
import org.gradle.api.provider.Property

@Throws(GradleException::class)
fun checkIfSanityAndValidateConfigs(config: FladleConfig) = when (config) {
is FlankGradleExtension -> config.checkAndValidateConfig() { option, _ ->
"Incorrect [base] configuration. [$option] can't be used together with sanityRobo."
}
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"
Copy link
Owner

Choose a reason for hiding this comment

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

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.

}
else -> throw GradleException("Unable to check for sanity, check config type")
Copy link
Owner

Choose a reason for hiding this comment

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

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.

}

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

Choose a reason for hiding this comment

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

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 {

roboDirectives.isNotPresentOrEmpty -> throw GradleException(message("roboDirectives", name))
Copy link
Owner

Choose a reason for hiding this comment

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

I think an IllegalArgumentException might be better here.

roboScript.isNotPresentOrBlank -> throw GradleException(message("roboScript", name))
instrumentationApk.isNotPresentOrBlank -> throw GradleException(message("instrumentationApk", name))
additionalTestApks.isNotPresentOrEmpty -> throw GradleException(message("additionalTestApks", name))
}
}

private val Property<String>.isNotPresentOrBlank
get() = orNull.isNullOrBlank().not()

private val <T> ListProperty<T>.isNotPresentOrEmpty
get() = getOrElse(emptyList()).isEmpty().not()
43 changes: 25 additions & 18 deletions buildSrc/src/main/java/com/osacky/flank/gradle/YamlWriter.kt
Original file line number Diff line number Diff line change
Expand Up @@ -12,27 +12,31 @@ internal class YamlWriter {
check(base.serviceAccountCredentials.isPresent) { "ServiceAccountCredentials in fladle extension not set. https://github.com/runningcode/fladle#serviceaccountcredentials" }
}
check(base.debugApk.isPresent) { "debugApk must be specified" }
check(base.instrumentationApk.isPresent xor !base.roboScript.orNull.isNullOrBlank()) {
val prefix = if (base.instrumentationApk.isPresent && !base.roboScript.orNull.isNullOrBlank()) {
"Both instrumentationApk file and roboScript file were specified, but only one is expected."
} else {
"Must specify either a instrumentationApk file or a roboScript file."
}
"""
if (config.sanityRobo.get() == false) {
check(config.instrumentationApk.isPresent xor !config.roboScript.orNull.isNullOrBlank()) {
val prefix = if (base.instrumentationApk.isPresent && !config.roboScript.orNull.isNullOrBlank()) {
"Both instrumentationApk file and roboScript file were specified, but only one is expected."
} else {
"Must specify either a instrumentationApk file or a roboScript file."
}
"""
$prefix
instrumentationApk=${base.instrumentationApk.orNull}
roboScript=${base.roboScript.orNull}
""".trimIndent()
instrumentationApk=${config.instrumentationApk.orNull}
roboScript=${config.roboScript.orNull}
""".trimIndent()
}
}

val shouldPrintTestAndRobo = config.sanityRobo.get().not()
val additionalProperties = writeAdditionalProperties(config)
val flankProperties = writeFlankProperties(config)

return buildString {
appendln("gcloud:")
appendln(" app: ${base.debugApk.get()}")
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
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@pawelpasterz pawelpasterz Oct 1, 2020

Choose a reason for hiding this comment

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

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

if (shouldPrintTestAndRobo && config.instrumentationApk.isPresent) {
appendln(" test: ${config.instrumentationApk.get()}")
}
if (config.devices.isPresentAndNotEmpty) appendln(createDeviceString(config.devices.get()))
appendln(additionalProperties)
Expand All @@ -50,7 +54,8 @@ internal class YamlWriter {
appendProperty(config.projectId, name = "project")
appendProperty(config.keepFilePath, name = "keep-file-path")
appendListProperty(config.filesToDownload, name = "files-to-download") { appendln(" - $it") }
appendListProperty(config.additionalTestApks, name = "additional-app-test-apks") { appendln(" $it") }
if (!config.sanityRobo.get())
appendListProperty(config.additionalTestApks, name = "additional-app-test-apks") { appendln(" $it") }
appendProperty(config.runTimeout, name = "run-timeout")
appendProperty(config.ignoreFailedTests, name = "ignore-failed-tests")
appendProperty(config.disableSharding, name = "disable-sharding")
Expand Down Expand Up @@ -82,10 +87,12 @@ internal class YamlWriter {
appendMapProperty(config.clientDetails, name = "client-details") { appendln(" ${it.key}: ${it.value}") }
appendMapProperty(config.otherFiles, name = "other-files") { appendln(" ${it.key}: ${it.value}") }
appendProperty(config.networkProfile, name = "network-profile")
appendProperty(config.roboScript, name = "robo-script")
appendListProperty(config.roboDirectives, name = "robo-directives") {
val value = it.getOrElse(2) { "" }.let { stringValue -> if (stringValue.isBlank()) "\"\"" else stringValue }
appendln(" ${it[0]}:${it[1]}: $value")
if (!config.sanityRobo.get()) {
appendProperty(config.roboScript, name = "robo-script")
appendListProperty(config.roboDirectives, name = "robo-directives") {
val value = it.getOrElse(2) { "" }.let { stringValue -> if (stringValue.isBlank()) "\"\"" else stringValue }
appendln(" ${it[0]}:${it[1]}: $value")
}
}
}

Expand Down
Loading