Refactoring DefaultTestExecuter to allow customizable wiring within 3rd-party plugin

Help/Discuss
1

Hi,

I am the author of the dockerized test plugin and recently I am experimenting with test running in a cluster of docker hosts, i.e. Gradle tests worker processes running on a remote machine. It turned out that ForkingTestClassProcessor code is not robust enough: worker process can die before the connection gets established, or even connections could not be established due to some temp networking issue. This leads to failing test rounds, quite unnecessarily - a test class processor could try to start the worker again, hoping that this time everything will be ok, or if not throw the error if the number of attempts has reached some threshold.

Unfortunately, in order to implement such a strategy in my plugin, I need literally to copy a few classes from Gradle codebase and adjust relevant parts because factories are declared as anonymous classes, or the methods I need to overwrite are declared as private or package visible. IMHO, it would be a way better if the relevant Gradle classes could be extended, overwriting relevant factory methods, or even injecting relevant factories directly in Test task.

I am proposing that we:

  • either add `getters and setters for Factory<TestClassProcessor> and Runnable testClassScannerFactory to Test task, or
  • adding similar methods to DefaultTestExecuter, declare fields and the new methods as protected

What do Gradle maintainers think? I would gladly provide a PR.

Thanks,

Predrag

2

I’m not sure I like the idea of exposing getters/setters for internal classes so they can be used in a public fashion. I would prefer to approach this as a discussion of what sorts of things we should expose publicly on the Test task to enable a use case like yours. It may be that we end up making something like TestClassProcessor public and exposing a factory setter anyways, but I would rather arrive at that from the direction of enabling a publicly available feature. Can you elaborate on what your use case is? Perhaps there is a more general feature here that we want to expose…

3

As I mentioned in the post, at the moment is very hard to develop a plugin that needs to hook/replace a component in the Gradle test execution chain. You can set Test.testExecuter to use a custom executor, but if you want to replace just WorkerProcessFactory, ActorFactory, ModuleRegistry or BuildOperationWorkerRegistry is much harder. Furthermore, DefaultTestExecuter creates instances of TestClassProcessor and it is hard-coded that these are actually instances of ForkingTestClassProcessor. Same applies to MaxNParallelTestClassProcessor, or RestartEveryNTestClassProcessor.

In my latest experiments with dockerized-test plugin, I am playing with executing Gradle testworkers within Docker container running on a machine different from the one where Gradle main process is running. In such scenario, it might happen that an instance of WorkerProcess does not start or even crashes before it becomes ready for test execution. This is quite normal in the distributed setting, and we should try to start the process again, until some threshold is reached. Unfortunately, ForkingTestClassProcessor does that only once, and what is even worst, if there is an error, it is going to fail the whole test round at the end.

We could debate ofcourse, if we need to fix ForkingTestClassProcessor directly, but then we need to think about providing a customizable retry strategy, etc.

Now, if a plugin wants to replace ForkingTestClassProcessor, we need literally to copy the source code of DefaultTestExecuter in the plugin code base, and tweak lines responsible for returning the instance of TestClassProcessor. Not very nice, IMHO. Additionally, if something else gets fixed in Gradle codebase later, the plugin will not benefit from it, because it uses the class snapshoted at some point in time.

We could improve the current situation:

  • instead of creating factories within DefaultTestExecuter, we could inject them, as the others services we inject at the moment.
  • we extract factories into dedicated protected methods in DefaultTestExecuter and a plugin can subclass it, overrriding what needs to be changed.

I would rather inject all services instead of creating them explicitly in a class.

I hope it is clearer now.

Thanks,

Predrag