Testing conventions task part 2

build.gradle

@@ -640,4 +640,3 @@ allprojects {
 
 
 
-

buildSrc/build.gradle

@@ -147,10 +147,8 @@ if (project == rootProject) {
       mavenLocal()
     }
   }
-  test {
-    include "**/*Tests.class"
-    exclude "**/*IT.class"
-  }
+  // only run tests as build-tools
+  test.enabled = false
 }
 
 /*****************************************************************************
@@ -178,7 +176,11 @@ if (project != rootProject) {
 
   // tests can't  be run with randomized test runner
   // it's fine as we run them as part of :buildSrc
-  test.enabled = false
+  test {
+    include "**/*Tests.class"
+    exclude "**/*IT.class"
+  }
+  // This can't be an RandomizedTestingTask because we can't yet reference it
   task integTest(type: Test) {
     // integration test requires the local testing repo for example plugin builds
     dependsOn project.rootProject.allprojects.collect {

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

@@ -770,13 +770,25 @@ class BuildPlugin implements Plugin<Project> {
     }
 
     static void applyCommonTestConfig(Project project) {
-        project.tasks.withType(RandomizedTestingTask) {
+        project.tasks.withType(RandomizedTestingTask) {task ->
             jvm "${project.runtimeJavaHome}/bin/java"
             parallelism System.getProperty('tests.jvms', project.rootProject.ext.defaultParallel)
             ifNoTests System.getProperty('tests.ifNoTests', 'fail')
             onNonEmptyWorkDirectory 'wipe'
             leaveTemporary true
 
+            if (name != "test") {
+                project.tasks.matching { it.name == "test"}.all { testTask ->
+                    task.testClassesDirs = testTask.testClassesDirs
+                    task.classpath = testTask.classpath
+                    task.shouldRunAfter testTask
+                    // no loose ends: check has to depend on all test tasks
+                    project.tasks.matching {it.name == "check"}.all {
+                        dependsOn(task)
+                    }
+                }
+            }
+
             // TODO: why are we not passing maxmemory to junit4?
             jvmArg '-Xmx' + System.getProperty('tests.heap.size', '512m')
             jvmArg '-Xms' + System.getProperty('tests.heap.size', '512m')

buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginBuildPlugin.groovy

@@ -129,7 +129,6 @@ public class PluginBuildPlugin extends BuildPlugin {
         RestIntegTestTask integTest = project.tasks.create('integTest', RestIntegTestTask.class)
         integTest.mustRunAfter(project.precommit, project.test)
         project.integTestCluster.distribution = System.getProperty('tests.distribution', 'integ-test-zip')
-        project.check.dependsOn(integTest)
     }
 
     /**

buildSrc/src/main/java/org/elasticsearch/gradle/precommit/TestingConventionsTasks.java

@@ -20,15 +20,20 @@
 
 import org.elasticsearch.gradle.tool.Boilerplate;
 import org.gradle.api.DefaultTask;
+import org.gradle.api.Task;
 import org.gradle.api.file.FileCollection;
+import org.gradle.api.file.FileTree;
 import org.gradle.api.tasks.Input;
 import org.gradle.api.tasks.OutputFile;
 import org.gradle.api.tasks.SkipWhenEmpty;
 import org.gradle.api.tasks.TaskAction;
+import org.gradle.api.tasks.testing.Test;
+import org.gradle.api.tasks.util.PatternFilterable;
 
 import java.io.File;
 import java.io.IOException;
 import java.lang.annotation.Annotation;
+import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
 import java.net.MalformedURLException;
@@ -40,8 +45,9 @@
 import java.nio.file.Path;
 import java.nio.file.StandardOpenOption;
 import java.nio.file.attribute.BasicFileAttributes;
-import java.util.ArrayList;
-import java.util.List;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Set;
 import java.util.function.Predicate;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
@@ -57,7 +63,7 @@ public class TestingConventionsTasks extends DefaultTask {
      */
     private Boolean activeTestsExists;
 
-    private List<String> testClassNames;
+    private Map<String, File> testClassNames;
 
     public TestingConventionsTasks() {
         setDescription("Tests various testing conventions");
@@ -68,56 +74,154 @@ public TestingConventionsTasks() {
     @TaskAction
     public void doCheck() throws IOException {
         activeTestsExists = false;
-        final List<String> problems;
+        final String problems;
 
         try (URLClassLoader isolatedClassLoader = new URLClassLoader(
             getTestsClassPath().getFiles().stream().map(this::fileToUrl).toArray(URL[]::new)
         )) {
-            List<? extends Class<?>> classes = getTestClassNames().stream()
-                .map(name -> loadClassWithoutInitializing(name, isolatedClassLoader))
-                .collect(Collectors.toList());
-
             Predicate<Class<?>> isStaticClass = clazz -> Modifier.isStatic(clazz.getModifiers());
             Predicate<Class<?>> isPublicClass = clazz -> Modifier.isPublic(clazz.getModifiers());
-            Predicate<Class<?>> implementsNamingConvention = clazz -> clazz.getName().endsWith(TEST_CLASS_SUFIX) ||
-                clazz.getName().endsWith(INTEG_TEST_CLASS_SUFIX);
+            Predicate<Class<?>> implementsNamingConvention = clazz ->
+                clazz.getName().endsWith(TEST_CLASS_SUFIX) ||
+                    clazz.getName().endsWith(INTEG_TEST_CLASS_SUFIX);
+
+            Map<File, ? extends Class<?>> classes = getTestClassNames().entrySet().stream()
+                .collect(Collectors.toMap(
+                    Map.Entry::getValue,
+                    entry -> loadClassWithoutInitializing(entry.getKey(), isolatedClassLoader))
+                );
+
+            FileTree allTestClassFiles = getProject().files(
+                classes.values().stream()
+                    .filter(isStaticClass.negate())
+                    .filter(isPublicClass)
+                    .filter(implementsNamingConvention)
+                    .map(clazz -> testClassNames.get(clazz.getName()))
+                    .collect(Collectors.toList())
+            ).getAsFileTree();
 
-            problems = Stream.concat(
+            final Map<String, Set<File>> classFilesPerRandomizedTestingTask = classFilesPerRandomizedTestingTask(allTestClassFiles);
+            final Map<String, Set<File>> classFilesPerGradleTestTask = classFilesPerGradleTestTask();
+
+            problems = collectProblems(
                 checkNoneExists(
                     "Test classes implemented by inner classes will not run",
-                    classes.stream()
+                    classes.values().stream()
                         .filter(isStaticClass)
                         .filter(implementsNamingConvention.or(this::seemsLikeATest))
-                ).stream(),
+                ),
                 checkNoneExists(
                     "Seem like test classes but don't match naming convention",
-                    classes.stream()
+                    classes.values().stream()
                         .filter(isStaticClass.negate())
                         .filter(isPublicClass)
                         .filter(this::seemsLikeATest)
                         .filter(implementsNamingConvention.negate())
-                ).stream()
-            ).collect(Collectors.toList());
+                ),
+                checkNoneExists(
+                    "Test classes are not included in any enabled task (" +
+                        Stream.concat(
+                            classFilesPerRandomizedTestingTask.keySet().stream(),
+                            classFilesPerGradleTestTask.keySet().stream()
+                        ).collect(Collectors.joining(",")) + ")",
+                    allTestClassFiles.getFiles().stream()
+                        .filter(testFile ->
+                            classFilesPerRandomizedTestingTask.values().stream()
+                                .anyMatch(fileSet -> fileSet.contains(testFile)) == false &&
+                                classFilesPerGradleTestTask.values().stream()
+                                    .anyMatch(fileSet -> fileSet.contains(testFile)) == false
+                        )
+                        .map(classes::get)
+                )
+            );
         }
 
         if (problems.isEmpty()) {
+            getLogger().error(problems);
+            throw new IllegalStateException("Testing conventions are not honored");
+        } else {
             getSuccessMarker().getParentFile().mkdirs();
             Files.write(getSuccessMarker().toPath(), new byte[]{}, StandardOpenOption.CREATE);
-        } else {
-            problems.forEach(getProject().getLogger()::error);
-            throw new IllegalStateException("Testing conventions are not honored");
+        }
+    }
+
+
+    private String collectProblems(String... problems) {
+        return Stream.of(problems)
+            .map(String::trim)
+            .filter(String::isEmpty)
+            .map(each -> each + "\n")
+            .collect(Collectors.joining());
+    }
+
+
+    @Input
+    public Map<String, Set<File>> classFilesPerRandomizedTestingTask(FileTree testClassFiles) {
+        return Stream.concat(
+            getProject().getTasks().withType(getRandomizedTestingTask()).stream(),
+            // Look at sub-projects too. As sometimes tests are implemented in parent but ran in sub-projects against
+            // different configurations
+            getProject().getSubprojects().stream().flatMap(subproject ->
+                subproject.getTasks().withType(getRandomizedTestingTask()).stream()
+            )
+        )
+            .filter(Task::getEnabled)
+            .collect(Collectors.toMap(
+                Task::getPath,
+                task -> testClassFiles.matching(getRandomizedTestingPatternSet(task)).getFiles()
+            ));
+    }
+
+    @Input
+    public Map<String, Set<File>> classFilesPerGradleTestTask() {
+        return Stream.concat(
+            getProject().getTasks().withType(Test.class).stream(),
+            getProject().getSubprojects().stream().flatMap(subproject ->
+                subproject.getTasks().withType(Test.class).stream()
+            )
+        )
+            .filter(Task::getEnabled)
+            .collect(Collectors.toMap(
+                Task::getPath,
+                task -> task.getCandidateClassFiles().getFiles()
+            ));
+    }
+
+    @SuppressWarnings("unchecked")
+    private PatternFilterable getRandomizedTestingPatternSet(Task task) {
+        try {
+            if (
+                getRandomizedTestingTask().isAssignableFrom(task.getClass()) == false
+            ) {
+                throw new IllegalStateException("Expected " + task + " to be RandomizedTestingTask or Test but it was " + task.getClass());
+            }
+            Method getPatternSet = task.getClass().getMethod("getPatternSet");
+            return (PatternFilterable) getPatternSet.invoke(task);
+        } catch (NoSuchMethodException e) {
+            throw new IllegalStateException("Expecte task to have a `patternSet` " + task, e);
+        } catch (IllegalAccessException | InvocationTargetException e) {
+            throw new IllegalStateException("Failed to get pattern set from task" + task, e);
+        }
+    }
+
+    @SuppressWarnings("unchecked")
+    private Class<? extends Task> getRandomizedTestingTask() {
+        try {
+            return (Class<? extends Task>) Class.forName("com.carrotsearch.gradle.junit4.RandomizedTestingTask");
+        } catch (ClassNotFoundException | ClassCastException e) {
+            throw new IllegalStateException("Failed to load randomized testing class", e);
         }
     }
 
     @Input
     @SkipWhenEmpty
-    public List<String> getTestClassNames() {
+    public Map<String, File> getTestClassNames() {
         if (testClassNames == null) {
             testClassNames = Boilerplate.getJavaSourceSets(getProject()).getByName("test").getOutput().getClassesDirs()
                 .getFiles().stream()
                 .filter(File::exists)
-                .flatMap(testRoot -> walkPathAndLoadClasses(testRoot).stream())
-                .collect(Collectors.toList());
+                .flatMap(testRoot -> walkPathAndLoadClasses(testRoot).entrySet().stream())
+                .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
         }
         return testClassNames;
     }
@@ -127,16 +231,15 @@ public File getSuccessMarker() {
         return new File(getProject().getBuildDir(), "markers/" + getName());
     }
 
-    private List<String> checkNoneExists(String message, Stream<? extends Class<?>> stream) {
-        List<String> problems = new ArrayList<>();
-        List<Class<?>> entries = stream.collect(Collectors.toList());
-        if (entries.isEmpty() == false) {
-            problems.add(message + ":");
-            entries.stream()
-                .map(each -> "  * " + each.getName())
-            .forEach(problems::add);
+    private String checkNoneExists(String message, Stream<? extends Class<?>> stream) {
+        String problem = stream
+            .map(each -> "  * " + each.getName())
+            .collect(Collectors.joining("\n"));
+        if (problem.isEmpty() == false) {
+            return message + ":\n" + problem;
+        } else{
+            return "";
         }
-        return problems;
     }
 
     private boolean seemsLikeATest(Class<?> clazz) {
@@ -197,8 +300,8 @@ private FileCollection getTestsClassPath() {
         );
     }
 
-    private List<String> walkPathAndLoadClasses(File testRoot) {
-        List<String> classes = new ArrayList<>();
+    private Map<String, File> walkPathAndLoadClasses(File testRoot) {
+        Map<String, File> classes = new HashMap<>();
         try {
             Files.walkFileTree(testRoot.toPath(), new FileVisitor<Path>() {
                 private String packageName;
@@ -227,7 +330,7 @@ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IO
                     String filename = file.getFileName().toString();
                     if (filename.endsWith(".class")) {
                         String className = filename.substring(0, filename.length() - ".class".length());
-                        classes.add(packageName + className);
+                        classes.put(packageName + className, file.toFile());
                     }
                     return FileVisitResult.CONTINUE;
                 }

buildSrc/src/test/java/org/elasticsearch/gradle/precommit/FilePermissionsTaskTests.java

@@ -19,7 +19,6 @@
 package org.elasticsearch.gradle.precommit;
 
 import java.io.File;
-import java.io.IOException;
 import java.nio.charset.Charset;
 import java.nio.file.Files;
 import java.util.List;
@@ -32,12 +31,8 @@
 import org.gradle.api.plugins.JavaPlugin;
 import org.gradle.testfixtures.ProjectBuilder;
 import org.junit.Assert;
-import org.junit.Rule;
-import org.junit.rules.TemporaryFolder;
 
 public class FilePermissionsTaskTests extends GradleUnitTestCase {
-    @Rule
-    public TemporaryFolder temporaryFolder = new TemporaryFolder();
 
     public void testCheckPermissionsWhenAnExecutableFileExists() throws Exception {
         RandomizedTest.assumeFalse("Functionality is Unix specific", Os.isFamily(Os.FAMILY_WINDOWS));
@@ -93,16 +88,16 @@ public void testCheckPermissionsWhenNoExecutableFileExists() throws Exception {
         assertEquals("done", result.get(0));
 
         file.delete();
-
     }
 
-    private Project createProject() throws IOException {
-        Project project = ProjectBuilder.builder().withProjectDir(temporaryFolder.newFolder()).build();
+    private Project createProject() {
+        Project project = ProjectBuilder.builder().build();
         project.getPlugins().apply(JavaPlugin.class);
         return project;
     }
 
     private FilePermissionsTask createTask(Project project) {
         return project.getTasks().create("filePermissionsTask", FilePermissionsTask.class);
     }
+
 }

plugins/repository-hdfs/build.gradle

@@ -301,6 +301,7 @@ if (secureFixtureSupported) {
   // Security tests unsupported. Don't run these tests.
   integTestSecure.enabled = false
   integTestSecureHa.enabled = false
+  testingConventions.enabled = false
 }
 
 thirdPartyAudit.excludes = [

plugins/repository-s3/build.gradle

@@ -73,10 +73,7 @@ task testRepositoryCreds(type: RandomizedTestingTask) {
   include '**/RepositoryCredentialsTests.class'
   include '**/S3BlobStoreRepositoryTests.class'
   systemProperty 'es.allow_insecure_settings', 'true'
-  classpath = tasks.test.classpath
-  testClassesDirs = tasks.test.testClassesDirs
 }
-project.check.dependsOn(testRepositoryCreds)
 
 test {
   // these are tested explicitly in separate test tasks

server/build.gradle

@@ -320,10 +320,6 @@ if (isEclipse == false || project.path == ":server-tests") {
                  group: JavaBasePlugin.VERIFICATION_GROUP,
                  description: 'Multi-node tests',
                  dependsOn: test.dependsOn) {
-    classpath = project.test.classpath
-    testClassesDirs = project.test.testClassesDirs
     include '**/*IT.class'
   }
-  check.dependsOn integTest
-  integTest.mustRunAfter test
 }