Support test sharding

[kcooney]

Overview

Add LauncherDiscoveryRequestBuilder.visitors()

This allows users of the launcher to learn about the discovered tests
before filters are applied. This is essential for build systems
like Bazel that want to "shard" (aka partition) a test run into
multiple processes while ensuring that the tests included in
each "shard" are consistent, even if test discovery does not
result in a deterministic ordering of tests (see https://goo.gl/Yj4fXL
for what Bazel does to Shard JUnit4 tests today).

This change also creates a wrapper to make a TestDescription
unmodifiable. This ensures that tests cannot be removed during
the visit phase, and cannot be added during the filtering stage.

Finally, FilterResult is made final, so that no one can override
the methods to make them inconsistent with one another.

---

I hereby agree to the terms of the JUnit Contributor License Agreement.

---

Definition of Done

• There are no TODOs left in the code
• Method preconditions are checked and documented in the method's Javadoc
• Coding conventions (e.g. for logging) have been followed
• Change is covered by automated tests
• Public API has Javadoc and @API annotations
• Change is documented in the User Guide and Release Notes
• All continuous integration builds pass

[sormuras]

This is very useful enhancement. I guess, being a couple days away from 5.0 GA, it will be part of the 5.1 train. What do you think, @junit-team/junit-lambda ?

[kcooney]

Sorry, @sormuras sorry I didn't see these issues sooner. I have been bugging the Bazel team to look at the Launcher API for a while, but they have been busy with other things

[marcphilipp]

@kcooney Can you provide a few more details why the visitors need to see descriptors before filters are applied? Please note that some filters (see implementations of DiscoveryFilter) are already applied by test engines during discovery. Will Bazel add a custom PostDiscoveryFilter for sharding?

Alternatively, could Bazel inspect the TestPlan, i.e. the result of the discovery process (without any filters), and then run test execution with additional filters (TestIdentifer is already immutable)?

[marcphilipp]

This is very useful enhancement. I guess, being a couple days away from 5.0 GA, it will be part of the 5.1 train. What do you think, @junit-team/junit-lambda ?

Some changes would break backwards compatibility so we should consider them for 5.0 GA.

[marcphilipp]

Tentatively slated for 5.0 GA in order to allow the team to assess this PR before GA (even though parts of it might get moved to 5.1).

[kcooney]

FYI: I realized that the only constructor for FilterResult is private, so there is no need to make it final. I removed the commit.

[kcooney]

Can you provide a few more details why the visitors need to see descriptors before filters are applied? Please note that some filters (see implementations of DiscoveryFilter) are already applied by test engines during discovery. Will Bazel add a custom PostDiscoveryFilter for sharding?

Yes, Bazel would add a custom PostDiscoveryFilter for test sharding.

I will do my best. @iirina can perhaps add more details.

For those that don't know, Bazel is a build tool, similar to Maven, but with advanced caching capabilities. In order to do that, Bazel actions need to be deterministic. Internally, each action knows about all of its inputs, so Bazel always knows what artifacts to build and what not to build.

Test sharding allows a test to be automatically split into multiple partitions ("shards") and run in separate processes. Here's an example of a sharded test:

java_test(
    name = "ServerTest",
    srcs = glob(["*.java"])
    shard_count = 3,
    flaky = 1,
    deps = [
        ...
    ],
)

Let's say that ServerTest has six test methods: a, b, c, d, e, f.

Let's assume that the test uses the round-robin strategy so that which shard a test is run is determined by this expression:

shard = testIndex % numShards;

Imagine that the first time the test is run, all three shards get the same sequence of test methods, in this order:

[ a, b, c, d, e, f]

Bazel will run the these test methods on each shard:

• shard 0: [a, d]
• shard 1: [b, e]
• shard 2: [c, f]

Imagine that test f fails because the code under test is broken. The java_test rule is marked as flaky so Bazel will try shard 2 again. In this re-run, JUnit provides the test methods in this order:

[ f, a, b, c, d, e]

In this case, shard 2 will run [b, e]. The failing test method is no longer on shard 2, and the test passes.

To avoid this, the round-robin strategy sorts the tests before determining which test is on which shard. So the shard that fails will always have the same tests when it is retried.

