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

feat: add test targets for shard #1299

Merged
merged 55 commits into from
Nov 18, 2020

Conversation

Sloox
Copy link
Contributor

@Sloox Sloox commented Nov 4, 2020

Fixes #1203
add test targets for shard
https://cloud.google.com/sdk/gcloud/reference/alpha/firebase/test/android/run#--test-targets-for-shard

Test Plan

How do we know the code works?

  1. Run config flank-single-shards-simple.yml
    gcloud:
      app: app-debug.apk
      test: app-multiple-success-debug-androidTest.apk
      test-targets-for-shard:
        - class com.example.test_app.bar.BarInstrumentedTest
        - class com.example.test_app.foo.FooInstrumentedTest
    
  2. Observe a matrix with 2 shards is running
  3. Each shard contains only 2 tests, and one of them is ignored
  4. Number of dumped shards is equal to expected (2)

Above is just a simple test plan, please, feel free to play around with this feature.

Checklist

  • Documented
  • Unit tested

@Sloox Sloox added Feature New Option Used to track PR with new configuration option (useful when updating fladle) labels Nov 4, 2020
@Sloox Sloox self-assigned this Nov 4, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Nov 4, 2020

Timestamp: 2020-11-18 16:42:11
Buildscan url for ubuntu-workflow run 370616001
https://gradle.com/s/rt3d6hqxteljc

@codecov-io
Copy link

codecov-io commented Nov 4, 2020

Codecov Report

Merging #1299 (56a9572) into master (415330a) will decrease coverage by 0.42%.
The diff coverage is 62.63%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1299      +/-   ##
============================================
- Coverage     79.55%   79.13%   -0.43%     
- Complexity      735      738       +3     
============================================
  Files           237      237              
  Lines          4579     4640      +61     
  Branches        796      808      +12     
============================================
+ Hits           3643     3672      +29     
- Misses          526      550      +24     
- Partials        410      418       +8     

@Sloox Sloox marked this pull request as ready for review November 6, 2020 07:52
@bootstraponline bootstraponline force-pushed the #1203-add-test-targets-for-shard branch from 53f2661 to ba650e1 Compare November 6, 2020 08:01
@Sloox
Copy link
Contributor Author

Sloox commented Nov 17, 2020

Unfortunately still a printing issue
Screenshot 2020-11-17 at 06 24 17

I am unsure how to approach this one? Any ideas?

@adamfilipow92
Copy link
Contributor

adamfilipow92 commented Nov 17, 2020

Unfortunately still a printing issue
Screenshot 2020-11-17 at 06 24 17

I am unsure how to approach this one? Any ideas?

ShardChunks is a List of List of Strings this is the reason why the single string is wrapped in []

so you could do something like this


fun shardsChunksListToString(objects: ShardChunks): String {
        if (objects.isNullOrEmpty()) return ""
        return NEW_LINE + objects.flatten()
            .joinToString("\n") { "         $it" }
    }

This should produce a result like:

      test-targets-for-shard: 
         class com.example.test_app.bar.BarInstrumentedTest
         class com.example.test_app.foo.FooInstrumentedTest

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.

  • Tested with filters
  • Tested without filters
  • Now output looks correct
    num-flaky-test-attempts: 0
      test-targets-for-shard: 
        - class com.example.test_app.bar.BarInstrumentedTest
        - class com.example.test_app.foo.FooInstrumentedTest

Good work 👍

@@ -265,6 +265,20 @@ object ArgsHelper {

return shards.map { Chunk(find + it.testMethods.filterNot { method -> find.contains(method) }) }
}

fun calculateDummyShards(
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need 2 methods named calculateDummyShards which both have only one call? IMO It's unnecessary redundancy. Besides that ArgHelper will be refactored soon.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the intention was to be in line with the current implementation

private fun InstrumentationTestContext.calculateShards(
args: AndroidArgs,
testFilter: TestFilter = TestFilters.fromTestTargets(args.testTargets)
): InstrumentationTestContext = ArgsHelper.calculateShards(

As you mentioned, there will be a huge refactor soon so within this PR we either refactor all similar occurrences or leave it as it is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed the reference from ArgsHelper 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pawelpasterz I removed the reference from ArgsHelper. and merely moved the logic into the CreateAndroidTestContext

Copy link
Contributor

@jan-goral jan-goral left a comment

Choose a reason for hiding this comment

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

Would be great to have a dedicated yml config for each testable flank's feature. So, the config from the description could be placed in the repository.

[EDIT] My bad, it already is

@Sloox Sloox requested a review from jan-goral November 18, 2020 11:57
@piotradamczyk5
Copy link
Contributor

@Mergifyio refresh

@mergify
Copy link

mergify bot commented Nov 18, 2020

Command refresh: success

Pull request refreshed

@piotradamczyk5 piotradamczyk5 merged commit 6a9a81c into master Nov 18, 2020
@piotradamczyk5 piotradamczyk5 deleted the #1203-add-test-targets-for-shard branch November 18, 2020 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android Feature New Option Used to track PR with new configuration option (useful when updating fladle) Tiger 🐯
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Android | Add --test-targets-for-shard
7 participants