Skip to content

Conversation

@RBusarow
Copy link
Contributor

@RBusarow RBusarow commented Jun 14, 2023

These shard tasks collectively depend upon all Android connectedCheck tasks in the entire project.

Each shard depends upon the connectedCheck tasks of some subset of Android projects. Projects are assigned to a shard by counting the number of @Test annotations within their androidTest directory, then associating those projects to a shard in a round-robin fashion.

These shards are invoked in CI using a matrix. If the number of shards changes, the connectedCheckShardMatrixYamlUpdate task can automatically update the workflow file so that they're all invoked.

The shard tasks are invoked as:

# roughly 1/3 of the tests
./gradlew connectedCheckShard1
# the second third
./gradlew connectedCheckShard2
# the last third
./gradlew connectedCheckShard3

The task filtering we currently use in CI (./gradlew fooShard1 -x :my-project:foo) will still work here, however the "cost" of the excluded task's tests is still accounted for when the sharding is performed.

@RBusarow RBusarow force-pushed the rick/connected_check_shards branch 14 times, most recently from 9ff304a to 037e21d Compare June 21, 2023 20:08
@RBusarow RBusarow marked this pull request as ready for review June 22, 2023 19:38
@RBusarow RBusarow requested review from a team and zach-klippenstein as code owners June 22, 2023 19:38
@RBusarow RBusarow marked this pull request as draft June 22, 2023 22:31
@RBusarow RBusarow force-pushed the rick/connected_check_shards branch from 037e21d to ebb3533 Compare June 22, 2023 23:15
sortedProjects.groupBy { (shardIndex++ % shardCount) + 1 }
}

(1..shardCount).map { shardIndex ->
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we have an off by one error and some boundary tests fall between the cracks, how will we notice that shard1 + shard2 + shard3 < allShards? Wondering if there's an invariant check we can put somewhere. 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe extract a core of this routine so that you can unit test it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't (and still don't) want to introduce unit tests to build-logic, since the tasks for an included build are not hooked into Gradle's implicit task delegation by default.

For example, if we invoke ./gradlew test today, it'll invoke all unit tests in kotlin-jvm modules. We just expect that to work. But that hook (implicit task delegation) only applies to sub-projects. The build-logic build is not affected by this, so its test task would never be invoked.

We can manually stitch things together, such as with this in the root build.gradle.kts:

tasks.named("test") {
  dependsOn(gradle.includedBuild("build-logic").task(":test"))
}

...but if anything were to happen to that, then the sharding could break in the way you described and no one would be the wiser.

So, instead I refactored it to be as off-by-one-proof as I could. As the tasks are configuring, they assert that the shard allocations total up to the exact number of Android projects and there are no duplicates. So they test themselves every time they're used.

@RBusarow RBusarow force-pushed the rick/connected_check_shards branch 7 times, most recently from ce5cb0e to 416bd38 Compare June 27, 2023 16:11
@RBusarow RBusarow marked this pull request as ready for review June 27, 2023 16:11
@RBusarow RBusarow force-pushed the rick/connected_check_shards branch 2 times, most recently from c8a4831 to 3cd2645 Compare June 27, 2023 21:29
@RBusarow RBusarow force-pushed the rick/connected_check_shards branch 9 times, most recently from 3078059 to 2f28d16 Compare June 28, 2023 03:00
@RBusarow RBusarow enabled auto-merge June 28, 2023 03:28
RBusarow added 3 commits June 27, 2023 22:28
These shard tasks collectively depend upon all Android `connectedCheck` tasks in the entire project.

Each shard depends upon the `connectedCheck` tasks of some subset of Android projects.
Projects are assigned to a shard by counting the number of `@Test` annotations within their
`androidTest` directory, then associating those projects to a shard in a round-robin fashion.

These shards are invoked in CI using a GitHub Actions matrix. If the number of shards changes,
the `connectedCheckShardMatrixYamlUpdate` task can automatically update the workflow file so
that they're all invoked.

The shard tasks are invoked as:

```shell
# roughly 1/3 of the tests
./gradlew connectedCheckShard1
# the second third
./gradlew connectedCheckShard2
# the last third
./gradlew connectedCheckShard3
```

The task filtering we currently use in CI (`./gradlew fooShard1 -x :my-project:foo`) will still work here, however the "cost" of the excluded task's tests is still accounted for when the sharding is performed.
@RBusarow RBusarow force-pushed the rick/connected_check_shards branch from 2f28d16 to 0c0cbb3 Compare June 28, 2023 03:28
@RBusarow RBusarow merged commit 00a2ef0 into main Jun 28, 2023
@RBusarow RBusarow deleted the rick/connected_check_shards branch June 28, 2023 03:51
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.

3 participants