More generally, Bazel will pass the sharding strategy all of the tests, and the sharding strategy provides a filter. For Bazel to support sharding for JUnit 5 tests, it will need a way to see all tests before the filter runs.

It's okay if some engines provide filtering during discovery, as long as they do that in a deterministic way. So, for example, the test engine couldn't decide to filter out all tests annotated as @Nightly unless the test run was happening between midnight and 6am. The developer could, of course, specify in the build rule to run tests annotated with @Nightly and then decide to only run those tests once a night.

Alternatively, could Bazel inspect the TestPlan, i.e. the result of the discovery process (without any filters), and then run test execution with additional filters

How do you get access to the TestPlan before the filters are applied? testPlanExecutionStarted appears to be called after applyPostDiscoveryFilters() is called. Or are there opportunities after applyPostDiscoveryFilters() to do more filtering?

[marcphilipp]

How do you get access to the TestPlan before the filters are applied? testPlanExecutionStarted appears to be called after applyPostDiscoveryFilters() is called. Or are there opportunities after applyPostDiscoveryFilters() to do more filtering?

That's correct. My idea was to first call Launcher.discover() with a LauncherDiscoveryRequest without any PostDiscoveryFilters. discover() returns the TestPlan which can be inspected by Bazel. If it wants to run shard 1, it can pass a new LauncherDiscoveryRequest to Launcher.execute() that includes its implementation of PostDiscoveryFilter. Does that make sense?

[marcphilipp]

One additional idea: To ensure that a shard isn't modified, could Bazel store the unique IDs of the contained tests and re-run them using UniqueIdSelectors?

[kcooney]

Thanks for the response, @marcphilipp

I think your TestPlan solution will work (see below). I am still a bit worried that a PostDiscoveryFilter can modify the tree in unpredictable ways. I didn't realize that TestIdentifier existed. Perhaps PostDiscoveryFilter should take that instead of a TestDescriptor so we wouldn't need an UnmodifiableTestDescriptor. Although that would also be a breaking change, it is a small one (see https://github.com/kcooney/junit-lambda/tree/PostDiscoveryFilter)

One additional idea: To ensure that a shard isn't modified, could Bazel store the unique IDs of the contained tests and re-run them using UniqueIdSelectors?

Bazel could not store the unique IDs for the contained tests since it doesn't read the Java source files when determining whether a build action needs to be rerun (it just computes a hash of the files)

That's correct. My idea was to first call Launcher.discover() with a LauncherDiscoveryRequest without any PostDiscoveryFilters. discover() returns the TestPlan which can be inspected by Bazel. If it wants to run shard 1, it can pass a new LauncherDiscoveryRequest to Launcher.execute()

I think that would work, as long as the TestPlan contained a superset of the tests executed by Launcher.execute().

Thinking through this a bit in the next two paragraphs. If it's too long for you, feel free to ignore the rest.

In fact, that might be better, since Bazel fails if the test is "over-sharded" so a shard is empty (so we don't waste resources starting a JVM only to find out there are no tests). Bazel checks if a shard is empty after the sharding filter runs but before command line filtering happens. Bazel could check for empty shards by looking at the TestPlan, then when running the tests filter on shard and command-line args.

I don't think dynamically-generated tests would be a problem, as long as it's deterministic. For example, in my previous comment, if test b created test z during the test run, that's fine, since z will always be on the same shard as b.

[marcphilipp]

I'm closing this PR because test sharding can be implemented by using the existing Launcher API as described above. The potential problem of PostDiscoveryFilters modifying TestDescriptors persists and we might introduce an unmodifiable wrapper in a future release. For now, we've mitigated that edge case by documenting that PostDiscoveryFilters must not modify TestDescriptors in any way.

[kcooney]

Thanks, Marc. Great work on making the launcher API so flexible :-)

I sent out a related pull request related to test sharding. See #1055

[swankjesse]

FYI, I hacked together an extension to do this. It’s a bit unsatisfying because it filters with local information only, which is not great for balancing.
https://gist.github.com/swankjesse/92c93842f9c3705ca976031e7d0e664a

I’d love an official built-in sharding filter!

[sormuras]

Interesting implementation of an ExecutionCondition, Jesse.

Will bring it to our team discussion this week.

Scope is Jupiter-only, in contrast to the issue discussed here, which targeted Platform, IIRC.

PS: I'd love to update square/javapoet#677 as well.