Move NamingConventionsCheck into buildSrc

This will let things that don't depend on :test:framework like the
client use it.

Also skip initializing the classes we check because we don't care
about their initialization behavior because we're not executing them.
This makes the naming conventions check pretty close to instant
from a "human eye" perspective.

build.gradle

@@ -160,6 +160,7 @@ subprojects {
     them as external dependencies so the build plugin that we use can be used
     to build elasticsearch plugins outside of the elasticsearch source tree. */
   ext.projectSubstitutions = [
+    "org.elasticsearch.gradle:build-tools:${version}": ':build-tools',
     "org.elasticsearch:rest-api-spec:${version}": ':rest-api-spec',
     "org.elasticsearch:elasticsearch:${version}": ':core',
     "org.elasticsearch.test:framework:${version}": ':test:framework',

buildSrc/build.gradle

@@ -103,6 +103,7 @@ if (project == rootProject) {
       url "https://oss.sonatype.org/content/repositories/snapshots/"
     }
   }
+  test.exclude 'org/elasticsearch/test/NamingConventionsCheckBadClasses*'
 }
 
 /*****************************************************************************
@@ -122,8 +123,8 @@ if (project != rootProject) {
   // build-tools is not ready for primetime with these...
   dependencyLicenses.enabled = false
   forbiddenApisMain.enabled = false
+  forbiddenApisTest.enabled = false
   jarHell.enabled = false
-  loggerUsageCheck.enabled = false
   thirdPartyAudit.enabled = false
 
   // test for elasticsearch.build tries to run with ES...
@@ -137,4 +138,9 @@ if (project != rootProject) {
     // the file that actually defines nocommit
     exclude '**/ForbiddenPatternsTask.groovy'
   }
+
+  namingConventions {
+    testClass = 'org.elasticsearch.test.NamingConventionsCheckBadClasses$UnitTestCase'
+    integTestClass = 'org.elasticsearch.test.NamingConventionsCheckBadClasses$IntegTestCase'
+  }
 }

buildSrc/src/main/groovy/org/elasticsearch/gradle/precommit/NamingConventionsTask.groovy

@@ -21,6 +21,7 @@ package org.elasticsearch.gradle.precommit
 
 import org.elasticsearch.gradle.LoggedExec
 import org.elasticsearch.gradle.VersionProperties
+import org.gradle.api.artifacts.Dependency
 import org.gradle.api.file.FileCollection
 import org.gradle.api.tasks.Input
 import org.gradle.api.tasks.InputFiles
