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

Fix #4255: Add support for sharding app module Gradle tests in CI #4256

Merged
merged 5 commits into from
Mar 19, 2022

Conversation

BenHenning
Copy link
Sponsor Member

@BenHenning BenHenning commented Mar 18, 2022

Explanation

Fixes #4255

This fixes the recent Gradle flakes we've seen by sharding app module tests into 4 separate runners.

Introduction of Gradle commands to facilitate sharding

Gradle doesn't have sharding built in, so I needed to take a hacky approach by excluding files from other shards when executing a particular shard (leaving only the tests corresponding to that shard). Since Gradle doesn't expect its source sets to change between builds, a clean build must separate running the shards. Running a shard is as simple as:

./gradlew :app:testDebugUnitTest --shard0

The current shards and their respective tests can be listed using:

./gradlew :app:testDebugUnitTest --list-shards

(Note that some of the tests that are excluded for localization are also included in the list but won't actually be executed).

Another requirement is ensuring that test shard bucketing is deterministic and consistent between runs and environments. To this end, it made sense to use the hash of test files' paths relative to the app module project directory. However, simply using the hash to bucket the tests (such as might happen in a hash table) resulted in a less even distribution of tests (probably due to Java's string hashCode function being fairly weak for uniqueness), so I decided to instead use the index as a seed of a PRNG. This resulted in a better distribution of tests among shards, and determinism.

CI changes & verifying that it works

The shards are fixed rather than computed like Bazel, so 4 new workflows were added to separately execute each shard. The results are then combined together in the same way as Bazel, and the result of that workflow is what blocks PR submission. I reused the existing app module workflow for the latter part, so we don't need to change the CI checks requirements (which then means no one needs to update their branches for submission).

See this workflow for an example failure, and this workflow to see that it the required check fails due to the shard failing.

Each shard uploads its test results separately (as demonstrated from this run).

The sharding also significantly speeds up running the app module tests (to the point where they're now probably faster than their Bazel counterparts), so this should be a nice qualify-of-life improvement beyond just the flake going away.

Essential Checklist

  • The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

N/A -- This is affecting only build infrastructure.

@BenHenning BenHenning marked this pull request as ready for review March 19, 2022 02:01
@BenHenning
Copy link
Sponsor Member Author

@anandwana001 PTAL.

@vinitamurthi if you have time, feel free to review as well. I'm happy to merge with either of your approvals.

Copy link
Contributor

@anandwana001 anandwana001 left a comment

Choose a reason for hiding this comment

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

LGTM

@oppiabot
Copy link

oppiabot bot commented Mar 19, 2022

Hi @BenHenning, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks!

@BenHenning
Copy link
Sponsor Member Author

Thanks @anandwana001! Going ahead and merging this since it should be a really big help on some of the large PRs currently pending merging.

@BenHenning BenHenning merged commit 8e19b37 into develop Mar 19, 2022
@BenHenning BenHenning deleted the shard-app-module-gradle-tests branch March 19, 2022 02:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gradle app module tests frequently fail in CI
3 participants