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 default flank settings to optimize performance #644

Merged
merged 2 commits into from
Feb 28, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 11 additions & 11 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ gcloud:
## (default: a timestamp with a random suffix).
# results-dir: tmp

## Enable video recording during the test. Enabled by default, use --no-record-video to disable.
## Enable video recording during the test. Disabled by default. Use --record-video to enable.
# record-video: true

## The max time this test execution can run before it is cancelled (default: 15m).
Expand Down Expand Up @@ -189,7 +189,7 @@ gcloud:
## (default: a timestamp with a random suffix).
# results-dir: tmp

## Enable video recording during the test. Enabled by default, use --no-record-video to disable.
## Enable video recording during the test. Disabled by default. Use --record-video to enable.
# record-video: true

## The max time this test execution can run before it is cancelled (default: 15m).
Expand Down Expand Up @@ -222,7 +222,7 @@ gcloud:
test: ../test_app/apks/app-debug-androidTest.apk

## Automatically log into the test device using a preconfigured Google account before beginning the test.
## Enabled by default, use --no-auto-google-login to disable.
## Disabled by default. Use --auto-google-login to enable.
# auto-google-login: true

## Whether each test runs in its own Instrumentation instance with the Android Test Orchestrator
Expand All @@ -244,10 +244,10 @@ gcloud:
# - /sdcard/

## Monitor and record performance metrics: CPU, memory, network usage, and FPS (game-loop only).
## Enabled by default, use --no-performance-metrics to disable.
## Disabled by default. Use --performance-metrics to enable.
# performance-metrics: true
## The fully-qualified Java class name of the instrumentation test runner

## The fully-qualified Java class name of the instrumentation test runner
## (default: the last name extracted from the APK manifest).
# test-runner-class: com.foo.TestRunner

Expand Down Expand Up @@ -315,7 +315,7 @@ flank:
## Keeps the full path of downloaded files. Required when file names are not unique.
## Default: false
# keep-file-path: false

## Include additional app/test apk pairs in the run. Apks are unique by just filename and not by path!
## If app is omitted, then the top level app is used for that pair.
# additional-app-test-apks:
Expand Down Expand Up @@ -346,7 +346,7 @@ android {
testCoverageEnabled true
}
}

