Replace usages RandomizedTestingTask with built-in Gradle Test #40564

mark-vieira · 2019-03-27T23:05:35Z

This pull request addresses #31496 by removing our existing RandomizedTestingPlugin and RandomizedTestingTask implementations from the build and instead using the built-in Test task from Gradle core.

While we don't use the Ant task provided by the randomized testing project, we do still otherwise still use the underlying testing framework so that hasn't changed. We continue to generate a random test seed per-build and inject it into all test task via the test.seed system property. All levels of testing now run as a Test task, to include unit tests, internal integration tests and rest integration tests.

There are a number of benefits to this change:

We no longer have to deal with multiple axis of parallelism. Gradle Test respects the global max-workers setting such that at any given time, no more the max workers number of tests can execute in parallel, regardless of how many other test tasks might be running at once. This severely limited our ability to run builds in parallel before, as we had to use a very conservative number for test.jvms or max-workers in order to not explode the number of concurrently executing tests.
Test results are now reported in build scans. This makes locating information pertaining to a particular build failure much easier. No more painful digging through Jenkins build logs.
We've been able to remove a bunch of custom logic and code for wrapping the randomized runner Ant task and dealing with progress logging.
Better IDE integration as now that tests use a "real" Test task, IDEs like IntelliJ will reliably interact with them. Features like delegating to the Gradle test runner from IntelliJ will work as expected, and with IntelliJ 2019.1 we'll be able to easily do things like run REST tests from the IDE.

Some examples of what test failures look link in build scans from a user perspective:

Failed unit test
Failed unit test task output
Failed integration test
Failed integration test task output
Failed REST test
Failed REST test task output
Link to specific test task output lines

elasticmachine · 2019-03-27T23:05:37Z

Pinging @elastic/es-core-infra

This commit puts a workaround in place for our existing test convention enforcement. The full-cluster-restart tests here are unique in that they include another test projects source in order to extend the test classes. The test conventions task isn't aware of this an fails due to these tests being excluded, but not run by any other task. We'll soon want to ditch the hack for a better method for sharing this kind of logic across projects anyhow, as this kind of thing won't work with newer versions of IntelliJ.

rjernst

This looks good overall. I'm really excited for the parallel execution benefits we can get!

I noticed when trying out the branch that using a reproduce line seems to run through many test suites, even though only one was specified. It slowed down running a single test by a noticeable margin.

Most of my other comments are about retaining equivalent output, which I think we can do with the output logger.

Additionally, when we originally switched to gradle, we output test failures as soon as they happened. Somewhere along the way we lost this. With gradle's Test class is it possible to dump test failures as they happen instead of waiting for the entire task to fail?

rjernst · 2019-03-28T04:27:18Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/AntTask.groovy

