diff --git a/buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginBuildPlugin.groovy b/buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginBuildPlugin.groovy index 9b070f71c20..bdb563e001b 100644 --- a/buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginBuildPlugin.groovy +++ b/buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginBuildPlugin.groovy @@ -51,6 +51,11 @@ public class PluginBuildPlugin extends BuildPlugin { project.integTest.clusterConfig.plugin(name, project.bundlePlugin.outputs.files) project.tasks.run.clusterConfig.plugin(name, project.bundlePlugin.outputs.files) } + + project.namingConventions { + // Plugins decalare extensions of ESIntegTestCase as "Tests" instead of IT. + skipIntegTestInDisguise = true + } } createIntegTestTask(project) createBundleTask(project) diff --git a/buildSrc/src/main/groovy/org/elasticsearch/gradle/precommit/NamingConventionsTask.groovy b/buildSrc/src/main/groovy/org/elasticsearch/gradle/precommit/NamingConventionsTask.groovy new file mode 100644 index 00000000000..672d208c01c --- /dev/null +++ b/buildSrc/src/main/groovy/org/elasticsearch/gradle/precommit/NamingConventionsTask.groovy @@ -0,0 +1,89 @@ +/* + * 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.gradle.precommit + +import org.elasticsearch.gradle.LoggedExec +import org.elasticsearch.gradle.VersionProperties +import org.gradle.api.file.FileCollection +import org.gradle.api.tasks.Input +import org.gradle.api.tasks.InputFiles +import org.gradle.api.tasks.OutputFile + +/** + * Runs NamingConventionsCheck on a classpath/directory combo to verify that + * tests are named according to our conventions so they'll be picked up by + * gradle. Read the Javadoc for NamingConventionsCheck to learn more. + */ +public class NamingConventionsTask extends LoggedExec { + /** + * We use a simple "marker" file that we touch when the task succeeds + * as the task output. This is compared against the modified time of the + * 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 + + /** + * Should we skip the integ tests in disguise tests? Defaults to true because only core names its + * integ tests correctly. + */ + @Input + boolean skipIntegTestInDisguise = false + + public NamingConventionsTask() { + dependsOn(classpath) + description = "Runs NamingConventionsCheck on ${classpath}" + executable = new File(project.javaHome, 'bin/java') + 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. + */ + project.afterEvaluate { + doFirst { + args('-cp', classpath.asPath, 'org.elasticsearch.test.NamingConventionsCheck') + if (skipIntegTestInDisguise) { + args('--skip-integ-tests-in-disguise') + } + /* + * The test framework has classes that fail the checks to validate that the checks fail properly. + * Since these would cause the build to fail we have to ignore them with this parameter. The + * 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)) { + args('--self-test') + } + args('--', project.sourceSets.test.output.classesDir.absolutePath) + } + } + doLast { successMarker.setText("", 'UTF-8') } + } +} diff --git a/buildSrc/src/main/groovy/org/elasticsearch/gradle/precommit/PrecommitTasks.groovy b/buildSrc/src/main/groovy/org/elasticsearch/gradle/precommit/PrecommitTasks.groovy index dd77c38e679..ab524351274 100644 --- a/buildSrc/src/main/groovy/org/elasticsearch/gradle/precommit/PrecommitTasks.groovy +++ b/buildSrc/src/main/groovy/org/elasticsearch/gradle/precommit/PrecommitTasks.groovy @@ -33,6 +33,7 @@ class PrecommitTasks { List precommitTasks = [ configureForbiddenApis(project), configureCheckstyle(project), + configureNamingConventions(project), project.tasks.create('forbiddenPatterns', ForbiddenPatternsTask.class), project.tasks.create('licenseHeaders', LicenseHeadersTask.class), project.tasks.create('jarHell', JarHellTask.class), @@ -109,4 +110,11 @@ class PrecommitTasks { } return checkstyleTask } + + private static Task configureNamingConventions(Project project) { + if (project.sourceSets.findByName("test")) { + return project.tasks.create('namingConventions', NamingConventionsTask) + } + return null + } } diff --git a/buildSrc/src/main/resources/checkstyle_suppressions.xml b/buildSrc/src/main/resources/checkstyle_suppressions.xml index 6b7ad9499a5..494ddf5d071 100644 --- a/buildSrc/src/main/resources/checkstyle_suppressions.xml +++ b/buildSrc/src/main/resources/checkstyle_suppressions.xml @@ -1525,7 +1525,6 @@ - @@ -1606,7 +1605,6 @@ - diff --git a/core/src/test/java/org/elasticsearch/NamingConventionTests.java b/core/src/test/java/org/elasticsearch/NamingConventionTests.java deleted file mode 100644 index 41d67b88390..00000000000 --- a/core/src/test/java/org/elasticsearch/NamingConventionTests.java +++ /dev/null @@ -1,177 +0,0 @@ -/* - * 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; - -import junit.framework.TestCase; -import org.apache.lucene.util.LuceneTestCase; -import org.elasticsearch.common.io.PathUtils; -import org.elasticsearch.test.ESIntegTestCase; -import org.elasticsearch.test.ESTestCase; -import org.elasticsearch.test.ESTokenStreamTestCase; - -import java.io.IOException; -import java.lang.reflect.Modifier; -import java.net.URISyntaxException; -import java.nio.file.FileVisitResult; -import java.nio.file.FileVisitor; -import java.nio.file.Files; -import java.nio.file.Path; -import java.nio.file.attribute.BasicFileAttributes; -import java.util.HashSet; -import java.util.Set; -import java.util.stream.Collectors; - -/** - * Simple class that ensures that all subclasses concrete of ESTestCase end with either Test | Tests - */ -public class NamingConventionTests extends ESTestCase { - - // see https://github.com/elasticsearch/elasticsearch/issues/9945 - public void testNamingConventions() - throws ClassNotFoundException, IOException, URISyntaxException { - final Set notImplementing = new HashSet<>(); - final Set pureUnitTest = new HashSet<>(); - final Set missingSuffix = new HashSet<>(); - final Set integTestsInDisguise = new HashSet<>(); - final Set notRunnable = new HashSet<>(); - final Set innerClasses = new HashSet<>(); - String[] packages = {"org.elasticsearch", "org.apache.lucene"}; - for (final String packageName : packages) { - final String path = "/" + packageName.replace('.', '/'); - final Path startPath = getDataPath(path); - Files.walkFileTree(startPath, new FileVisitor() { - private Path pkgPrefix = PathUtils.get(path).getParent(); - @Override - public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) throws IOException { - pkgPrefix = pkgPrefix.resolve(dir.getFileName()); - return FileVisitResult.CONTINUE; - } - - @Override - public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException { - try { - String filename = file.getFileName().toString(); - if (filename.endsWith(".class")) { - Class clazz = loadClass(filename); - if (clazz.getName().endsWith("Tests")) { // don't worry about the ones that match the pattern - - if (ESIntegTestCase.class.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); - } - // otherwise fine - } 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); - } - } - } - } catch (ClassNotFoundException e) { - throw new RuntimeException(e); - } - return FileVisitResult.CONTINUE; - } - - private boolean isTestCase(Class clazz) { - return LuceneTestCase.class.isAssignableFrom(clazz); - } - - private Class loadClass(String filename) throws ClassNotFoundException { - StringBuilder pkg = new StringBuilder(); - for (Path p : pkgPrefix) { - pkg.append(p.getFileName().toString()).append("."); - } - pkg.append(filename.substring(0, filename.length() - 6)); - return Thread.currentThread().getContextClassLoader().loadClass(pkg.toString()); - } - - @Override - public FileVisitResult visitFileFailed(Path file, IOException exc) throws IOException { - throw exc; - } - - @Override - public FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOException { - pkgPrefix = pkgPrefix.getParent(); - return FileVisitResult.CONTINUE; - } - }); - - } - assertTrue(missingSuffix.remove(WrongName.class)); - assertTrue(missingSuffix.remove(WrongNameTheSecond.class)); - assertTrue(notRunnable.remove(DummyAbstractTests.class)); - assertTrue(notRunnable.remove(DummyInterfaceTests.class)); - assertTrue(innerClasses.remove(InnerTests.class)); - assertTrue(notImplementing.remove(NotImplementingTests.class)); - assertTrue(pureUnitTest.remove(PlainUnit.class)); - - String classesToSubclass = String.join( - ",", - ESTestCase.class.getSimpleName(), - ESTestCase.class.getSimpleName(), - ESTokenStreamTestCase.class.getSimpleName(), - LuceneTestCase.class.getSimpleName() - ); - assertNoViolations("Not all subclasses of " + ESTestCase.class.getSimpleName() + " match the naming convention. Concrete classes must end with [Tests]:\n", missingSuffix); - assertNoViolations("Classes ending with [Tests] are abstract or interfaces:\n", notRunnable); - assertNoViolations("Found inner classes that are tests, which are excluded from the test runner:\n", innerClasses); - assertNoViolations("Pure Unit-Test found must subclass one of [" + classesToSubclass + "]:\n", pureUnitTest); - assertNoViolations("Classes ending with [Tests] must subclass [" + classesToSubclass + "]:\n", notImplementing); - assertNoViolations("Subclasses of ESIntegTestCase should end with IT as they are integration tests:\n", integTestsInDisguise); - } - - private String join(Set set) { - return set.stream().map(Object::toString).collect(Collectors.joining("\n")); - } - - private void assertNoViolations(String message, Set set) { - assertTrue(message + join(set), set.isEmpty()); - } - - /* - * Some test the test classes - */ - - public static final class NotImplementingTests {} - - public static final class WrongName extends ESTestCase {} - - public static abstract class DummyAbstractTests extends ESTestCase {} - - public interface DummyInterfaceTests {} - - public static final class InnerTests extends ESTestCase {} - - public static final class WrongNameTheSecond extends ESTestCase {} - - public static final class PlainUnit extends TestCase {} -} diff --git a/plugins/analysis-icu/src/test/java/org/elasticsearch/index/analysis/TestIndexableBinaryStringTools.java b/plugins/analysis-icu/src/test/java/org/elasticsearch/index/analysis/IndexableBinaryStringToolsTests.java similarity index 99% rename from plugins/analysis-icu/src/test/java/org/elasticsearch/index/analysis/TestIndexableBinaryStringTools.java rename to plugins/analysis-icu/src/test/java/org/elasticsearch/index/analysis/IndexableBinaryStringToolsTests.java index ab51c62d1cf..24890fed5a9 100644 --- a/plugins/analysis-icu/src/test/java/org/elasticsearch/index/analysis/TestIndexableBinaryStringTools.java +++ b/plugins/analysis-icu/src/test/java/org/elasticsearch/index/analysis/IndexableBinaryStringToolsTests.java @@ -40,7 +40,7 @@ import java.util.Locale; @ThreadLeakScope(Scope.NONE) @TimeoutSuite(millis = TimeUnits.HOUR) @LuceneTestCase.SuppressSysoutChecks(bugUrl = "we log a lot on purpose") -public class TestIndexableBinaryStringTools extends LuceneTestCase { +public class IndexableBinaryStringToolsTests extends LuceneTestCase { private static int NUM_RANDOM_TESTS; private static int MAX_RANDOM_BINARY_LENGTH; private static final String LINE_SEPARATOR = System.lineSeparator(); diff --git a/plugins/mapper-attachments/src/test/java/org/elasticsearch/mapper/attachments/AttachmentUnitTestCase.java b/plugins/mapper-attachments/src/test/java/org/elasticsearch/mapper/attachments/AttachmentUnitTestCase.java index 81a82825f8b..2d55587961b 100644 --- a/plugins/mapper-attachments/src/test/java/org/elasticsearch/mapper/attachments/AttachmentUnitTestCase.java +++ b/plugins/mapper-attachments/src/test/java/org/elasticsearch/mapper/attachments/AttachmentUnitTestCase.java @@ -27,7 +27,7 @@ import org.elasticsearch.indices.IndicesModule; import org.elasticsearch.test.ESTestCase; import org.junit.Before; -public class AttachmentUnitTestCase extends ESTestCase { +public abstract class AttachmentUnitTestCase extends ESTestCase { protected Settings testSettings; diff --git a/qa/smoke-test-ingest-with-all-dependencies/src/test/java/org/elasticsearch/ingest/AbstractMustacheTests.java b/qa/smoke-test-ingest-with-all-dependencies/src/test/java/org/elasticsearch/ingest/AbstractMustacheTestCase.java similarity index 79% rename from qa/smoke-test-ingest-with-all-dependencies/src/test/java/org/elasticsearch/ingest/AbstractMustacheTests.java rename to qa/smoke-test-ingest-with-all-dependencies/src/test/java/org/elasticsearch/ingest/AbstractMustacheTestCase.java index 49e9964427d..d13c4fe0f79 100644 --- a/qa/smoke-test-ingest-with-all-dependencies/src/test/java/org/elasticsearch/ingest/AbstractMustacheTests.java +++ b/qa/smoke-test-ingest-with-all-dependencies/src/test/java/org/elasticsearch/ingest/AbstractMustacheTestCase.java @@ -32,7 +32,7 @@ import org.junit.Before; import java.util.Collections; -public abstract class AbstractMustacheTests extends ESTestCase { +public abstract class AbstractMustacheTestCase extends ESTestCase { protected TemplateService templateService; @@ -43,12 +43,12 @@ public abstract class AbstractMustacheTests extends ESTestCase { .put(ScriptService.SCRIPT_AUTO_RELOAD_ENABLED_SETTING.getKey(), false) .build(); MustacheScriptEngineService mustache = new MustacheScriptEngineService(settings); - ScriptEngineRegistry scriptEngineRegistry = - new ScriptEngineRegistry(Collections.singletonList(new ScriptEngineRegistry.ScriptEngineRegistration(MustacheScriptEngineService.class, MustacheScriptEngineService.TYPES))); + ScriptEngineRegistry scriptEngineRegistry = new ScriptEngineRegistry(Collections.singletonList( + new ScriptEngineRegistry.ScriptEngineRegistration(MustacheScriptEngineService.class, MustacheScriptEngineService.TYPES))); ScriptContextRegistry scriptContextRegistry = new ScriptContextRegistry(Collections.emptyList()); ScriptSettings scriptSettings = new ScriptSettings(scriptEngineRegistry, scriptContextRegistry); - ScriptService scriptService = - new ScriptService(settings, new Environment(settings), Collections.singleton(mustache), null, scriptEngineRegistry, scriptContextRegistry, scriptSettings); + ScriptService scriptService = new ScriptService(settings, new Environment(settings), Collections.singleton(mustache), null, + scriptEngineRegistry, scriptContextRegistry, scriptSettings); templateService = new InternalTemplateService(scriptService); } diff --git a/qa/smoke-test-ingest-with-all-dependencies/src/test/java/org/elasticsearch/ingest/IngestDocumentMustacheIT.java b/qa/smoke-test-ingest-with-all-dependencies/src/test/java/org/elasticsearch/ingest/IngestDocumentMustacheIT.java index f27a8e4c8d6..c8c50603625 100644 --- a/qa/smoke-test-ingest-with-all-dependencies/src/test/java/org/elasticsearch/ingest/IngestDocumentMustacheIT.java +++ b/qa/smoke-test-ingest-with-all-dependencies/src/test/java/org/elasticsearch/ingest/IngestDocumentMustacheIT.java @@ -31,7 +31,7 @@ import java.util.Map; import static org.hamcrest.Matchers.equalTo; -public class IngestDocumentMustacheIT extends AbstractMustacheTests { +public class IngestDocumentMustacheIT extends AbstractMustacheTestCase { public void testAccessMetaDataViaTemplate() { Map document = new HashMap<>(); diff --git a/qa/smoke-test-ingest-with-all-dependencies/src/test/java/org/elasticsearch/ingest/IngestMustacheRemoveProcessorIT.java b/qa/smoke-test-ingest-with-all-dependencies/src/test/java/org/elasticsearch/ingest/IngestMustacheRemoveProcessorIT.java index e94765a4aad..621cbbc2beb 100644 --- a/qa/smoke-test-ingest-with-all-dependencies/src/test/java/org/elasticsearch/ingest/IngestMustacheRemoveProcessorIT.java +++ b/qa/smoke-test-ingest-with-all-dependencies/src/test/java/org/elasticsearch/ingest/IngestMustacheRemoveProcessorIT.java @@ -26,7 +26,7 @@ import java.util.Collections; import java.util.HashMap; import java.util.Map; -public class IngestMustacheRemoveProcessorIT extends AbstractMustacheTests { +public class IngestMustacheRemoveProcessorIT extends AbstractMustacheTestCase { public void testRemoveProcessorMustacheExpression() throws Exception { RemoveProcessor.Factory factory = new RemoveProcessor.Factory(templateService); diff --git a/qa/smoke-test-ingest-with-all-dependencies/src/test/java/org/elasticsearch/ingest/IngestMustacheSetProcessorIT.java b/qa/smoke-test-ingest-with-all-dependencies/src/test/java/org/elasticsearch/ingest/IngestMustacheSetProcessorIT.java index 68466795b74..ed5ad65466b 100644 --- a/qa/smoke-test-ingest-with-all-dependencies/src/test/java/org/elasticsearch/ingest/IngestMustacheSetProcessorIT.java +++ b/qa/smoke-test-ingest-with-all-dependencies/src/test/java/org/elasticsearch/ingest/IngestMustacheSetProcessorIT.java @@ -33,7 +33,7 @@ import java.util.Map; import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.CoreMatchers.instanceOf; -public class IngestMustacheSetProcessorIT extends AbstractMustacheTests { +public class IngestMustacheSetProcessorIT extends AbstractMustacheTestCase { public void testExpression() throws Exception { SetProcessor processor = createSetProcessor("_index", "text {{var}}"); diff --git a/qa/smoke-test-ingest-with-all-dependencies/src/test/java/org/elasticsearch/ingest/TemplateServiceIT.java b/qa/smoke-test-ingest-with-all-dependencies/src/test/java/org/elasticsearch/ingest/TemplateServiceIT.java index 1d1579fe66a..d33976b2507 100644 --- a/qa/smoke-test-ingest-with-all-dependencies/src/test/java/org/elasticsearch/ingest/TemplateServiceIT.java +++ b/qa/smoke-test-ingest-with-all-dependencies/src/test/java/org/elasticsearch/ingest/TemplateServiceIT.java @@ -27,7 +27,7 @@ import java.util.Map; import static org.hamcrest.Matchers.equalTo; -public class TemplateServiceIT extends AbstractMustacheTests { +public class TemplateServiceIT extends AbstractMustacheTestCase { public void testTemplates() { Map model = new HashMap<>(); diff --git a/qa/smoke-test-ingest-with-all-dependencies/src/test/java/org/elasticsearch/ingest/ValueSourceMustacheIT.java b/qa/smoke-test-ingest-with-all-dependencies/src/test/java/org/elasticsearch/ingest/ValueSourceMustacheIT.java index 18085b94b04..c7db97cd3dd 100644 --- a/qa/smoke-test-ingest-with-all-dependencies/src/test/java/org/elasticsearch/ingest/ValueSourceMustacheIT.java +++ b/qa/smoke-test-ingest-with-all-dependencies/src/test/java/org/elasticsearch/ingest/ValueSourceMustacheIT.java @@ -32,7 +32,7 @@ import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; -public class ValueSourceMustacheIT extends AbstractMustacheTests { +public class ValueSourceMustacheIT extends AbstractMustacheTestCase { public void testValueSourceWithTemplates() { Map model = new HashMap<>(); diff --git a/test/framework/src/main/java/org/elasticsearch/test/NamingConventionsCheck.java b/test/framework/src/main/java/org/elasticsearch/test/NamingConventionsCheck.java new file mode 100644 index 00000000000..13163cee029 --- /dev/null +++ b/test/framework/src/main/java/org/elasticsearch/test/NamingConventionsCheck.java @@ -0,0 +1,221 @@ +/* + * 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; + +import java.io.IOException; +import java.lang.reflect.Modifier; +import java.nio.file.FileVisitResult; +import java.nio.file.FileVisitor; +import java.nio.file.Files; +import java.nio.file.Path; +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 + * a class with a main method so gradle can call it for each project. This has the advantage of allowing gradle to calculate when it is + * {@code UP-TO-DATE} so it can be skipped if the compiled classes haven't changed. This is useful on large modules for which checking all + * the modules can be slow. + * + * Annoyingly, this cannot be tested using standard unit tests because to do so you'd have to declare classes that violate the rules. That + * would cause the test fail which would prevent the build from passing. So we have to make a mechanism for removing those test classes. Now + * that we have such a mechanism it isn't much work to fail the process if we don't detect the offending classes. Thus, the funky + * {@code --self-test} that is only run in the test:framework project. + */ +public class NamingConventionsCheck { + public static void main(String[] args) throws IOException, ClassNotFoundException { + NamingConventionsCheck check = new NamingConventionsCheck(); + boolean skipIntegTestsInDisguise = false; + boolean selfTest = false; + int i = 0; + while (true) { + switch (args[i]) { + case "--skip-integ-tests-in-disguise": + skipIntegTestsInDisguise = true; + i++; + continue; + case "--self-test": + selfTest = true; + i++; + continue; + case "--": + i++; + break; + default: + fail("Expected -- before a path."); + } + break; + } + check.check(PathUtils.get(args[i])); + + 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); + } + + // Now we should have no violations + assertNoViolations("Not all subclasses of " + ESTestCase.class.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(), + ESTokenStreamTestCase.class.getSimpleName(), LuceneTestCase.class.getSimpleName()); + 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); + } + } + + private final Set> notImplementing = new HashSet<>(); + private final Set> pureUnitTest = new HashSet<>(); + private final Set> missingSuffix = new HashSet<>(); + private final Set> integTestsInDisguise = new HashSet<>(); + private final Set> notRunnable = new HashSet<>(); + private final Set> innerClasses = new HashSet<>(); + + public void check(Path rootPath) 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; + + @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() + "."; + } + 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 = loadClass(className); + if (clazz.getName().endsWith("Tests")) { + if (ESIntegTestCase.class.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); + } + } + } + return FileVisitResult.CONTINUE; + } + + private boolean isTestCase(Class clazz) { + return LuceneTestCase.class.isAssignableFrom(clazz); + } + + private Class loadClass(String className) { + try { + return Thread.currentThread().getContextClassLoader().loadClass(packageName + className); + } catch (ClassNotFoundException e) { + throw new RuntimeException(e); + } + } + + @Override + public FileVisitResult visitFileFailed(Path file, IOException exc) throws IOException { + throw exc; + } + }); + } + + /** + * 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> set) { + if (false == set.isEmpty()) { + System.err.println(message + ":"); + for (Class bad : set) { + System.err.println(" * " + bad.getName()); + } + System.exit(1); + } + } + + /** + * 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> set) throws ClassNotFoundException { + className = "org.elasticsearch.test.test.NamingConventionsCheckBadClasses$" + 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); + } + } + + /** + * 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); + } +} diff --git a/test/framework/src/test/java/org/elasticsearch/test/test/NamingConventionsCheckBadClasses.java b/test/framework/src/test/java/org/elasticsearch/test/test/NamingConventionsCheckBadClasses.java new file mode 100644 index 00000000000..233e9fe5975 --- /dev/null +++ b/test/framework/src/test/java/org/elasticsearch/test/test/NamingConventionsCheckBadClasses.java @@ -0,0 +1,50 @@ +/* + * 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.test; + +import org.elasticsearch.test.ESTestCase; + +import junit.framework.TestCase; + +/** + * These inner classes all fail the NamingConventionsCheck. They have to live in the tests or else they won't be scanned. + */ +public class NamingConventionsCheckBadClasses { + public static final class NotImplementingTests { + } + + public static final class WrongName extends ESTestCase { + } + + public static abstract class DummyAbstractTests extends ESTestCase { + } + + public interface DummyInterfaceTests { + } + + public static final class InnerTests extends ESTestCase { + } + + public static final class WrongNameTheSecond extends ESTestCase { + } + + public static final class PlainUnit extends TestCase { + } +}