-
Notifications
You must be signed in to change notification settings - Fork 119
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
Sharding #621
Sharding #621
Conversation
Codecov Report
@@ Coverage Diff @@
## master #621 +/- ##
============================================
- Coverage 76.31% 76.22% -0.09%
Complexity 610 610
============================================
Files 81 81
Lines 2297 2297
Branches 327 327
============================================
- Hits 1753 1751 -2
Misses 324 324
- Partials 220 222 +2 |
@@ -136,7 +136,8 @@ object Shard { | |||
testcases.sortByDescending { it.time } | |||
|
|||
testcases.forEach { testMethod -> | |||
val shard = shards.first() | |||
// num_shards must be > 1, and <= 50 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just for clarification. num shards must be less than 50 or number of tests per shard must be less than 50?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--num-uniform-shards=int
The number of shards should be less than the total number of test cases. The number of shards specified must be >= 1 and <= 50.
https://cloud.google.com/sdk/gcloud/reference/alpha/firebase/test/android/run
I think (hopefully) there's no limit on the number of tests per shard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is >= 1, and <= 50
5e98257
to
4827fe0
Compare
@@ -136,7 +136,8 @@ object Shard { | |||
testcases.sortByDescending { it.time } | |||
|
|||
testcases.forEach { testMethod -> | |||
val shard = shards.first() | |||
// num_shards must be > 1, and <= 50 | |||
val shard = shards.first { it.testMethods.size + args.testTargetsAlwaysRun.size < 50 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a problem here. According to doc the number of shards should be less than 50, but this condition prevents from putting more than 50 test methods per shard, so probably it's not what we want.
Hi, I have added some additional apks for specific test cases. Currently we are checking if new sharding is working properly on android, also with num-flaky-test-attempts flag on. And if everything will be ok I guess we can merge that. |
577e2bc
to
c4fc2d8
Compare
CLA Assistant Lite All Contributors have signed the CLA. |
I have read the CLA Document and I hereby sign the CLA |
I have read the CLA Document and I hereby sign the CLA |
316d248
to
a74fc56
Compare
I have read the CLA Document and I hereby sign the CLA |
a74fc56
to
3be5841
Compare
I have read the CLA Document and I hereby sign the CLA |
@pawelpasterz hey, the code looks great. Let's move all the apks into https://github.com/Flank/test_artifacts We don't want large binary files in the repo increasing the repo size. To solve for this, I publish the artifact bundle saparately which is downloaded as part of the test execution process. For now, delete the apks and I can merge the PR. As a follow up, we can add the APKs to the test_artifacts repo and I'll publish a new test bundle. |
@bootstraponline @pawelpasterz I have removed test apks but there is still one bug during test results fetch related to output directory, so we cannot merge until it will be resolved. Also i have 404 error when i tried to upload test apks in to https://github.com/Flank/test_artifacts. I need some time to investigate it. |
@jan-gogo I added you to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@bootstraponline
I've tested solution with various options, as much as I've been able to to verify -- works very well
@pawelpasterz Thank you so much! Awesome work. I'll merge this. |
No description provided.