@@ -38,7 +38,7 @@ import java.nio.charset.Charset
 public abstract class AntTask extends DefaultTask {

    /**
-     * A buffer that will contain the output of the ant code run,
+     * A eventBuffer that will contain the output of the ant code run,


Looks like this file was accidentally changed through find/replace?

Good catch.

rjernst · 2019-03-28T04:34:35Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy

-            jvmArg '-Xms' + System.getProperty('tests.heap.size', '512m')
-            jvmArg '-XX:+HeapDumpOnOutOfMemoryError'
+        // none of this stuff is applicable to the `:buildSrc` project tests
+        if (project.path != ':build-tools') {


Can we do this conditional restriction in build-tools itself? I don't think BuildPlugin should know anything about our special pseudo project we use to publish buildSrc.

The logic lives in the plugin, so we'd have to expose some flag or DSL for project to tell the plugin "don't apply the test stuff". This might be a little overkill since this is an exception case. I do agree this is a bit coupled but it's not the only instance of this kind of thing.

I think the bigger problem is that the scope of build-plugin has perhaps gotten a bit large. It really should be chopped up into several more focused plugins, for which a general "build plugin" then applies. This would allow us to ditch stuff like this. And subsequently apply only the subset of plugins we actually need to projects like build-tools.

rjernst · 2019-03-28T04:51:58Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy

-                slowTests {
-                    heartbeat 10
-                    summarySize 5
+                if (System.getProperty('tests.jvm.argline')) {


No need for the conditional. Use the getProperty variant that takes a default, and pass it empty string.

Turns out this is necessary. If we pass an empty string as a jvm arg to the test worker Java interprets this as the main class name and explodes 💣

But it would not be passing an empty string? It's an empty iterable to jvmArgs, since that is what split on an empty string would return.

Hmmm, true. Something else must be going on. In any case I backed it out for now as there are other more pressing items than a superfluous null check 😉

rjernst · 2019-03-28T05:04:13Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy

+                // TODO: remove setting logging level via system property
+                systemProperty 'tests.logger.level', 'WARN'
+                System.getProperties().each { key, value ->
+                    if ((key.startsWith('tests.') || key.startsWith('es.')) && !key.equals('tests.seed')) {


I don't think we need the protection for tests.seed. If it was set as a system property, project.testSeed is equal to it. So we would just set it twice, but systemProperty is backed by a map, so it's essentially a no-op.

Yep, sounds good.

rjernst · 2019-03-28T05:10:31Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy

+                }
+
+                testLogging {
+                    maxGranularity = 2


why not granularity of 3? We have some classes that run for a long time due to slow test methods. Currently we show the test class and method that is executing.

I've tweaked this a bit. We now get per test errors that include the cause.

org.elasticsearch.cluster.routing.AllocationIdTests > testShardToStarted FAILED org.junit.ComparisonFailure: expected:<[foo]> but was:<[bar]> at __randomizedtesting.SeedInfo.seed([BCEB7E8F10F805F5:D5D51C2E74CF889D]:0) at org.junit.Assert.assertEquals(Assert.java:115) at org.junit.Assert.assertEquals(Assert.java:144) at org.elasticsearch.cluster.routing.AllocationIdTests.testShardToStarted(AllocationIdTests.java:57)

rjernst · 2019-03-28T06:09:31Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/test/ErrorReportingTestListener.groovy

+
+                        for (TestOutputEvent event : events) {
+                            if (event.getDestination() == TestOutputEvent.Destination.StdOut) {
+                                logger.lifecycle(event.getMessage())


The message here already has a newline in it. By using the logger methods, we double the newlines. Additionally, using the logger appears to be considerably slower. I tested a dummy failure in a test class with lots of output (CoordinatorTests). On master the output is immediate. On this branch, my terminal scrolls for 10s of seconds, maybe more (I killed it before it completed).

Indeed. I've changed this to just print directly to stdout and stderr. It doesn't seem just a bit quickly but nothing drastic.

rjernst · 2019-03-28T06:15:58Z

buildSrc/src/main/groovy/com/carrotsearch/gradle/junit4/TestReportLogger.groovy

-        forkedJvmCount = e.getSlaveCount();
-        jvmIdFormat = " J%-" + (1 + (int) Math.floor(Math.log10(forkedJvmCount))) + "d";
-
-        outStream = new LoggingOutputStream(logger: logger, level: LogLevel.LIFECYCLE, prefix: "  1> ")


We are losing the prefixes for stdout/stderr in test output. I think we should maintain those.

I've added these back into ErrorReportingTestListener.

rjernst · 2019-03-28T06:49:13Z

buildSrc/src/main/groovy/com/carrotsearch/gradle/junit4/TestReportLogger.groovy

-                            'Assumption #%d: %s',
-                            count, fm.getMessage() == null ? '(no message)' : fm.getMessage()));
-                } else {
-                    writer.write(String.format(Locale.ENGLISH,


We seem to have lost this failure output after a test fails. Currently we get something like this:

2> REPRODUCE WITH: ./gradlew :server:unitTest -Dtests.seed=36216D772C09F4C0 -Dtests.class=org.elasticsearch.common.io.StreamsTests -Dtests.method="testCopyFromByteArray" -Dtests.security.manager=true -Dtests.locale=bs-Latn-BA -Dtests.timezone=America/Knox_IN -Dcompiler.java=12 -Druntime.java=12 FAILURE 0.26s | StreamsTests.testCopyFromByteArray <<< FAILURES! > Throwable #1: java.lang.AssertionError: oops > at __randomizedtesting.SeedInfo.seed([36216D772C09F4C0:CCDF0A5443324BED]:0) > at org.elasticsearch.common.io.StreamsTests.testCopyFromByteArray(StreamsTests.java:47) > at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) > at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) > at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) > at java.base/java.lang.reflect.Method.invoke(Method.java:567) > at java.base/java.lang.Thread.run(Thread.java:835)

With this PR, I only see the reproduce line.

See above. I've tweaked the output such that we report per-test error causes.

rjernst · 2019-03-28T06:52:30Z

buildSrc/src/main/groovy/com/carrotsearch/gradle/junit4/TestReportLogger.groovy

-        if (config.showNumFailuresAtEnd > 0 && !failedTests.isEmpty()) {
-            List<Description> sublist = this.failedTests
-            StringBuilder b = new StringBuilder()
-            b.append('Tests with failures')


I think this summary of the tests that failed is much nicer than what gradle outputs. Can we do our own summary?

We currently get something like this:

Tests with failures: - org.elasticsearch.common.io.StreamsTests.testCopyFromByteArray [junit4] JVM J0: 0.54 .. 2.82 = 2.28s [junit4] Execution time total: 2.83 sec. [junit4] Tests summary: 1 suite, 5 tests, 1 failure

In this PR we get:

org.elasticsearch.common.io.StreamsTests FAILED 5 tests completed, 1 failed

So we lose the actual test method, the number of suites, and the timing breakdowns. The last two I think we can live without, since they are available in the local build report, but the list of test methods that failed should be available directly in the output. We shouldn't need to copy/paste the build report link just to find which tests actually failed.

With the tweaks to reporting we log the test method at failure time. If you want to see the exact tests that failed you'll have to scroll up, or peak at the build scan.

rjernst · 2019-03-28T07:15:08Z

buildSrc/src/main/groovy/com/carrotsearch/gradle/junit4/TestReportLogger.groovy

-    List<Description> failedTests = new ArrayList<>()
-
-    /** Stack trace filters. */
-    StackTraceFilter stackFilter = new StackTraceFilter()


I think the stack trace filter is something we want to keep. The traces can get quite long with all the additional cruft of junit/runner stuff.

This is going to be more difficult to implement. We could filter it out of the raw logs that we spit out at the end of suites but there aren't any hooks to tweak the stacktraces that get reported to Gradle, and by proxy, into build scans.

alpar-t

I'm looking forward to this!
I left a few comments.

alpar-t · 2019-03-28T11:50:47Z

buildSrc/build.gradle

@@ -199,24 +199,21 @@ if (project != rootProject) {
    into localDownloads
  }

-  unitTest {


This change was a bit forced on us since we couldn't replace test, but I don't think we should revert it as it brings clarity.

I'm not sure all projects having a test task that does nothing, in addition to a unitTest task is particularly clear. I'm not sure we should continue with this funny convention just for historical purposes. We can now also ditch all the goofy logic to disable it, as well as inherit the normal conventions for this task rather than duplicating what the java plugin already does for us. Seems unnecessary to me and as you say, this naming was only introduced in the first place due to a technical limitation.

I don't think educating folks that :test == unit tests is asking a lot.

The change wasn't done only as a technical limitations.
I do and always have hated the Gradle test task for the ambiguity it brings,
and for running IT by default, it makes it too easy to mangle tests of different kinds, and it really probably only exists for historical reasons because other build tools had it.
Also, the fact that it's being changed in this PR has nothing to do with the goal of the PR and just makes it much harder to read.

My goal is to have specific unitTest, integTest, and internalClusterTest tasks with well defined meanings eventually. test doesn't contribute to this clarity,
it's just a catch all which makes it seem ok to add stuff there and move on, whereas unitTest makes one think about what tests are being added and what's the best place for them.

Gradle is broken in that it enforces a testing conventions that does not scale for anything but projects of trivial size with no convenient way to opt-out.

I do and always have hated the Gradle test task for the ambiguity it brings,
and for running IT by default, it makes it too easy to mangle tests of different kinds,

To be fair, I think you are just describing Maven surefire and failsafe plugin behaviors. Which themselves are a workaround to a technical limitation which is that Maven doesn't support more than two source sets per module. The proper way to separate tests like this is to model them as separate source sets, not just different Test tasks with includes/excludes based on naming conventions. This has lots of other benefits as well.

My goal is to have specific unitTest, integTest, and internalClusterTest tasks with well defined meanings eventually.

I think we are bikeshedding to some degree. Again, I don't think it's a big deal to convey that test runs unit tests, and other tests run the tests as the name describes.

Gradle is broken in that it enforces a testing conventions that does not scale for anything but projects of trivial size with no convenient way to opt-out.

I think it's the opposite actually. It doesn't enforce any conventions, which I think is your complaint. I think the "convention" you have issue with is the name of the task it creates. This is really the only thing Gradle is "forcing" on us.

alpar-t · 2019-03-28T11:54:37Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy

-        project.tasks.withType(RandomizedTestingTask) {task ->
-            jvm "${project.runtimeJavaHome}/bin/java"
-            parallelism System.getProperty('tests.jvms', project.rootProject.ext.defaultParallel)
-            ifNoTests 'fail'


Do we have an equivalent of this ? This is important to catch typos when running with a single test from the command line.

The Gradle Test task will fail if you pass a --tests pattern that doesn't match any tests.

Execution failed for task ':server:test'. > No tests found for given includes: [**/*Tests.class](include rules) [**/*$*.class](exclude rules) [org.elasticsearch.cluster.coordinaasfasfdrdinatorTests.*](--tests filter)

That's fine as long as we use it reproduction lines or have the same with the system properties, or just fail if the system properties are there telling people the proper way to do it.

I've updated the reproduction line generate to use --tests instead.

alpar-t · 2019-03-28T11:59:59Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy

@@ -1068,13 +1043,6 @@ class BuildPlugin implements Plugin<Project> {
        return 'auto';


Will no longer work with Gradle :)

Yes, I actually wasn't using this method at all and instead was naively using max-workers but I've updated this such that we fall back to Runtime.availableProcessors() instead of returning "auto".

We should keep the default in the randomized runner that capped that to 4, or go with half the processors, using all of them is known to cause tests to fail.

alpar-t · 2019-03-28T12:01:24Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy

+        /* Set the testSeed on the root project first so other projects can use
+         * it during initialization. */
+        project.rootProject.ext.testSeed = testSeed
+        project.rootProject.subprojects {


Why set it on each project if we only look for it on the root ?

This was historical. I agree, I don't think it's necessary.

alpar-t · 2019-03-28T12:02:01Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/test/ErrorReportingTestListener.groovy

@@ -0,0 +1,121 @@
+package org.elasticsearch.gradle.test


Sorry, no more new Groovy classes.
Please convert it to Java, our goal is to get rid of Groovy in buildsrc.

FWIW, this class is marked with @CompileStatic which means we get all the same type safety and performance benefits of Java, with the conveniences of Groovy. But in general, I think I would prefer and all-Java codebase as well. I've added this annotation to a couple of other more trivial classes as well as part of this work while I was at it.

I had actually initially started to convert to Java all classes I so much as modified during this exercise but that quickly got out of hand as code in src/java cannot refer to code in src/groovy so you eventually hit a point where due to code dependencies, you have to just convert everything. One thing we could do to make this transition easier, and not have to basically move everything in one big step to is to actually put all code under src/groovy and leverage Groovy joint compilation. This would enable Groovy to call Java and vice versa. The downside is a performance hit during compilation, but would make it easier to convert things a bit more piecemeal.

In any case, the class in question has no such dependencies, so was trivial to move 😉

I'we resisted moving everything to src/groovy as being forced to think about the coupling between classes and converting them in this manner leads to a better structure and encourages removing coupling where not necessary (e.x. gluing things together in the build script rather than them knowing about each-other in buildSrc where it makes sense )

alpar-t · 2019-03-28T12:09:48Z

buildSrc/src/main/java/org/elasticsearch/gradle/testfixtures/TestFixturesPlugin.java

@@ -103,24 +102,19 @@ public void apply(Project project) {
                "Tests for {} require docker-compose at /usr/local/bin/docker-compose or /usr/bin/docker-compose " +
                    "but none could be found so these will be skipped", project.getPath()
            );
-            disableTaskByType(tasks, getTaskClass("com.carrotsearch.gradle.junit4.RandomizedTestingTask"));


We do need to disable Task here. This plays a role when docker is not available. -Dtests.fixture.enabled=false simulates this, so when the plugin is used it will skipp the tests assuming these can't run because there's no docker.

I think you meant Test, and yes, I believe you are right. I've fixed this.

alpar-t · 2019-03-28T12:11:28Z

docs/build.gradle

@@ -1051,7 +1051,7 @@ buildRestTests.setups['calendar_outages_addevent'] = buildRestTests.setups['cale
       ml.post_calendar_events:
         calendar_id: "planned-outages"
         body:  >
-           { "events" : [
+           { "eventBuffer" : [


probably unintentional

Yes, stupid IntelliJ. There were actually a number of other instances as well.

alpar-t · 2019-03-28T12:14:21Z

qa/die-with-dignity/build.gradle

@@ -27,11 +27,11 @@ esplugin {
 integTestRunner {
    systemProperty 'tests.security.manager', 'false'
    systemProperty 'tests.system_call_filter', 'false'
-    systemProperty 'pidfile', "${-> integTest.getNodes().get(0).pidFile}"
-    systemProperty 'log', "${-> integTest.getNodes().get(0).homeDir}/logs/${-> integTest.getNodes().get(0).clusterName}_server.json"
+    nonInputProperties.systemProperty 'pidfile', "${-> integTest.getNodes().get(0).pidFile}"


Wouldn't this work without nonInputProperties due to the closure in the Gstring ?
I think this extension is confusing and complicates builds scripts.

All GStrings which have a closure that includes a reference to something non-serializable will blow up at input snapshotting time. In this case you can see this closure refers to integTest which is a Task, which is most definitely not serializable.

Essentially, we need a way to be able to pass a system property to tasks, but not have Gradle treat it as an input. We actually need this for two reasons. First for the reason mentioned above, which is that property values that cannot be serialized blow up input snapshotting. Second, is that some of these will be unstable in nature and will break cacheability, and we want to be able to tell Gradle to ignore them.

We can certainly tweak the DSL and make it better. Perhaps rather than nonInputProperties.systemProperty we just make it nonInputSystemProperty?

Do you think something like dyncamicSystemProperty that reveals more context for the test author would be more appropriate ? I'm concerned that for someone not that intimate with the build and Gradle nonInput will be meaningless, or create a connotation that these are not being sent to the test runner.

Also, while at it, we should only have it take a closure and get rid of all the GString hacks for clarity.

Do you think something like dyncamicSystemProperty that reveals more context for the test author would be more appropriate ?

Yeah, I'm not sure. Mainly because we will certainly use this for things that are not "dynamic", in the sense that we won't always pass a lazy G-string here. We'll use this for anything that changes and we want Gradle to ignore as an input, for example, for local and pr builds, the test seed itself.

I'm concerned that for someone not that intimate with the build and Gradle nonInput will be meaningless

I'm sure this isn't the first case of developers interacting with something in the build that they don't fully grasp 😉. The goal should be to hide some of this complexity behind plugins so they aren't in build scripts. I think we should consider doing that instead as many of these instances are duplicates and could probably simply be part of the RestIntegTestTask (or a plugin we convert it to).

Also, while at it, we should only have it take a closure and get rid of all the GString hacks for clarity.

Agreed. We can do that simply enough.

How about notCachable or noCache instead of nonInput ?
I find nonInput really confusing as the system properties _are_inputs to the test and there's no indication that this refer to task.inputs

@rjernst care to join the bikeshedding? 😄

I'm fine with nonInput for now. I think it is something we can discuss further if there is confusion and rename easily.

# Conflicts: # buildSrc/src/main/java/org/elasticsearch/gradle/testfixtures/TestFixturesPlugin.java # plugins/repository-hdfs/build.gradle # test/fixtures/hdfs-fixture/build.gradle # test/fixtures/krb5kdc-fixture/build.gradle # x-pack/qa/full-cluster-restart/build.gradle # x-pack/qa/rolling-upgrade/build.gradle

…newlines

This reverts commit 33a5cd3

mark-vieira · 2019-03-28T23:27:02Z

I noticed when trying out the branch that using a reproduce line seems to run through many test suites, even though only one was specified. It slowed down running a single test by a noticeable margin.

@rjernst I noticed this too. This is because the -Dtest.class, etc properties get enforced at the runner level. Gradle still passes all the test classes then the runner filters them out. I think the better thing to do would be to adjust the reproduction line generator to use the Gradle --tests option syntax rather than the system properties.

alpar-t · 2019-04-01T05:53:58Z

@mark-vieira you can quickly spin up GCP VMs from CI worker images and run a build like intake would I think that might be beneficial given how broad the change is, would also be interesting to see if there are any significant differences in performance before we merge.

Intake build is a parameterized build, which allows you to choose a branch? Could I push this is a branch and run it that way. That way all the same env vars are set, etc. I ended up finding an issue simply because of the way Jenkins set the RUNTIME_JAVA_HOME.

That would put your intake job in line with other intake jobs, causing confusion with test triage, and making intake look worse than it is. I would be ok to set up some CI jobs that has no notifications to the team and a specific name for this purpose.
I'm used to using GCP VMs because that also allow me to experiment with the instance size and configuration when needed, but either way works.

mark-vieira · 2019-04-02T22:34:38Z

@atorok @rjernst Do you mind taking another look through my latest changes. I believe I've address all your comments.

I spent some time with @rjernst today discussing failure output and I think we settled on a good state. You can see an example of a few failed tests and the corresponding reporting in this build scan.

I also did as @atorok suggested and spun up a GCP instance to do a test intake build run to see if there was any regressions in terms of build times. So far everything is basically identical to the randomized runner task in terms of execution time. You can see what the intake build looks like here.

# Conflicts: # buildSrc/src/main/groovy/org/elasticsearch/gradle/test/RestIntegTestTask.groovy

rjernst

LGTM, two remaining things from me.

rjernst · 2019-04-04T05:48:39Z

test/framework/src/main/java/org/elasticsearch/test/junit/listeners/ReproduceInfoPrinter.java

-                //prints out the raw method description instead of methodName(description) which filters out the parameters
-                super.appendOpt(SYSPROP_TESTMETHOD(), "\"" + description.getMethodName() + "\"");
+            RandomizedContext ctx = null;
+            try {


I don't think copying appendAllOpts is necessary. We already override appendOpt, and filter away some properties we don't want to print.

Good call. I've just added the test class to the list of filtered items in appendOpt().

rjernst · 2019-04-04T05:49:56Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/test/StandaloneRestTestPlugin.groovy


        // only setup tests to build
-        project.sourceSets.create('test')
+        def sourceSets = project.extensions.getByType(SourceSetContainer)


This was left unaddressed?

alpar-t · 2019-04-04T07:45:58Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/test/RestIntegTestTask.groovy

-public class RestIntegTestTask extends DefaultTask {
+class RestIntegTestTask extends DefaultTask {
+
+    private static Logger LOGGER = Logging.getLogger(RestIntegTestTask)


the convention in existing code is to use lowercase for these logger

This should be marked final actually which would make the current case appropriate I believe.

This is a matter of personal preference for sure, for me uppercase logger hurts my eyes and fingers, final or not :)

alpar-t · 2019-04-04T07:47:23Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/test/RestIntegTestTask.groovy

+         * side-effect that these properties are NOT treated as inputs, therefore they don't influence things like the
+         * build cache key or up to date checking.
+         */
+        def nonInputProperties = new CommandLineArgumentProvider() {


I'm still not happy with this name, but we can look at it in a follow up.

alpar-t · 2019-04-04T07:53:39Z

buildSrc/src/main/java/org/elasticsearch/gradle/test/ErrorReportingTestListener.java

+            if (testDescriptor.getParent() != null) {
+                // go back and find the reproduction line for this test failure
+                List<TestOutputEvent> events = eventBuffer.get(Descriptor.of(testDescriptor.getParent()));
+                for (int i = events.size() - 1; i >= 0; i--) {


This is a matter of personal preference.
I find streams more readable. This could be IntStream.map().filter().foreach(System.err::print)

That's not the same here as we iterating backward through this list and then stopping when we find the first matching candidate.

You could handle that from the map as the size is known, but I don't have a strong preference.

alpar-t

LGTM just a few small things

# Conflicts: # modules/reindex/build.gradle

#40564)" This reverts commit 2b2a3f5

* elastic/master: (36 commits) Remove unneded cluster config from test (elastic#40856) Make Fuzziness reject illegal values earlier (elastic#33511) Remove test-only customisation from TransReplAct (elastic#40863) Fix dense/sparse vector limit documentation (elastic#40852) Make -try xlint warning disabled by default. (elastic#40833) Async Snapshot Repository Deletes (elastic#40144) Revert "Replace usages RandomizedTestingTask with built-in Gradle Test (elastic#40564)" Init global checkpoint after copy commit in peer recovery (elastic#40823) Replace usages RandomizedTestingTask with built-in Gradle Test (elastic#40564) [DOCS] Removed redundant (not quite right) information about upgrades. Remove string usages of old transport settings (elastic#40818) Rollup ignores time_zone on date histogram (elastic#40844) HLRC: fix uri encode bug when url path starts with '/' (elastic#34436) Mutes GatewayIndexStateIT.testRecoverBrokenIndexMetadata Docs: Pin two IDs in the rest client (elastic#40785) Adds version 6.7.2 [DOCS] Remind users to include @ symbol when applying license file (elastic#40688) HLRC: Convert xpack methods to client side objects (elastic#40705) Allow ILM to stop if indices have nonexistent policies (elastic#40820) Add an OpenID Connect authentication realm (elastic#40674) ...

…ic#40564) This commit replaces the existing RandomizedTestingTask and supporting code with Gradle's built-in JUnit support via the Test task type. Additionally, the previous workaround to disable all tasks named "test" and create new unit testing tasks named "unitTest" has been removed such that the "test" task now runs unit tests as per the normal Gradle Java plugin conventions

elastic#40564)" This reverts commit 2b2a3f5

Replace usages RandomizedTestingTask with built-in Gradle Test

2d9c898

mark-vieira added :Delivery/Build Build or test infrastructure v7.0.0 v8.0.0 v7.2.0 v6.7.1 labels Mar 27, 2019

mark-vieira self-assigned this Mar 27, 2019

mark-vieira requested review from rjernst and alpar-t March 27, 2019 23:05

rjernst requested changes Mar 28, 2019

View reviewed changes

alpar-t requested changes Mar 28, 2019

View reviewed changes

mark-vieira added 16 commits March 28, 2019 13:40

Fix up some merge conflicts

8bb56d4

Use existing mechanism for determining test parallelism

22d4db8

Fix overzelous find and replace

5437841

Tweak test event output

a6e23f2

Log strait to std streams as it's faster and doesn't introduce extra …

fb8e639

…newlines

Remove unnecessary conditional

33a5cd3

Remove unnecessary check

8081e84

Prefix test log output with std stream identifier

ee88d66

Fix overzelous find and replace

9e882d8

Fix overzelous find and replace

7df907d

Fix overzelous find and replace

d63296a

Fix overzelous find and replace

ceecad6

Simplify parallelism logic

371dba7

Disable test tasks when Docker is required but not available

71a7bb2

Revert "Remove unnecessary conditional"

313fb10

This reverts commit 33a5cd3

Merge remote-tracking branch 'origin/master' into gradle-test-runner

7801b8f

mark-vieira removed the v6.7.2 label Apr 1, 2019

mark-vieira added 3 commits April 1, 2019 12:52

Merge remote-tracking branch 'origin/master' into gradle-test-runner

4fe39f5

Don't both setting the test seed on all subprojects

a245b22

Improve test failure reporting

cf9b01b

mark-vieira added the >non-issue label Apr 3, 2019

Merge remote-tracking branch 'origin/master' into gradle-test-runner

7cc13b2

# Conflicts: # buildSrc/src/main/groovy/org/elasticsearch/gradle/test/RestIntegTestTask.groovy

rjernst approved these changes Apr 4, 2019

View reviewed changes

alpar-t reviewed Apr 4, 2019

View reviewed changes

alpar-t approved these changes Apr 4, 2019

View reviewed changes

mark-vieira added 4 commits April 4, 2019 09:27

Merge remote-tracking branch 'origin/master' into gradle-test-runner

5e0b83c

# Conflicts: # modules/reindex/build.gradle

Remove duplicate code in overridden method

429a506

Mark logger final

395a8ed

Declare variables with explicit types

8d67e0d

mark-vieira merged commit 2b2a3f5 into elastic:master Apr 5, 2019

mark-vieira added the backport pending label Apr 5, 2019

mark-vieira added a commit that referenced this pull request Apr 5, 2019

Revert "Replace usages RandomizedTestingTask with built-in Gradle Test (

b439127

#40564)" This reverts commit 2b2a3f5

mark-vieira removed the backport pending label Apr 5, 2019

jkakavas mentioned this pull request Apr 5, 2019

[CI] :modules:reindex task can't run on FIPS JVMs #40904

Closed

mark-vieira mentioned this pull request Apr 8, 2019

Replace usages RandomizedTestingTask with built-in Gradle Test #40978

Merged

gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this pull request May 27, 2019

Revert "Replace usages RandomizedTestingTask with built-in Gradle Test (

9a1fd06

elastic#40564)" This reverts commit 2b2a3f5

mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

		@@ -1068,13 +1043,6 @@ class BuildPlugin implements Plugin<Project> {
		return 'auto';

Replace usages RandomizedTestingTask with built-in Gradle Test #40564

Replace usages RandomizedTestingTask with built-in Gradle Test #40564

Conversation

mark-vieira commented Mar 27, 2019

elasticmachine commented Mar 27, 2019

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mark-vieira Mar 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpar-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mark-vieira Mar 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mark-vieira Mar 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mark-vieira commented Mar 28, 2019

alpar-t commented Apr 1, 2019

mark-vieira commented Apr 2, 2019

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpar-t left a comment

Choose a reason for hiding this comment

mark-vieira Mar 28, 2019 •

edited

Loading

mark-vieira Mar 28, 2019 •

edited

Loading

mark-vieira Mar 29, 2019 •

edited

Loading