// https://google.github.io/android-gradle-dsl/current/com.android.build.gradle.internal.dsl.TestOptions.html#com.android.build.gradle.internal.dsl.TestOptions:animationsDisabled
testOptions {
execution 'ANDROIDX_TEST_ORCHESTRATOR'
Expand Down Expand Up @@ -411,7 +411,7 @@ import static android.Manifest.permission.READ_EXTERNAL_STORAGE;
import static android.Manifest.permission.WRITE_EXTERNAL_STORAGE;

class MyEspressoTest {

@Rule
GrantPermissionRule grantPermissionRule = GrantPermissionRule.grant(
READ_EXTERNAL_STORAGE, WRITE_EXTERNAL_STORAGE);
Expand All @@ -427,11 +427,11 @@ Here's an example flank.yml. Note that `coverage` and `coverageFilePath` must be
gcloud:
app: ./app/build/outputs/apk/debug/app-debug.apk
test: ./app/build/outputs/apk/androidTest/debug/app-debug-androidTest.apk
environment-variables:
environment-variables:
coverage: true
coverageFilePath: /sdcard/
clearPackageData: true
directories-to-pull:
directories-to-pull:
- /sdcard/
# use a named results dir that's used by the gradle task
results-dir: coverage_ec
Expand Down
5 changes: 3 additions & 2 deletions test_runner/src/main/kotlin/ftl/args/yml/AndroidGcloudYml.kt
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package ftl.args.yml
import com.fasterxml.jackson.annotation.JsonIgnoreProperties
import com.fasterxml.jackson.annotation.JsonProperty
import ftl.config.Device
import ftl.config.FlankDefaults
import ftl.config.FtlConstants.defaultAndroidModel
import ftl.config.FtlConstants.defaultAndroidVersion

Expand All @@ -17,7 +18,7 @@ class AndroidGcloudYmlParams(
val test: String? = null,

@field:JsonProperty("auto-google-login")
val autoGoogleLogin: Boolean = true,
val autoGoogleLogin: Boolean = FlankDefaults.DISABLE_AUTO_LOGIN,

@field:JsonProperty("use-orchestrator")
val useOrchestrator: Boolean = true,
Expand All @@ -29,7 +30,7 @@ class AndroidGcloudYmlParams(
val directoriesToPull: List<String> = emptyList(),

@field:JsonProperty("performance-metrics")
val performanceMetrics: Boolean = true,
val performanceMetrics: Boolean = FlankDefaults.DISABLE_PERFORMANCE_METRICS,

@field:JsonProperty("test-runner-class")
val testRunnerClass: String? = null,
Expand Down
3 changes: 2 additions & 1 deletion test_runner/src/main/kotlin/ftl/args/yml/GcloudYml.kt
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package ftl.args.yml

import com.fasterxml.jackson.annotation.JsonIgnoreProperties
import com.fasterxml.jackson.annotation.JsonProperty
import ftl.config.FlankDefaults

/**
* Common Gcloud parameters shared between iOS and Android
Expand All @@ -18,7 +19,7 @@ class GcloudYmlParams(
var resultsDir: String? = null,

@field:JsonProperty("record-video")
val recordVideo: Boolean = true,
val recordVideo: Boolean = FlankDefaults.DISABLE_VIDEO_RECORDING,

val timeout: String = "15m",

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,13 +94,13 @@ class AndroidRunCommand : Runnable {
@Option(
names = ["--auto-google-login"],
description = ["Automatically log into the test device using a preconfigured " +
"Google account before beginning the test. Enabled by default, use --no-auto-google-login to disable."]
"Google account before beginning the test. Disabled by default."]
)
var autoGoogleLogin: Boolean? = null

@Option(
names = ["--no-auto-google-login"],
description = ["Google account not logged in. See --auto-google-login."]
description = ["Google account not logged in (default behavior). Use --auto-google-login to enable"]
)
var noAutoGoogleLogin: Boolean? = null

Expand Down Expand Up @@ -145,13 +145,13 @@ class AndroidRunCommand : Runnable {
@Option(
names = ["--performance-metrics"],
description = ["Monitor and record performance metrics: CPU, memory, " +
"network usage, and FPS (game-loop only). Enabled by default, use --no-performance-metrics to disable."]
"network usage, and FPS (game-loop only). Disabled by default."]
)
var performanceMetrics: Boolean? = null

@Option(
names = ["--no-performance-metrics"],
description = ["Disables performance metrics. See --performance-metrics"]
description = ["Disables performance metrics (default behavior). Use --performance-metrics to enable."]
)
var noPerformanceMetrics: Boolean? = null

Expand Down Expand Up @@ -221,13 +221,13 @@ class AndroidRunCommand : Runnable {
@Option(
names = ["--record-video"],
description = ["Enable video recording during the test. " +
"Enabled by default, use --no-record-video to disable."]
"Disabled by default."]
)
var recordVideo: Boolean? = null

@Option(
names = ["--no-record-video"],
description = ["Disable video recording during the test. See --record-video to enable."]
description = ["Disable video recording during the test (default behavior). Use --record-video to enable."]
)
var noRecordVideo: Boolean? = null

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,13 +144,13 @@ class IosRunCommand : Runnable {
@Option(
names = ["--record-video"],
description = ["Enable video recording during the test. " +
"Enabled by default, use --no-record-video to disable."]
"Disabled by default."]
)
var recordVideo: Boolean? = null

@Option(
names = ["--no-record-video"],
description = ["Disable video recording during the test. See --record-video to enable."]
description = ["Disable video recording during the test (default behavior). Use --record-video to enable."]
)
var noRecordVideo: Boolean? = null

Expand Down
12 changes: 12 additions & 0 deletions test_runner/src/main/kotlin/ftl/config/FlankDefaults.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package ftl.config

/**
* Flank specific default settings.
* Values should assure best (quickest) test performance.
* Each of value can be overwritten by config *.yml file
*/
object FlankDefaults {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if hiding default values behind constants was necessary. Any of those constant are used only once and they are not telling more than members which they are assigned. But we have one file more to check, if we want to know defaults.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general case I agree. In this particular one I wanted to point explicitly that those are not default, gcloud values, and what those values mean (ex when I see DISABLE_VIDEO_RECORDING I would conclude that Flank's default setting is to have this disabled not being particularly interested if this is false, -1 or sth else). Probably enum based would be better but this will require bigger refactor that's why I decided to proceed with this solution.

I could also just change true -> false and leave additional comment why it is implemented that way.

TBH both solutions are fine for me. Let me know what you think, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

That was only suggestion so you can leave it as is. Your arguments are reasonable, it's ok for me.

const val DISABLE_VIDEO_RECORDING = false
const val DISABLE_AUTO_LOGIN = false
const val DISABLE_PERFORMANCE_METRICS = false
}
3 changes: 0 additions & 3 deletions test_runner/src/test/kotlin/ftl/args/AndroidArgsFileTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -85,9 +85,6 @@ class AndroidArgsFileTest {
Device("shamu", "22", "zh_CN", "default")
)
)
}

with(args) {
assert(maxTestShards, 1)
assert(repeatTests, 1)
}
Expand Down
6 changes: 3 additions & 3 deletions test_runner/src/test/kotlin/ftl/args/AndroidArgsTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -271,19 +271,19 @@ AndroidArgs
with(androidArgs) {
// GcloudYml
assert(resultsBucket, "mockBucket")
assert(recordVideo, true)
assert(recordVideo, false)
assert(testTimeout, "15m")
assert(async, false)
assert(project, "mockProjectId")

// AndroidGcloudYml
assert(appApk, appApkAbsolutePath)
assert(testApk, testApkAbsolutePath)
assert(autoGoogleLogin, true)
assert(autoGoogleLogin, false)
assert(useOrchestrator, true)
assert(environmentVariables, emptyMap<String, String>())
assert(directoriesToPull, empty)
assert(performanceMetrics, true)
assert(performanceMetrics, false)
assert(testRunnerClass, null)
assert(testTargets, empty)
assert(devices, listOf(Device("NexusLowRes", "28")))
Expand Down
11 changes: 10 additions & 1 deletion test_runner/src/test/kotlin/ftl/args/IosArgsTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ import ftl.config.FtlConstants.defaultIosVersion
import ftl.test.util.FlankTestRunner
import ftl.test.util.TestHelper.absolutePath
import ftl.test.util.TestHelper.assert
import ftl.test.util.TestHelper.getPath
import org.junit.Assert.assertFalse
import org.junit.Assume
import org.junit.Rule
import org.junit.Test
Expand All @@ -26,6 +28,7 @@ import picocli.CommandLine
@RunWith(FlankTestRunner::class)
class IosArgsTest {
private val empty = emptyList<String>()
private val simpleFlankPath = getPath("src/test/kotlin/ftl/fixtures/simple-ios-flank.yml")
private val testPath = "./src/test/kotlin/ftl/fixtures/tmp/ios_earlgrey2.zip"
private val xctestrunFile =
"./src/test/kotlin/ftl/fixtures/tmp/EarlGreyExampleSwiftTests_iphoneos12.1-arm64e.xctestrun"
Expand Down Expand Up @@ -216,7 +219,7 @@ IosArgs
with(args) {
// GcloudYml
assert(resultsBucket, "mockBucket")
assert(recordVideo, true)
assert(recordVideo, false)
assert(testTimeout, "15m")
assert(async, false)
assert(project, "mockProjectId")
Expand Down Expand Up @@ -749,4 +752,10 @@ IosArgs

assertThat(actual).containsExactlyElementsIn(expected)
}

@Test
fun `verify flank default settings for ios`() {
val args = IosArgs.load(simpleFlankPath)
assertFalse(args.recordVideo)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ class CancelCommandTest {
fun cancelCommandRuns() {
exit.expectSystemExit()
val runCmd = AndroidRunCommand()
runCmd.configPath = "./src/test/kotlin/ftl/fixtures/android.yml"
runCmd.configPath = "./src/test/kotlin/ftl/fixtures/simple-android-flank.yml"
runCmd.run()
CancelCommand().run()
val output = systemOutRule.log
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ class AndroidRunCommandTest {
fun androidRunCommandRuns() {
exit.expectSystemExit()
val runCmd = AndroidRunCommand()
runCmd.configPath = "./src/test/kotlin/ftl/fixtures/android.yml"
runCmd.configPath = "./src/test/kotlin/ftl/fixtures/simple-android-flank.yml"
runCmd.run()
val output = systemOutRule.log
assertThat(output).contains("1 / 1 (100.00%)")
Expand Down
3 changes: 3 additions & 0 deletions test_runner/src/test/kotlin/ftl/fixtures/simple-ios-flank.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
gcloud:
test: ./src/test/kotlin/ftl/fixtures/tmp/ios_earlgrey2.zip
xctestrun-file: ./src/test/kotlin/ftl/fixtures/tmp/EarlGreyExampleSwiftTests_iphoneos12.1-arm64e.xctestrun