Dump shards by bootstraponline · Pull Request #794 · Flank/flank

bootstraponline · 2020-05-16T17:26:12Z

Fixes #791

Checklist

Documented
Unit tested
release_notes.md updated

pawelpasterz

I like the solution. One thing to mention, I think it would be nice to cover that case with unit tests.

pawelpasterz · 2020-05-16T20:52:36Z

+
+        val filteredTests = testApks.map { getTestMethods(args, it) }.flatten()
+
+        if (filteredTests.isEmpty()) println("${FtlConstants.indent}No tests for ${testApks.joinToString(", ")}")


How about returning with empty list here?

bootstraponline · 2020-05-16T23:22:10Z

There's a lot of duplication with getTestShardChunks as well, I should probably refactor this.

I will have time Monday to address the review comments and add a unit test. Thanks for the review. 🙂

runningcode · 2020-05-17T07:58:51Z

Looks great! Thanks.

jan-goral · 2020-05-18T15:40:15Z

            listOf(filteredTests.map(FlankTestMethod::testName))
    }

+    fun getAllLocalTestShardChunks(args: AndroidArgs): ShardChunks {


Is Local meaning that this method doesn't accept test apks by gcs paths, right?

codecov-commenter · 2020-05-19T05:47:58Z

Codecov Report

Merging #794 into master will decrease coverage by 0.18%.
The diff coverage is 75.00%.

@@             Coverage Diff              @@
##             master     #794      +/-   ##
============================================
- Coverage     78.90%   78.71%   -0.19%     
  Complexity      696      696              
============================================
  Files           137      143       +6     
  Lines          2891     2950      +59     
  Branches        417      423       +6     
============================================
+ Hits           2281     2322      +41     
- Misses          342      354      +12     
- Partials        268      274       +6

pawelpasterz

There are two things that are concerns to me:

wrong result (or correct me if I'm wrong)
we've lost some clarity of code. I like refactor and reusability of code but there are some places that are unclear for me (at first glance)

pawelpasterz · 2020-05-19T07:50:49Z

+    }

-        val filteredTests = getTestMethods(args, testLocalApk)
+    fun getTestShardChunks(args: AndroidArgs, testApks: List<String> = listOf()): ShardChunks {


The actual need to pass list here is only when user use --dump-shards option. I am not convinced with this solution for normal (test run) flow, then we need to create list with one element (for every test pair):

AndroidTestShards L41 args.resolveApks().forEach { apks: ResolvedApks -> val testShards = apks.test?.let { test -> AndroidTestShard.getTestShardChunks(args, listOf(test)) }

I think wrapping one element into list is a bit code smell, what do you think?

Maybe just overload getTestShardChunks with

fun getTestShardChunks(args: AndroidArgs, testApk: String) = getTestShardChunks(args, listOf(testApk))

to reduce changes in other files :)

I think wrapping one element into list is a bit code smell, what do you think?

I agree

pawelpasterz · 2020-05-19T08:39:30Z

-        val filteredTests = getTestMethods(args, testLocalApk)
+    fun getTestShardChunks(args: AndroidArgs, testApks: List<String> = listOf()): ShardChunks {
+        val resolvedApks = getTestApks(args, testApks)
+        val filteredTests = resolvedApks.map { getTestMethods(args, it) }.flatten()


I think this is producing wrong results. Here we have list of all tests from all test pairs. With normal flow we have new matrix for each test pair and inside that matrix flank divides tests into shards. With this solution we have all tests in one list (from all test pairs) and then flank make shards, so we basically neglect how tests are divided among matrices. I may be wrong, but this is not result we would like to have.

Thanks. I think you're right.

I think there are two different use cases:

Serialize the shards to android_shards.json for debugging.

Given a single test apk, generate shards.

Maybe these should be different methods after all.

bootstraponline · 2020-05-20T04:08:11Z

I think for this PR, we probably want an output format in android_shards.json like this:

{
  "matrix-1": {
    "app": "foo.apk",
    "test": "bar.apk",
    "shards": {
        "shard-1": ["class com.example.app.ExampleUiTest#testPasses"]
    }
  }
}

Then it will be very clear to the user how many matrices will be created, and what shards will be used.

The goal is to provide as much information as possible to debug problems related to the sharding algorithm.

pawelpasterz · 2020-05-20T04:27:36Z

@bootstraponline
Looks good to me!

jan-goral · 2020-05-22T19:17:48Z

@bootstraponline

{
  "matrix-0": {
    "app": "/Users/janek/projects/flank-project/flank/test_runner/src/test/kotlin/ftl/fixtures/tmp/apk/app-debug.apk",
    "test": "/var/folders/p5/ll89kbn91t37gz9ry9kwxbh40000gp/T/tmp806579008668219334.tmp",
    "shards": {
      "shard-0": [
        "class com.example.test_app.InstrumentedTest#test"
      ]
    }
  },
  "matrix-1": {
    "app": "/Users/janek/projects/flank-project/flank/test_runner/src/test/kotlin/ftl/fixtures/tmp/apk/app-debug.apk",
    "test": "/var/folders/p5/ll89kbn91t37gz9ry9kwxbh40000gp/T/tmp5261068495618932935.tmp",
    "shards": {
      "shard-0": [
        "class com.example.test_app.InstrumentedTest#test1",
        "class com.example.test_app.InstrumentedTest#test2"
      ],
      "shard-1": [
        "class com.example.test_app.InstrumentedTest#test0",
        "class com.example.test_app.bar.BarInstrumentedTest#testBar",
        "class com.example.test_app.foo.FooInstrumentedTest#testFoo"
      ]
    }
  }
}

May replace !! with .orEmpty() in the future

bootstraponline · 2020-05-22T19:54:50Z

    // Flank debug

-    @Option(names = ["--dump-shards"], description = ["Dumps the shards to $shardFile for debugging"])
+    @Option(names = ["--dump-shards"], description = ["Dumps the shards to $ANDROID_SHARD_FILE for debugging"])


IosRunCommand.kt references ANDROID_SHARD_FILE, is that correct?

shame on me...

bootstraponline · 2020-05-22T19:55:12Z

    // Flank debug

-    @Option(names = ["--dump-shards"], description = ["Dumps the shards to $shardFile for debugging"])
+    @Option(names = ["--dump-shards"], description = ["Dumps the shards to $IOS_SHARD_FILE for debugging"])


AndroidRunCommand.kt references $IOS_SHARD_FILE ?

bootstraponline · 2020-05-22T20:04:49Z

gcloud:
  app: ../test_app/apks/app-debug.apk
  test: ./src/test/kotlin/ftl/fixtures/tmp/apk/app-debug-androidTest_155.apk
  num-uniform-shards: 50

produces:

{
  "matrix-0": {
    "app": "/Users/mac/code/flank/test_app/apks/app-debug.apk",
    "test": "/Users/mac/code/flank/test_runner/src/test/kotlin/ftl/fixtures/tmp/apk/app-debug-androidTest_155.apk",
    "shards": {
      "shard-0": [ ... ]

I think we should update --dump-shards to take into account num-uniform-shards. The idea is that given a config, we can preview the matrix/shard layout before running.

bootstraponline · 2020-05-22T20:16:56Z

I tested with max-test-shards and additional-app-test-apks. The feature looks great!

bootstraponline · 2020-05-22T20:19:59Z

The help message is a bit mixed up 🙂

$ flank android run --help
--dump-shards Dumps the shards to ios_shards.json for debugging

pawelpasterz · 2020-05-23T12:45:50Z

+        testFilter = TestFilters.fromTestTargets(args.testTargets)
+    ).mapValues { (_, testMethods: List<FlankTestMethod>) ->
+        when {
+            testMethods.isEmpty() -> emptyList<List<String>>().also { printNoTests(testApks) }


I think also should be used if we want to perform some additional actions on caller object which is not true here. I would prefer simple block code. We can write it in one line if you want
testMethods.isEmpty() -> { printNoTests(testApks); emptyList() }

IMO with also looks better because emptyList is important here but print is only additional side operation but I agree that avoiding unnecessary calls is a good idea 👍

pawelpasterz · 2020-05-23T12:51:11Z

+    testFilter: TestFilter
+): Map<InstrumentationTestApk, List<FlankTestMethod>> =
+    testApks.associateWith { testApk ->
+        DexParser.findTestMethods(testApk.test.local).asSequence().distinct().filter(testFilter.shouldRun).map { testMethod ->


How about more functional styling here, WDYT?

There are only function calls, no value assignments, no ordinary loops, even no conditions so I have no idea how to do it more functional :). Pls explain me by example.

Well my comment was just cosmetic one. Here we have invocation chain in one line, I was thinking about splitting it to new line each. So as I said 100% cosmetic

pawelpasterz · 2020-05-23T12:53:56Z

+    testApks: List<InstrumentationTestApk>
+): Map<InstrumentationTestApk, ShardChunks> =
+    getFlankTestMethods(
+        testApks = testApks.download(),


It would be great to have concurrent download but I guess it's for another PR :)
cc @bootstraponline

I was thinking about concurrent download, but decided to leave as is due to additional changes in tests. But I can add it easily. @bootstraponline ?

I think concurrent downloads would be nice

bootstraponline · 2020-05-23T17:40:03Z

The code looks great!

pawelpasterz previously approved these changes May 16, 2020

View reviewed changes

jan-goral reviewed May 18, 2020

View reviewed changes

bootstraponline dismissed pawelpasterz’s stale review via 2044e40 May 19, 2020 05:15

bootstraponline force-pushed the dump_shards branch from 2044e40 to d180af0 Compare May 19, 2020 05:19

pawelpasterz suggested changes May 19, 2020

View reviewed changes

jan-goral assigned jan-goral and bootstraponline and unassigned jan-goral May 19, 2020

jan-goral self-assigned this May 21, 2020

bootstraponline and others added 9 commits May 22, 2020 21:20

Dump shards for all test apks

cfa8c9d

Dump shards for all test apks

d6dc2ef

Refactor getTestShardChunks

d2e8881

Update release_notes.md

b7abfe3

Fix lint

5a93d6c

Remove ?: ""

9aa7f52

May replace !! with .orEmpty() in the future

Clean up code

6a6d464

Implement new output format for android_shards.json

e673d41

Update release_notes.md

07ed2c3

jan-goral force-pushed the dump_shards branch from af0c1fb to 07ed2c3 Compare May 22, 2020 19:24

jan-goral requested review from adamfilipow92, pawelpasterz and piotradamczyk5 May 22, 2020 19:25

bootstraponline commented May 22, 2020

View reviewed changes

pawelpasterz previously approved these changes May 23, 2020

View reviewed changes

Handle num-uniform-shards with dump-shards & fix descriptions

5606561

jan-goral dismissed pawelpasterz’s stale review via 5606561 May 23, 2020 14:18

Remove unnecessary also

e878905

pawelpasterz approved these changes May 25, 2020

View reviewed changes

pawelpasterz merged commit 2c328e1 into master May 25, 2020

pawelpasterz deleted the dump_shards branch May 25, 2020 11:06


		val filteredTests = testApks.map { getTestMethods(args, it) }.flatten()

		if (filteredTests.isEmpty()) println("${FtlConstants.indent}No tests for ${testApks.joinToString(", ")}")

Conversation

bootstraponline commented May 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

pawelpasterz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bootstraponline commented May 16, 2020

Uh oh!

runningcode commented May 17, 2020

Uh oh!

jan-goral May 18, 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!

codecov-commenter commented May 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pawelpasterz 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 May 19, 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!

bootstraponline commented May 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pawelpasterz commented May 20, 2020

Uh oh!

jan-goral commented May 22, 2020

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!

bootstraponline commented May 22, 2020

Uh oh!

bootstraponline commented May 22, 2020

Uh oh!

bootstraponline commented May 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jan-goral May 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!

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!

bootstraponline commented May 23, 2020

Uh oh!

Reviewers

Assignees

bootstraponline commented May 16, 2020 •

edited

Loading

jan-goral May 18, 2020 •

edited

Loading

codecov-commenter commented May 19, 2020 •

edited

Loading

pawelpasterz May 19, 2020 •

edited

Loading

bootstraponline commented May 20, 2020 •

edited

Loading

jan-goral May 23, 2020 •

edited

Loading