@@ -57,8 +58,27 @@ public class NamingConventionsTask extends LoggedExec {
     @Input
     boolean skipIntegTestInDisguise = false
 
+    /**
+     * Superclass for all tests.
+     */
+    @Input
+    String testClass = 'org.apache.lucene.util.LuceneTestCase'
+
+    /**
+     * Superclass for all integration tests.
+     */
+    @Input
+    String integTestClass = 'org.elasticsearch.test.ESIntegTestCase'
+
     public NamingConventionsTask() {
-        dependsOn(classpath)
+        // Extra classpath contains the actual test
+        project.configurations.create('namingConventions')
+        Dependency buildToolsDep = project.dependencies.add('namingConventions',
+                "org.elasticsearch.gradle:build-tools:${VersionProperties.elasticsearch}")
+        buildToolsDep.transitive = false // We don't need gradle in the classpath. It conflicts.
+        FileCollection extraClasspath = project.configurations.namingConventions
+        dependsOn(extraClasspath)
+
         description = "Runs NamingConventionsCheck on ${classpath}"
         executable = new File(project.javaHome, 'bin/java')
         onlyIf { project.sourceSets.test.output.classesDir.exists() }
@@ -69,7 +89,8 @@ public class NamingConventionsTask extends LoggedExec {
         project.afterEvaluate {
             doFirst {
                 args('-Djna.nosys=true')
-                args('-cp', classpath.asPath, 'org.elasticsearch.test.NamingConventionsCheck')
+                args('-cp', (classpath + extraClasspath).asPath, 'org.elasticsearch.test.NamingConventionsCheck')
+                args(testClass, integTestClass)
                 if (skipIntegTestInDisguise) {
                     args('--skip-integ-tests-in-disguise')
                 }
@@ -79,7 +100,7 @@ public class NamingConventionsTask extends LoggedExec {
                  * process of ignoring them lets us validate that they were found so this ignore parameter acts
                  * as the test for the NamingConventionsCheck.
                  */
-                if (':test:framework'.equals(project.path)) {
+                if (':build-tools'.equals(project.path)) {
                     args('--self-test')
                 }
                 args('--', project.sourceSets.test.output.classesDir.absolutePath)

buildSrc/src/main/groovy/org/elasticsearch/gradle/precommit/PrecommitTasks.groovy

@@ -34,7 +34,6 @@ class PrecommitTasks {
             configureForbiddenApis(project),
             configureCheckstyle(project),
             configureNamingConventions(project),
-            configureLoggerUsage(project),
             project.tasks.create('forbiddenPatterns', ForbiddenPatternsTask.class),
             project.tasks.create('licenseHeaders', LicenseHeadersTask.class),
             project.tasks.create('jarHell', JarHellTask.class),
@@ -49,6 +48,20 @@ class PrecommitTasks {
             UpdateShasTask updateShas = project.tasks.create('updateShas', UpdateShasTask.class)
             updateShas.parentTask = dependencyLicenses
         }
+        if (project.path != ':build-tools') {
+            /*
+             * Sadly, build-tools can't have logger-usage-check because that
+             * would create a circular project dependency between build-tools
+             * (which provides NamingConventionsCheck) and :test:logger-usage
+             * which provides the logger usage check. Since the build tools
+             * don't use the logger usage check because they don't have any
+             * of Elaticsearch's loggers and :test:logger-usage actually does
+             * use the NamingConventionsCheck we break the circular dependency
+             * here.
+             */
+            precommitTasks.add(configureLoggerUsage(project))
+        }
+
 
         Map<String, Object> precommitOptions = [
             name: 'precommit',

buildSrc/src/main/java/org/elasticsearch/test/NamingConventionsCheck.java

@@ -25,14 +25,11 @@ import java.nio.file.FileVisitResult;
 import java.nio.file.FileVisitor;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.nio.file.Paths;
 import java.nio.file.attribute.BasicFileAttributes;
 import java.util.HashSet;
 import java.util.Set;
 
-import org.apache.lucene.util.LuceneTestCase;
-import org.elasticsearch.common.SuppressForbidden;
-import org.elasticsearch.common.io.PathUtils;
-
 /**
  * Checks that all tests in a directory are named according to our naming conventions. This is important because tests that do not follow
  * our conventions aren't run by gradle. This was once a glorious unit test but now that Elasticsearch is a multi-module project it must be
@@ -46,11 +43,13 @@ import org.elasticsearch.common.io.PathUtils;
  * {@code --self-test} that is only run in the test:framework project.
  */
 public class NamingConventionsCheck {
-    public static void main(String[] args) throws IOException, ClassNotFoundException {
+    public static void main(String[] args) throws IOException {
-        NamingConventionsCheck check = new NamingConventionsCheck();
+        int i = 0;
+        NamingConventionsCheck check = new NamingConventionsCheck(
+                loadClassWithoutInitializing(args[i++]),
+                loadClassWithoutInitializing(args[i++]));
         boolean skipIntegTestsInDisguise = false;
         boolean selfTest = false;
-        int i = 0;
         while (true) {
             switch (args[i]) {
             case "--skip-integ-tests-in-disguise":
@@ -69,7 +68,7 @@ public class NamingConventionsCheck {
             }
             break;
         }
-        check.check(PathUtils.get(args[i]));
+        check.check(Paths.get(args[i]));
 
         if (selfTest) {
             assertViolation("WrongName", check.missingSuffix);
@@ -82,14 +81,12 @@ public class NamingConventionsCheck {
         }
 
         // Now we should have no violations
-        assertNoViolations("Not all subclasses of " + ESTestCase.class.getSimpleName()
+        assertNoViolations("Not all subclasses of " + check.testClass.getSimpleName()
                 + " match the naming convention. Concrete classes must end with [Tests]", check.missingSuffix);
         assertNoViolations("Classes ending with [Tests] are abstract or interfaces", check.notRunnable);
         assertNoViolations("Found inner classes that are tests, which are excluded from the test runner", check.innerClasses);
-        String classesToSubclass = String.join(",", ESTestCase.class.getSimpleName(), ESTestCase.class.getSimpleName(),
+        assertNoViolations("Pure Unit-Test found must subclass [" + check.testClass.getSimpleName() + "]", check.pureUnitTest);
-                ESTokenStreamTestCase.class.getSimpleName(), LuceneTestCase.class.getSimpleName());
+        assertNoViolations("Classes ending with [Tests] must subclass [" + check.testClass.getSimpleName() + "]", check.notImplementing);
-        assertNoViolations("Pure Unit-Test found must subclass one of [" + classesToSubclass + "]", check.pureUnitTest);
-        assertNoViolations("Classes ending with [Tests] must subclass [" + classesToSubclass + "]", check.notImplementing);
         if (!skipIntegTestsInDisguise) {
             assertNoViolations("Subclasses of ESIntegTestCase should end with IT as they are integration tests",
                     check.integTestsInDisguise);
@@ -103,6 +100,14 @@ public class NamingConventionsCheck {
     private final Set<Class<?>> notRunnable = new HashSet<>();
     private final Set<Class<?>> innerClasses = new HashSet<>();
 
+    private final Class<?> testClass;
+    private final Class<?> integTestClass;
+
+    public NamingConventionsCheck(Class<?> testClass, Class<?> integTestClass) {
+        this.testClass = testClass;
+        this.integTestClass = integTestClass;
+    }
+
     public void check(Path rootPath) throws IOException {
         Files.walkFileTree(rootPath, new FileVisitor<Path>() {
             /**
@@ -136,9 +141,9 @@ public class NamingConventionsCheck {
                 String filename = file.getFileName().toString();
                 if (filename.endsWith(".class")) {
                     String className = filename.substring(0, filename.length() - ".class".length());
-                    Class<?> clazz = loadClass(className);
+                    Class<?> clazz = loadClassWithoutInitializing(packageName + className);
                     if (clazz.getName().endsWith("Tests")) {
-                        if (ESIntegTestCase.class.isAssignableFrom(clazz)) {
+                        if (integTestClass.isAssignableFrom(clazz)) {
                             integTestsInDisguise.add(clazz);
                         }
                         if (Modifier.isAbstract(clazz.getModifiers()) || Modifier.isInterface(clazz.getModifiers())) {
@@ -164,15 +169,7 @@ public class NamingConventionsCheck {
             }
 
             private boolean isTestCase(Class<?> clazz) {
-                return LuceneTestCase.class.isAssignableFrom(clazz);
+                return testClass.isAssignableFrom(clazz);
-            }
-
-            private Class<?> loadClass(String className) {
-                try {
-                    return Thread.currentThread().getContextClassLoader().loadClass(packageName + className);
-                } catch (ClassNotFoundException e) {
-                    throw new RuntimeException(e);
-                }
             }
 
             @Override
@@ -186,7 +183,6 @@ public class NamingConventionsCheck {
      * Fail the process if there are any violations in the set. Named to look like a junit assertion even though it isn't because it is
      * similar enough.
      */
-    @SuppressForbidden(reason = "System.err/System.exit")
     private static void assertNoViolations(String message, Set<Class<?>> set) {
         if (false == set.isEmpty()) {
             System.err.println(message + ":");
@@ -201,10 +197,9 @@ public class NamingConventionsCheck {
      * Fail the process if we didn't detect a particular violation. Named to look like a junit assertion even though it isn't because it is
      * similar enough.
      */
-    @SuppressForbidden(reason = "System.err/System.exit")
+    private static void assertViolation(String className, Set<Class<?>> set) {
-    private static void assertViolation(String className, Set<Class<?>> set) throws ClassNotFoundException {
+        className = "org.elasticsearch.test.NamingConventionsCheckBadClasses$" + className;
-        className = "org.elasticsearch.test.test.NamingConventionsCheckBadClasses$" + className;
+        if (false == set.remove(loadClassWithoutInitializing(className))) {
-        if (false == set.remove(Class.forName(className))) {
             System.err.println("Error in NamingConventionsCheck! Expected [" + className + "] to be a violation but wasn't.");
             System.exit(1);
         }
@@ -213,9 +208,20 @@ public class NamingConventionsCheck {
     /**
      * Fail the process with the provided message.
      */
-    @SuppressForbidden(reason = "System.err/System.exit")
     private static void fail(String reason) {
         System.err.println(reason);
         System.exit(1);
     }
+
+    static Class<?> loadClassWithoutInitializing(String name) {
+        try {
+            return Class.forName(name,
+                    // Don't initialize the class to save time. Not needed for this test and this doesn't share a VM with any other tests.
+                    false,
+                    // Use our classloader rather than the bootstrap class loader.
+                    NamingConventionsCheck.class.getClassLoader());
+        } catch (ClassNotFoundException e) {
+            throw new RuntimeException(e);
+        }
+    }
 }

buildSrc/src/test/java/org/elasticsearch/test/NamingConventionsCheckBadClasses.java

@@ -17,9 +17,7 @@
  * under the License.
  */
 
-package org.elasticsearch.test.test;
+package org.elasticsearch.test;
-
-import org.elasticsearch.test.ESTestCase;
 
 import junit.framework.TestCase;
 
@@ -30,21 +28,35 @@ public class NamingConventionsCheckBadClasses {
     public static final class NotImplementingTests {
     }
 
-    public static final class WrongName extends ESTestCase {
+    public static final class WrongName extends UnitTestCase {
+        /*
+         * Dummy test so the tests pass. We do this *and* skip the tests so anyone who jumps back to a branch without these tests can still
+         * compile without a failure. That is because clean doesn't actually clean these....
+         */
+        public void testDummy() {}
     }
 
-    public static abstract class DummyAbstractTests extends ESTestCase {
+    public static abstract class DummyAbstractTests extends UnitTestCase {
     }
 
     public interface DummyInterfaceTests {
     }
 
-    public static final class InnerTests extends ESTestCase {
+    public static final class InnerTests extends UnitTestCase {
+        public void testDummy() {}
     }
 
-    public static final class WrongNameTheSecond extends ESTestCase {
+    public static final class WrongNameTheSecond extends UnitTestCase {
+        public void testDummy() {}
     }
 
     public static final class PlainUnit extends TestCase {
+        public void testDummy() {}
+    }
+
+    public abstract static class UnitTestCase extends TestCase {
+    }
+
+    public abstract static class IntegTestCase extends UnitTestCase {
     }
 }