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

Run tests with Gradle test runner instead of randomizedtesting.junit4-ant #31496

Closed
alpar-t opened this issue Jun 21, 2018 · 14 comments
Closed
Assignees
Labels
:Delivery/Build Build or test infrastructure Team:Delivery Meta label for Delivery team

Comments

@alpar-t
Copy link
Contributor

alpar-t commented Jun 21, 2018

Currently we use https://github.com/randomizedtesting/ for running the tests.

The Gradle plugin calls into ant to do the heavy lifting.
It also replaces the Gradle Test task that the java plugin creates ( called test ) with it's own RandomizedTestingTask . This proved to be problematic, see #31324, and might no longer be possible in some future Gradle release.
Also, running the tests in parallel with the ant runner is independent from Gradle and does not honor Gradle --max-workers setting, which makes it hard to make other parts of Gradle run in parallel as it can lead to too many threads being created.

It is possible to use the reandmizedtesting runner i.e. @RunWith and run the tests with the Grade test runner. This proposal is to replace the ant part and not the runner.

@alpar-t alpar-t added the :Delivery/Build Build or test infrastructure label Jun 21, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@alpar-t
Copy link
Contributor Author

alpar-t commented Jun 21, 2018

Looking at what the ant runner implements and what we use it seems that we'll loose test class randomization and heartbeat. The former might eventually land in junit5 junit-team/junit5#13.
Gradle also measures how long tests take, so we should be fine without the heart beat.

It doesn't seem extremely difficult to implement random test order in Gradle, there is a precedent for it:
https://github.com/gradle/gradle/blob/93c34bdb6709f7ae9276c28e3d1b47b0495abb6a/subprojects/testing-base/src/test/groovy/org/gradle/api/internal/tasks/testing/processors/RunPreviousFailedFirstTestClassProcessorTest.groovy

However this is internal API so would need to convince Gradle it's useful and contribute it.

Other features of randomizedtesting, like stall thread detection are implemented in the runner.

@rjernst
Copy link
Member

rjernst commented Jun 21, 2018

it seems that we'll loose test class randomization and heartbeat.

I don't think it is acceptable to lose any of the functionality from randomized runner. We would need to implement this functionality over the gradle runner. I think it is all doable, though, at least with some work.

@alpar-t
Copy link
Contributor Author

alpar-t commented Jun 22, 2018

I agree. Especially the w.r.t the ordering of tests. Gradle doesn't have a public API (or any API I could find for that matter) that we can use. Implementing this by making changes to Gradle isn't neither hard or time consuming, but will have to figure out if that's something Gradle will accept, or if we are willing to live with the overhead of maintaining a custom Gradle distribution.

Another distinction that we might have to make is: are we ok to go without randomizing test class order temporarily, but I think in order to have an informed decision we would need to measure the advantages in terms of opportunity of build time reduction this will give us, and cluster formation improvements (#30904) also come into play there.

I think the first thing is to get to have a PoC run of the tests with Gradle so that we can at least have an initial measurement and comparison.

@alpar-t
Copy link
Contributor Author

alpar-t commented Jun 25, 2018

I isolated the hanging tests task in this repo: https://github.com/atorok/gradle_test_randomizedtesting_deadlock

@alpar-t
Copy link
Contributor Author

alpar-t commented Jun 25, 2018

The problem is in BootstrapForTesting if removed the tests run.

@alpar-t
Copy link
Contributor Author

alpar-t commented Jun 25, 2018

It's the security manage ./gradlew :server:test -Dtests.security.manager=false doesn't block and there's already gradle/gradle#3526

@alpar-t
Copy link
Contributor Author

alpar-t commented Jun 25, 2018

The Gradle runner seems to be a bit slower, running :server:test in 2m 6s vs 1m 52s (12.5%) both running on 6 forks, average of 3 runs each.

@rjernst
Copy link
Member

rjernst commented Jun 25, 2018

Another distinction that we might have to make is: are we ok to go without randomizing test class order temporarily

No, losing this would be a deal breaker. Reproducibility is paramount above all other concerns. It doesn't matter if we could run all tests in 10 seconds; if we can't reproduce failures, the speed is worthless.

It is unfortunate (but not surprising) that gradle cannot handle running with security manager. This is also a deal breaker for using the gradle runner.

Another option I considered which would gain us speed, would be to have separate randomized runner tasks, one for each jvm we plan to spawn. We could then do the assignment of classes to jvms late in configuration (or just before execution in a common doFirst or dependsOn). This would allow us all the utility of randomized runner, but utilizing separate tasks would allow parallelization with gradle.

@alpar-t
Copy link
Contributor Author

alpar-t commented Jul 13, 2018

I created an issue some time ago to be able to randomize test class order with Gradle and just relized it's not mentioned here: gradle/gradle#5760

@alpar-t
Copy link
Contributor Author

alpar-t commented Sep 5, 2018

It seems that Gradle won't be adding direct support for this as the direction is to move to junit 5 platform which deals with test execution order. On the bright side, our tests do seem to work with junit-vintage-engine without any code changes. We could implement our own vintage engine that also happens to randomize the order. Looking at the size of the engine I don't think maintenance would be too bad.

@mark-vieira
Copy link
Contributor

Sounds like controlling ordering is now supported in JUnit 4.13. This would still require a contribution to Gradle core to allow deferring ordering to the JUnit runner but that might be a more manageable way forward than migrating to JUnit 5, even with the legacy support. Probably worth spiking both options to get a better idea of what might be involved.

@mark-vieira mark-vieira self-assigned this Mar 14, 2019
@mark-vieira
Copy link
Contributor

@atorok I've removed the stalled label as I intend to progress this next week using your branch as a potential starting point.

@alpar-t
Copy link
Contributor Author

alpar-t commented Mar 15, 2019

For context we discussed this in the team and came to the conclusion that randomizing the class order might be not as important as we initially taught, so we will try to propose an implementation that doesn't take this into account.
- the test ordering right now is not fully deterministic based on the seed, there's balancing across JVMs based on historical timing data. This means that the order of classes might not be the same between runs even if the same seed is passed in. We would rather not have randomization and have reproducible order instead.
- ephemeral workers in CI miss out on the historical timing data entirely
- the performance benefit of the balancing is not significant enough to continue investing in this feature.
- the class of problems the test class order randomization was meant to solve ( static fields and initialization ) was largely eliminated and we are not overly concerned about it keeping back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Delivery/Build Build or test infrastructure Team:Delivery Meta label for Delivery team
Projects
None yet
Development

No branches or pull requests

4 participants