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 AndroidTestContext as base data for dump shards & test execution #817

Merged
merged 9 commits into from
Jun 1, 2020

Conversation

jan-goral
Copy link
Contributor

@jan-goral jan-goral commented May 26, 2020

#718
Probably will fix #818

This is refactoring of code that belongs to android-run and dumps-shards

Why those changes are important:

  1. Robo and instrumentation tests was not separated clearly so I have add AndroidTestContext sealed class which holds data for specific case.
  2. Some of code used for shard dumping and android test running was redundant, so I added createAndroidTestContexts function, which provides common data for shard dumping & android test running.

@codecov-commenter
Copy link

codecov-commenter commented May 26, 2020

Codecov Report

Merging #817 into master will increase coverage by 1.45%.
The diff coverage is 91.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #817      +/-   ##
============================================
+ Coverage     78.30%   79.75%   +1.45%     
  Complexity      667      667              
============================================
  Files           149      148       -1     
  Lines          3014     3008       -6     
  Branches        439      435       -4     
============================================
+ Hits           2360     2399      +39     
+ Misses          374      334      -40     
+ Partials        280      275       -5     

@bootstraponline
Copy link
Contributor

this looks awesome!

@jan-goral
Copy link
Contributor Author

this looks awesome!

Thanks 👍 I still have to make sure I didn't break anything.

@jan-goral
Copy link
Contributor Author

The additional side effect of this refactor is that now we can specify both robo test with instrumented test in one config and run it together.
Tested with following config:

gcloud:
  app: ./src/test/kotlin/ftl/fixtures/tmp/apk/app-debug.apk
  robo-script: ./src/test/kotlin/ftl/fixtures/test_app_cases/MainActivity_robo_script.json
  num-flaky-test-attempts: 2

flank:
  disable-sharding: false
  max-test-shards: 2
  num-test-runs: 1
  additional-app-test-apks:
    - test: ./src/test/kotlin/ftl/fixtures/tmp/apk/app-single-success-debug-androidTest.apk
    - test: ./src/test/kotlin/ftl/fixtures/tmp/apk/app-multiple-flaky-debug-androidTest.apk
    - test: ../test_app/apks/invalid.apk

@bootstraponline
Copy link
Contributor

The additional side effect of this refactor is that now we can specify both robo test with instrumented test in one config and run it together.

That's quite nice.

@jan-goral jan-goral marked this pull request as ready for review May 27, 2020 01:57
@jan-goral jan-goral requested a review from bootstraponline May 27, 2020 01:57
@jan-goral jan-goral self-assigned this May 27, 2020
@jan-goral jan-goral marked this pull request as draft May 27, 2020 01:58
@jan-goral
Copy link
Contributor Author

Should be merged after #801

Base automatically changed from 800-app-test-apks to master May 27, 2020 06:08
@jan-goral jan-goral marked this pull request as ready for review May 27, 2020 11:42
@jan-goral jan-goral force-pushed the android-test-context branch from b6996d8 to fa520f8 Compare May 27, 2020 11:54
get() = appApk != null && testApk != null ||
additionalAppTestApks.isNotEmpty() &&
(appApk != null || additionalAppTestApks.all { (app, _) -> app != null })
val isRoboTest
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be private?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

perhaps, now it could

runBlocking {
if (dumpShards)
dumpShards(config) else
newTestRun(config)
Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion keeping curly braces is more redable

runBlocking {
            if (dumpShards)
                dumpShards(config) else
                newTestRun(config)
      }

vs

runBlocking {
            if (dumpShards) {
                dumpShards(config)
            } else {
                newTestRun(config)
            }
        }

or

override fun run() {
        if (dryRun) {
            MockServer.start()
        }
        val config = AndroidArgs.load(Paths.get(configPath), cli = this)
        runProcess(config)
    }

    private fun runProcess(config: AndroidArgs) = runBlocking {
        if (dumpShards) {
            dumpShards(config)
        } else {
            newTestRun(config)
        }
    }

Tell me what you think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Curly braces make sense when u need to open new scope for more than one operation. Otherwise I prefer clear declarative expressions where curly braces are redundant.

adamfilipow92
adamfilipow92 previously approved these changes May 28, 2020
Copy link
Contributor

@adamfilipow92 adamfilipow92 left a comment

Choose a reason for hiding this comment

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

👍

@jan-goral jan-goral merged commit 0f30fe0 into master Jun 1, 2020
@jan-goral jan-goral deleted the android-test-context branch June 1, 2020 15:25
@inktomi
Copy link

inktomi commented Jun 1, 2020

I think this closed #818 but it shouldn't have. I was able to reproduce the issue on this branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shards failed - no test cases inside
7 participants