From fc97e25b564011bdb1d272a15e5b6b1f22c0c633 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Tue, 25 Apr 2017 21:11:47 -0400 Subject: [PATCH] Add task to look for tests in src/main (#24298) Creates a new task `namingConventionsMain`, that runs on the `buildSrc` and `test:framework` projects and fails the build if any of the classes in the main artifacts are named like tests or are non-abstract subclasses of ESTestCase. It also fixes the three tests that would cause it to fail. --- buildSrc/build.gradle | 7 + .../precommit/NamingConventionsTask.groovy | 50 ++-- .../test/NamingConventionsCheck.java | 232 ++++++++++++------ .../test/NamingConventionsCheckInMainIT.java | 26 ++ .../NamingConventionsCheckInMainTests.java | 26 ++ test/framework/build.gradle | 5 + .../AnalysisFactoryTestCase.java | 2 +- .../disruption/LongGCDisruptionTests.java} | 2 +- .../test/disruption/NetworkDisruptionIT.java | 0 .../disruption/NetworkDisruptionTests.java | 0 10 files changed, 255 insertions(+), 95 deletions(-) create mode 100644 buildSrc/src/main/java/org/elasticsearch/test/NamingConventionsCheckInMainIT.java create mode 100644 buildSrc/src/main/java/org/elasticsearch/test/NamingConventionsCheckInMainTests.java rename test/framework/src/{main/java/org/elasticsearch/test/disruption/LongGCDisruptionTest.java => test/java/org/elasticsearch/test/disruption/LongGCDisruptionTests.java} (99%) rename test/framework/src/{main => test}/java/org/elasticsearch/test/disruption/NetworkDisruptionIT.java (100%) rename test/framework/src/{main => test}/java/org/elasticsearch/test/disruption/NetworkDisruptionTests.java (100%) diff --git a/buildSrc/build.gradle b/buildSrc/build.gradle index 6536c77e587..0839b8a22f8 100644 --- a/buildSrc/build.gradle +++ b/buildSrc/build.gradle @@ -156,4 +156,11 @@ if (project != rootProject) { testClass = 'org.elasticsearch.test.NamingConventionsCheckBadClasses$UnitTestCase' integTestClass = 'org.elasticsearch.test.NamingConventionsCheckBadClasses$IntegTestCase' } + + task namingConventionsMain(type: org.elasticsearch.gradle.precommit.NamingConventionsTask) { + checkForTestsInMain = true + testClass = namingConventions.testClass + integTestClass = namingConventions.integTestClass + } + precommit.dependsOn namingConventionsMain } diff --git a/buildSrc/src/main/groovy/org/elasticsearch/gradle/precommit/NamingConventionsTask.groovy b/buildSrc/src/main/groovy/org/elasticsearch/gradle/precommit/NamingConventionsTask.groovy index 52de7dac2d5..2711a0e38f2 100644 --- a/buildSrc/src/main/groovy/org/elasticsearch/gradle/precommit/NamingConventionsTask.groovy +++ b/buildSrc/src/main/groovy/org/elasticsearch/gradle/precommit/NamingConventionsTask.groovy @@ -38,17 +38,7 @@ public class NamingConventionsTask extends LoggedExec { * inputs (ie the jars/class files). */ @OutputFile - File successMarker = new File(project.buildDir, 'markers/namingConventions') - - /** - * The classpath to run the naming conventions checks against. Must contain the files in the test - * output directory and everything required to load those classes. - * - * We don't declare the actual test files as a dependency or input because if they change then - * this will change. - */ - @InputFiles - FileCollection classpath = project.sourceSets.test.runtimeClasspath + File successMarker = new File(project.buildDir, "markers/${this.name}") /** * Should we skip the integ tests in disguise tests? Defaults to true because only core names its @@ -69,18 +59,35 @@ public class NamingConventionsTask extends LoggedExec { @Input String integTestClass = 'org.elasticsearch.test.ESIntegTestCase' + /** + * Should the test also check the main classpath for test classes instead of + * doing the usual checks to the test classpath. + */ + @Input + boolean checkForTestsInMain = false; + public NamingConventionsTask() { // 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. + if (false == project.configurations.names.contains('namingConventions')) { + 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}" + FileCollection classpath = project.sourceSets.test.runtimeClasspath + inputs.files(classpath) + description = "Tests that test classes aren't misnamed or misplaced" executable = new File(project.javaHome, 'bin/java') - onlyIf { project.sourceSets.test.output.classesDir.exists() } + if (false == checkForTestsInMain) { + /* This task is created by default for all subprojects with this + * setting and there is no point in running it if the files don't + * exist. */ + onlyIf { project.sourceSets.test.output.classesDir.exists() } + } + /* * We build the arguments in a funny afterEvaluate/doFirst closure so that we can wait for the classpath to be * ready for us. Strangely neither one on their own are good enough. @@ -104,7 +111,14 @@ public class NamingConventionsTask extends LoggedExec { if (':build-tools'.equals(project.path)) { args('--self-test') } - args('--', project.sourceSets.test.output.classesDir.absolutePath) + if (checkForTestsInMain) { + args('--main') + args('--') + args(project.sourceSets.main.output.classesDir.absolutePath) + } else { + args('--') + args(project.sourceSets.test.output.classesDir.absolutePath) + } } } doLast { successMarker.setText("", 'UTF-8') } diff --git a/buildSrc/src/main/java/org/elasticsearch/test/NamingConventionsCheck.java b/buildSrc/src/main/java/org/elasticsearch/test/NamingConventionsCheck.java index cbfa31d1aaf..9bd14675d34 100644 --- a/buildSrc/src/main/java/org/elasticsearch/test/NamingConventionsCheck.java +++ b/buildSrc/src/main/java/org/elasticsearch/test/NamingConventionsCheck.java @@ -28,6 +28,7 @@ import java.nio.file.Path; import java.nio.file.Paths; import java.nio.file.attribute.BasicFileAttributes; import java.util.HashSet; +import java.util.Objects; import java.util.Set; /** @@ -49,6 +50,7 @@ public class NamingConventionsCheck { Path rootPath = null; boolean skipIntegTestsInDisguise = false; boolean selfTest = false; + boolean checkMainClasses = false; for (int i = 0; i < args.length; i++) { String arg = args[i]; switch (arg) { @@ -64,6 +66,9 @@ public class NamingConventionsCheck { case "--self-test": selfTest = true; break; + case "--main": + checkMainClasses = true; + break; case "--": rootPath = Paths.get(args[++i]); break; @@ -73,28 +78,43 @@ public class NamingConventionsCheck { } NamingConventionsCheck check = new NamingConventionsCheck(testClass, integTestClass); - check.check(rootPath, skipIntegTestsInDisguise); + if (checkMainClasses) { + check.checkMain(rootPath); + } else { + check.checkTests(rootPath, skipIntegTestsInDisguise); + } if (selfTest) { - assertViolation("WrongName", check.missingSuffix); - assertViolation("WrongNameTheSecond", check.missingSuffix); - assertViolation("DummyAbstractTests", check.notRunnable); - assertViolation("DummyInterfaceTests", check.notRunnable); - assertViolation("InnerTests", check.innerClasses); - assertViolation("NotImplementingTests", check.notImplementing); - assertViolation("PlainUnit", check.pureUnitTest); + if (checkMainClasses) { + assertViolation(NamingConventionsCheckInMainTests.class.getName(), check.testsInMain); + assertViolation(NamingConventionsCheckInMainIT.class.getName(), check.testsInMain); + } else { + assertViolation("WrongName", check.missingSuffix); + assertViolation("WrongNameTheSecond", check.missingSuffix); + assertViolation("DummyAbstractTests", check.notRunnable); + assertViolation("DummyInterfaceTests", check.notRunnable); + assertViolation("InnerTests", check.innerClasses); + assertViolation("NotImplementingTests", check.notImplementing); + assertViolation("PlainUnit", check.pureUnitTest); + } } // Now we should have no violations - assertNoViolations("Not all subclasses of " + check.testClass.getSimpleName() - + " match the naming convention. Concrete classes must end with [Tests]", check.missingSuffix); + 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); assertNoViolations("Pure Unit-Test found must subclass [" + check.testClass.getSimpleName() + "]", check.pureUnitTest); assertNoViolations("Classes ending with [Tests] must subclass [" + check.testClass.getSimpleName() + "]", check.notImplementing); + assertNoViolations( + "Classes ending with [Tests] or [IT] or extending [" + check.testClass.getSimpleName() + "] must be in src/test/java", + check.testsInMain); if (skipIntegTestsInDisguise == false) { - assertNoViolations("Subclasses of " + check.integTestClass.getSimpleName() + - " should end with IT as they are integration tests", check.integTestsInDisguise); + assertNoViolations( + "Subclasses of " + check.integTestClass.getSimpleName() + " should end with IT as they are integration tests", + check.integTestsInDisguise); } } @@ -104,86 +124,78 @@ public class NamingConventionsCheck { private final Set> integTestsInDisguise = new HashSet<>(); private final Set> notRunnable = new HashSet<>(); private final Set> innerClasses = new HashSet<>(); + private final Set> testsInMain = new HashSet<>(); private final Class testClass; private final Class integTestClass; public NamingConventionsCheck(Class testClass, Class integTestClass) { - this.testClass = testClass; + this.testClass = Objects.requireNonNull(testClass, "--test-class is required"); this.integTestClass = integTestClass; } - public void check(Path rootPath, boolean skipTestsInDisguised) throws IOException { - Files.walkFileTree(rootPath, new FileVisitor() { - /** - * The package name of the directory we are currently visiting. Kept as a string rather than something fancy because we load - * just about every class and doing so requires building a string out of it anyway. At least this way we don't need to build the - * first part of the string over and over and over again. - */ - private String packageName; - + public void checkTests(Path rootPath, boolean skipTestsInDisguised) throws IOException { + Files.walkFileTree(rootPath, new TestClassVisitor() { @Override - public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) throws IOException { - // First we visit the root directory - if (packageName == null) { - // And it package is empty string regardless of the directory name - packageName = ""; - } else { - packageName += dir.getFileName() + "."; + protected void visitTestClass(Class clazz) { + if (skipTestsInDisguised == false && integTestClass.isAssignableFrom(clazz)) { + integTestsInDisguise.add(clazz); } - return FileVisitResult.CONTINUE; - } - - @Override - public FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOException { - // Go up one package by jumping back to the second to last '.' - packageName = packageName.substring(0, 1 + packageName.lastIndexOf('.', packageName.length() - 2)); - return FileVisitResult.CONTINUE; - } - - @Override - public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException { - String filename = file.getFileName().toString(); - if (filename.endsWith(".class")) { - String className = filename.substring(0, filename.length() - ".class".length()); - Class clazz = loadClassWithoutInitializing(packageName + className); - if (clazz.getName().endsWith("Tests")) { - if (skipTestsInDisguised == false && integTestClass.isAssignableFrom(clazz)) { - integTestsInDisguise.add(clazz); - } - if (Modifier.isAbstract(clazz.getModifiers()) || Modifier.isInterface(clazz.getModifiers())) { - notRunnable.add(clazz); - } else if (isTestCase(clazz) == false) { - notImplementing.add(clazz); - } else if (Modifier.isStatic(clazz.getModifiers())) { - innerClasses.add(clazz); - } - } else if (clazz.getName().endsWith("IT")) { - if (isTestCase(clazz) == false) { - notImplementing.add(clazz); - } - } else if (Modifier.isAbstract(clazz.getModifiers()) == false && Modifier.isInterface(clazz.getModifiers()) == false) { - if (isTestCase(clazz)) { - missingSuffix.add(clazz); - } else if (junit.framework.Test.class.isAssignableFrom(clazz)) { - pureUnitTest.add(clazz); - } - } + if (Modifier.isAbstract(clazz.getModifiers()) || Modifier.isInterface(clazz.getModifiers())) { + notRunnable.add(clazz); + } else if (isTestCase(clazz) == false) { + notImplementing.add(clazz); + } else if (Modifier.isStatic(clazz.getModifiers())) { + innerClasses.add(clazz); } - return FileVisitResult.CONTINUE; - } - - private boolean isTestCase(Class clazz) { - return testClass.isAssignableFrom(clazz); } @Override - public FileVisitResult visitFileFailed(Path file, IOException exc) throws IOException { - throw exc; + protected void visitIntegrationTestClass(Class clazz) { + if (isTestCase(clazz) == false) { + notImplementing.add(clazz); + } + } + + @Override + protected void visitOtherClass(Class clazz) { + if (Modifier.isAbstract(clazz.getModifiers()) || Modifier.isInterface(clazz.getModifiers())) { + return; + } + if (isTestCase(clazz)) { + missingSuffix.add(clazz); + } else if (junit.framework.Test.class.isAssignableFrom(clazz)) { + pureUnitTest.add(clazz); + } } }); } + public void checkMain(Path rootPath) throws IOException { + Files.walkFileTree(rootPath, new TestClassVisitor() { + @Override + protected void visitTestClass(Class clazz) { + testsInMain.add(clazz); + } + + @Override + protected void visitIntegrationTestClass(Class clazz) { + testsInMain.add(clazz); + } + + @Override + protected void visitOtherClass(Class clazz) { + if (Modifier.isAbstract(clazz.getModifiers()) || Modifier.isInterface(clazz.getModifiers())) { + return; + } + if (isTestCase(clazz)) { + testsInMain.add(clazz); + } + } + }); + + } + /** * 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. @@ -203,7 +215,7 @@ public class NamingConventionsCheck { * similar enough. */ private static void assertViolation(String className, Set> set) { - className = "org.elasticsearch.test.NamingConventionsCheckBadClasses$" + className; + className = className.startsWith("org") ? className : "org.elasticsearch.test.NamingConventionsCheckBadClasses$" + className; if (false == set.remove(loadClassWithoutInitializing(className))) { System.err.println("Error in NamingConventionsCheck! Expected [" + className + "] to be a violation but wasn't."); System.exit(1); @@ -229,4 +241,74 @@ public class NamingConventionsCheck { throw new RuntimeException(e); } } + + abstract class TestClassVisitor implements FileVisitor { + /** + * The package name of the directory we are currently visiting. Kept as a string rather than something fancy because we load + * just about every class and doing so requires building a string out of it anyway. At least this way we don't need to build the + * first part of the string over and over and over again. + */ + private String packageName; + + /** + * Visit classes named like a test. + */ + protected abstract void visitTestClass(Class clazz); + /** + * Visit classes named like an integration test. + */ + protected abstract void visitIntegrationTestClass(Class clazz); + /** + * Visit classes not named like a test at all. + */ + protected abstract void visitOtherClass(Class clazz); + + @Override + public final FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) throws IOException { + // First we visit the root directory + if (packageName == null) { + // And it package is empty string regardless of the directory name + packageName = ""; + } else { + packageName += dir.getFileName() + "."; + } + return FileVisitResult.CONTINUE; + } + + @Override + public final FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOException { + // Go up one package by jumping back to the second to last '.' + packageName = packageName.substring(0, 1 + packageName.lastIndexOf('.', packageName.length() - 2)); + return FileVisitResult.CONTINUE; + } + + @Override + public final FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException { + String filename = file.getFileName().toString(); + if (filename.endsWith(".class")) { + String className = filename.substring(0, filename.length() - ".class".length()); + Class clazz = loadClassWithoutInitializing(packageName + className); + if (clazz.getName().endsWith("Tests")) { + visitTestClass(clazz); + } else if (clazz.getName().endsWith("IT")) { + visitIntegrationTestClass(clazz); + } else { + visitOtherClass(clazz); + } + } + return FileVisitResult.CONTINUE; + } + + /** + * Is this class a test case? + */ + protected boolean isTestCase(Class clazz) { + return testClass.isAssignableFrom(clazz); + } + + @Override + public final FileVisitResult visitFileFailed(Path file, IOException exc) throws IOException { + throw exc; + } + } } diff --git a/buildSrc/src/main/java/org/elasticsearch/test/NamingConventionsCheckInMainIT.java b/buildSrc/src/main/java/org/elasticsearch/test/NamingConventionsCheckInMainIT.java new file mode 100644 index 00000000000..46adc7f065b --- /dev/null +++ b/buildSrc/src/main/java/org/elasticsearch/test/NamingConventionsCheckInMainIT.java @@ -0,0 +1,26 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.test; + +/** + * This class should fail the naming conventions self test. + */ +public class NamingConventionsCheckInMainIT { +} diff --git a/buildSrc/src/main/java/org/elasticsearch/test/NamingConventionsCheckInMainTests.java b/buildSrc/src/main/java/org/elasticsearch/test/NamingConventionsCheckInMainTests.java new file mode 100644 index 00000000000..27c0b41eb3f --- /dev/null +++ b/buildSrc/src/main/java/org/elasticsearch/test/NamingConventionsCheckInMainTests.java @@ -0,0 +1,26 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.test; + +/** + * This class should fail the naming conventions self test. + */ +public class NamingConventionsCheckInMainTests { +} diff --git a/test/framework/build.gradle b/test/framework/build.gradle index 6756495e0a1..13a5ef11ce2 100644 --- a/test/framework/build.gradle +++ b/test/framework/build.gradle @@ -63,3 +63,8 @@ thirdPartyAudit.excludes = [ 'org.easymock.IArgumentMatcher', 'org.jmock.core.Constraint', ] + +task namingConventionsMain(type: org.elasticsearch.gradle.precommit.NamingConventionsTask) { + checkForTestsInMain = true +} +precommit.dependsOn namingConventionsMain diff --git a/test/framework/src/main/java/org/elasticsearch/AnalysisFactoryTestCase.java b/test/framework/src/main/java/org/elasticsearch/AnalysisFactoryTestCase.java index d49a1b4cae5..cbabdeef4af 100644 --- a/test/framework/src/main/java/org/elasticsearch/AnalysisFactoryTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/AnalysisFactoryTestCase.java @@ -113,7 +113,7 @@ import java.util.regex.Pattern; * If we don't want to expose one for a specific reason, just map it to Void. * The deprecated ones can be mapped to Deprecated.class. */ -public class AnalysisFactoryTestCase extends ESTestCase { +public abstract class AnalysisFactoryTestCase extends ESTestCase { private static final Pattern UNDERSCORE_THEN_ANYTHING = Pattern.compile("_(.)"); diff --git a/test/framework/src/main/java/org/elasticsearch/test/disruption/LongGCDisruptionTest.java b/test/framework/src/test/java/org/elasticsearch/test/disruption/LongGCDisruptionTests.java similarity index 99% rename from test/framework/src/main/java/org/elasticsearch/test/disruption/LongGCDisruptionTest.java rename to test/framework/src/test/java/org/elasticsearch/test/disruption/LongGCDisruptionTests.java index a5cd7c30723..48bd18986c2 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/disruption/LongGCDisruptionTest.java +++ b/test/framework/src/test/java/org/elasticsearch/test/disruption/LongGCDisruptionTests.java @@ -34,7 +34,7 @@ import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.greaterThan; -public class LongGCDisruptionTest extends ESTestCase { +public class LongGCDisruptionTests extends ESTestCase { static class LockedExecutor { ReentrantLock lock = new ReentrantLock(); diff --git a/test/framework/src/main/java/org/elasticsearch/test/disruption/NetworkDisruptionIT.java b/test/framework/src/test/java/org/elasticsearch/test/disruption/NetworkDisruptionIT.java similarity index 100% rename from test/framework/src/main/java/org/elasticsearch/test/disruption/NetworkDisruptionIT.java rename to test/framework/src/test/java/org/elasticsearch/test/disruption/NetworkDisruptionIT.java diff --git a/test/framework/src/main/java/org/elasticsearch/test/disruption/NetworkDisruptionTests.java b/test/framework/src/test/java/org/elasticsearch/test/disruption/NetworkDisruptionTests.java similarity index 100% rename from test/framework/src/main/java/org/elasticsearch/test/disruption/NetworkDisruptionTests.java rename to test/framework/src/test/java/org/elasticsearch/test/disruption/NetworkDisruptionTests.java