From 37768b7eac24ad226dc0e4b0d34614d98b33fcd2 Mon Sep 17 00:00:00 2001 From: Alpar Torok Date: Thu, 24 Jan 2019 17:30:50 +0200 Subject: [PATCH] Testing conventions now checks for tests in main (#37321) * Testing conventions now checks for tests in main This is the last outstanding feature of the old NamingConventionsTask, so time to remove it. * PR review --- buildSrc/build.gradle | 5 - .../gradle/plugin/PluginBuildPlugin.groovy | 5 - .../gradle/precommit/PrecommitTasks.groovy | 10 - .../precommit/NamingConventionsTask.java | 167 --------- .../precommit/TestingConventionsTasks.java | 47 ++- .../test/NamingConventionsCheck.java | 318 ------------------ .../precommit/NamingConventionsTaskIT.java | 58 ---- .../precommit/TestingConventionsTasksIT.java | 11 + buildSrc/src/testKit/jarHell/build.gradle | 1 - .../namingConventionsSelfTest/build.gradle | 24 -- .../namingConventionsSelfTest/settings.gradle | 0 .../NamingConventionsCheckBadClasses.java | 62 ---- .../test/NamingConventionsCheckInMainIT.java | 31 -- .../testKit/testingConventions/build.gradle | 4 + .../testingConventions/settings.gradle | 3 +- .../gradle/testkit/NamingConventionIT.java} | 9 +- .../testkit/NamingConventionTests.java} | 7 +- client/rest/build.gradle | 6 - client/sniffer/build.gradle | 6 - client/test/build.gradle | 2 - client/transport/build.gradle | 6 - .../tools/java-version-checker/build.gradle | 1 - distribution/tools/launchers/build.gradle | 5 - libs/secure-sm/build.gradle | 4 - qa/vagrant/build.gradle | 2 + test/framework/build.gradle | 6 - x-pack/qa/openldap-tests/build.gradle | 5 - x-pack/transport-client/build.gradle | 6 - 28 files changed, 66 insertions(+), 745 deletions(-) delete mode 100644 buildSrc/src/main/java/org/elasticsearch/gradle/precommit/NamingConventionsTask.java delete mode 100644 buildSrc/src/main/minimumRuntime/org/elasticsearch/test/NamingConventionsCheck.java delete mode 100644 buildSrc/src/test/java/org/elasticsearch/gradle/precommit/NamingConventionsTaskIT.java delete mode 100644 buildSrc/src/testKit/namingConventionsSelfTest/build.gradle delete mode 100644 buildSrc/src/testKit/namingConventionsSelfTest/settings.gradle delete mode 100644 buildSrc/src/testKit/namingConventionsSelfTest/src/main/java/org/elasticsearch/test/NamingConventionsCheckBadClasses.java delete mode 100644 buildSrc/src/testKit/namingConventionsSelfTest/src/test/java/org/elasticsearch/test/NamingConventionsCheckInMainIT.java rename buildSrc/src/testKit/{namingConventionsSelfTest/src/test/java/org/elasticsearch/test/WrongName.java => testingConventions/tests_in_main/src/main/java/org/elasticsearch/gradle/testkit/NamingConventionIT.java} (81%) rename buildSrc/src/testKit/{namingConventionsSelfTest/src/test/java/org/elasticsearch/test/NamingConventionsCheckInMainTests.java => testingConventions/tests_in_main/src/main/java/org/elasticsearch/gradle/testkit/NamingConventionTests.java} (84%) diff --git a/buildSrc/build.gradle b/buildSrc/build.gradle index b98cdd8788b..0acdc95c86b 100644 --- a/buildSrc/build.gradle +++ b/buildSrc/build.gradle @@ -235,11 +235,6 @@ if (project != rootProject) { exclude '**/ForbiddenPatternsTask.java' } - namingConventions { - testClass = 'org.elasticsearch.gradle.test.GradleUnitTestCase' - integTestClass = 'org.elasticsearch.gradle.test.GradleIntegrationTestCase' - } - testingConventions { naming.clear() naming { 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 306ac4a05e8..9f35c6b9e68 100644 --- a/buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginBuildPlugin.groovy +++ b/buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginBuildPlugin.groovy @@ -72,11 +72,6 @@ public class PluginBuildPlugin extends BuildPlugin { if (isModule == false || isXPackModule) { addNoticeGeneration(project) } - - project.namingConventions { - // Plugins declare integration tests as "Tests" instead of IT. - skipIntegTestInDisguise = true - } } project.testingConventions { naming.clear() 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 28c86a28f71..b2a9663cf7a 100644 --- a/buildSrc/src/main/groovy/org/elasticsearch/gradle/precommit/PrecommitTasks.groovy +++ b/buildSrc/src/main/groovy/org/elasticsearch/gradle/precommit/PrecommitTasks.groovy @@ -43,7 +43,6 @@ class PrecommitTasks { List precommitTasks = [ configureCheckstyle(project), configureForbiddenApisCli(project), - configureNamingConventions(project), project.tasks.create('forbiddenPatterns', ForbiddenPatternsTask.class), project.tasks.create('licenseHeaders', LicenseHeadersTask.class), project.tasks.create('filepermissions', FilePermissionsTask.class), @@ -230,15 +229,6 @@ class PrecommitTasks { return checkstyleTask } - private static Task configureNamingConventions(Project project) { - if (project.sourceSets.findByName("test")) { - Task namingConventionsTask = project.tasks.create('namingConventions', NamingConventionsTask) - namingConventionsTask.javaHome = project.compilerJavaHome - return namingConventionsTask - } - return null - } - private static Task configureLoggerUsage(Project project) { project.configurations.create('loggerUsagePlugin') project.dependencies.add('loggerUsagePlugin', diff --git a/buildSrc/src/main/java/org/elasticsearch/gradle/precommit/NamingConventionsTask.java b/buildSrc/src/main/java/org/elasticsearch/gradle/precommit/NamingConventionsTask.java deleted file mode 100644 index b0e36982918..00000000000 --- a/buildSrc/src/main/java/org/elasticsearch/gradle/precommit/NamingConventionsTask.java +++ /dev/null @@ -1,167 +0,0 @@ -package org.elasticsearch.gradle.precommit; - -import org.elasticsearch.gradle.LoggedExec; -import org.elasticsearch.test.NamingConventionsCheck; -import org.gradle.api.GradleException; -import org.gradle.api.file.FileCollection; -import org.gradle.api.plugins.JavaPluginConvention; -import org.gradle.api.tasks.Classpath; -import org.gradle.api.tasks.Input; -import org.gradle.api.tasks.InputFiles; -import org.gradle.api.tasks.SkipWhenEmpty; -import org.gradle.api.tasks.SourceSetContainer; -import org.gradle.api.tasks.TaskAction; - -import java.io.File; -import java.net.URISyntaxException; -import java.net.URL; - -/** - * 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. - */ -@SuppressWarnings("unchecked") -public class NamingConventionsTask extends PrecommitTask { - - public NamingConventionsTask() { - setDescription("Tests that test classes aren't misnamed or misplaced"); - dependsOn(getJavaSourceSets().getByName(checkForTestsInMain ? "main" : "test").getClassesTaskName()); - } - - @TaskAction - public void runNamingConventions() { - LoggedExec.javaexec(getProject(), spec -> { - spec.classpath( - getNamingConventionsCheckClassFiles(), - getSourceSetClassPath() - ); - spec.executable(getJavaHome() + "/bin/java"); - spec.jvmArgs("-Djna.nosys=true"); - spec.setMain(NamingConventionsCheck.class.getName()); - spec.args("--test-class", getTestClass()); - if (isSkipIntegTestInDisguise()) { - spec.args("--skip-integ-tests-in-disguise"); - } else { - spec.args("--integ-test-class", getIntegTestClass()); - } - if (isCheckForTestsInMain()) { - spec.args("--main"); - spec.args("--"); - } else { - spec.args("--"); - } - spec.args(getExistingClassesDirs().getAsPath()); - }); - } - - @Input - public Object getJavaHome() { - return javaHome; - } - - public void setJavaHome(Object javaHome) { - this.javaHome = javaHome; - } - - @Classpath - public FileCollection getSourceSetClassPath() { - SourceSetContainer sourceSets = getJavaSourceSets(); - return getProject().files( - sourceSets.getByName("test").getCompileClasspath(), - sourceSets.getByName("test").getOutput(), - checkForTestsInMain ? sourceSets.getByName("main").getRuntimeClasspath() : getProject().files() - ); - } - - @InputFiles - public File getNamingConventionsCheckClassFiles() { - // This works because the class only depends on one class from junit that will be available from the - // tests compile classpath. It's the most straight forward way of telling Java where to find the main - // class. - URL location = NamingConventionsCheck.class.getProtectionDomain().getCodeSource().getLocation(); - if (location.getProtocol().equals("file") == false) { - throw new GradleException("Unexpected location for NamingConventionCheck class: "+ location); - } - try { - return new File(location.toURI().getPath()); - } catch (URISyntaxException e) { - throw new AssertionError(e); - } - } - - @InputFiles - @SkipWhenEmpty - public FileCollection getExistingClassesDirs() { - FileCollection classesDirs = getJavaSourceSets().getByName(checkForTestsInMain ? "main" : "test") - .getOutput().getClassesDirs(); - return classesDirs.filter(it -> it.exists()); - } - - @Input - public boolean isSkipIntegTestInDisguise() { - return skipIntegTestInDisguise; - } - - public void setSkipIntegTestInDisguise(boolean skipIntegTestInDisguise) { - this.skipIntegTestInDisguise = skipIntegTestInDisguise; - } - - @Input - public String getTestClass() { - return testClass; - } - - public void setTestClass(String testClass) { - this.testClass = testClass; - } - - @Input - public String getIntegTestClass() { - return integTestClass; - } - - public void setIntegTestClass(String integTestClass) { - this.integTestClass = integTestClass; - } - - @Input - public boolean isCheckForTestsInMain() { - return checkForTestsInMain; - } - - public void setCheckForTestsInMain(boolean checkForTestsInMain) { - this.checkForTestsInMain = checkForTestsInMain; - } - - private SourceSetContainer getJavaSourceSets() { - return getProject().getConvention().getPlugin(JavaPluginConvention.class).getSourceSets(); - } - - /** - * The java home to run the check with - */ - private Object javaHome; // Make it an Object to allow for Groovy GString - - /** - * Should we skip the integ tests in disguise tests? Defaults to true because only core names its - * integ tests correctly. - */ - private boolean skipIntegTestInDisguise = false; - - /** - * Superclass for all tests. - */ - private String testClass = "org.apache.lucene.util.LuceneTestCase"; - - /** - * Superclass for all integration tests. - */ - private 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. - */ - private boolean checkForTestsInMain = false; -} diff --git a/buildSrc/src/main/java/org/elasticsearch/gradle/precommit/TestingConventionsTasks.java b/buildSrc/src/main/java/org/elasticsearch/gradle/precommit/TestingConventionsTasks.java index 0f207ad3fe1..04e1343f4ac 100644 --- a/buildSrc/src/main/java/org/elasticsearch/gradle/precommit/TestingConventionsTasks.java +++ b/buildSrc/src/main/java/org/elasticsearch/gradle/precommit/TestingConventionsTasks.java @@ -27,6 +27,8 @@ 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.SourceSet; +import org.gradle.api.tasks.SourceSetContainer; import org.gradle.api.tasks.TaskAction; import org.gradle.api.tasks.testing.Test; import org.gradle.api.tasks.util.PatternFilterable; @@ -122,6 +124,23 @@ public class TestingConventionsTasks extends DefaultTask { naming.configure(action); } + @Input + public Set getMainClassNamedLikeTests() { + SourceSetContainer javaSourceSets = Boilerplate.getJavaSourceSets(getProject()); + if (javaSourceSets.findByName(SourceSet.MAIN_SOURCE_SET_NAME) == null) { + // some test projects don't have a main source set + return Collections.emptySet(); + } + return javaSourceSets.getByName(SourceSet.MAIN_SOURCE_SET_NAME) + .getOutput().getClassesDirs().getAsFileTree() + .getFiles().stream() + .filter(file -> file.getName().endsWith(".class")) + .map(File::getName) + .map(name -> name.substring(0, name.length() - 6)) + .filter(this::implementsNamingConvention) + .collect(Collectors.toSet()); + } + @TaskAction public void doCheck() throws IOException { final String problems; @@ -235,10 +254,12 @@ public class TestingConventionsTasks extends DefaultTask { ); }).sorted() .collect(Collectors.joining("\n")) - ) + ), // TODO: check that the testing tasks are included in the right task based on the name ( from the rule ) - // TODO: check to make sure that the main source set doesn't have classes that match - // the naming convention (just the names, don't load classes) + checkNoneExists( + "Classes matching the test naming convention should be in test not main", + getMainClassNamedLikeTests() + ) ); } @@ -296,6 +317,18 @@ public class TestingConventionsTasks extends DefaultTask { } } + private String checkNoneExists(String message, Set candidates) { + String problem = candidates.stream() + .map(each -> " * " + each) + .sorted() + .collect(Collectors.joining("\n")); + if (problem.isEmpty() == false) { + return message + ":\n" + problem; + } else { + return ""; + } + } + private String checkAtLeastOneExists(String message, Stream> stream) { if (stream.findAny().isPresent()) { return ""; @@ -337,10 +370,14 @@ public class TestingConventionsTasks extends DefaultTask { } private boolean implementsNamingConvention(Class clazz) { + return implementsNamingConvention(clazz.getName()); + } + + private boolean implementsNamingConvention(String className) { if (naming.stream() .map(TestingConventionRule::getSuffix) - .anyMatch(suffix -> clazz.getName().endsWith(suffix))) { - getLogger().debug("{} is a test because it matches the naming convention", clazz.getName()); + .anyMatch(suffix -> className.endsWith(suffix))) { + getLogger().debug("{} is a test because it matches the naming convention", className); return true; } return false; diff --git a/buildSrc/src/main/minimumRuntime/org/elasticsearch/test/NamingConventionsCheck.java b/buildSrc/src/main/minimumRuntime/org/elasticsearch/test/NamingConventionsCheck.java deleted file mode 100644 index 17d885e21bc..00000000000 --- a/buildSrc/src/main/minimumRuntime/org/elasticsearch/test/NamingConventionsCheck.java +++ /dev/null @@ -1,318 +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.test; - -import java.io.File; -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.Paths; -import java.nio.file.attribute.BasicFileAttributes; -import java.util.HashSet; -import java.util.Objects; -import java.util.Set; -import java.util.regex.Pattern; - -/** - * 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. - */ -public class NamingConventionsCheck { - public static void main(String[] args) throws IOException { - Class testClass = null; - Class integTestClass = null; - String rootPathList = null; - boolean skipIntegTestsInDisguise = false; - boolean checkMainClasses = false; - for (int i = 0; i < args.length; i++) { - String arg = args[i]; - switch (arg) { - case "--test-class": - testClass = loadClassWithoutInitializing(args[++i]); - break; - case "--integ-test-class": - integTestClass = loadClassWithoutInitializing(args[++i]); - break; - case "--skip-integ-tests-in-disguise": - skipIntegTestsInDisguise = true; - break; - case "--main": - checkMainClasses = true; - break; - case "--": - rootPathList = args[++i]; - break; - default: - fail("unsupported argument '" + arg + "'"); - } - } - if (rootPathList == null) { - fail("No paths provided"); - return; - } - - NamingConventionsCheck check = new NamingConventionsCheck(testClass, integTestClass); - for (String rootDir : rootPathList.split(Pattern.quote(File.pathSeparator))) { - Path rootPath = Paths.get(rootDir); - if (checkMainClasses) { - check.checkMain(rootPath); - } else { - check.checkTests(rootPath, skipIntegTestsInDisguise); - } - } - - // Now we should have no violations - int exitCode = 0 ; - exitCode += countAndPrintViolations( - "Not all subclasses of " + check.testClass.getSimpleName() - + " match the naming convention. Concrete classes must end with [Tests]", - check.missingSuffix) ; - exitCode += countAndPrintViolations( - "Classes ending with [Tests] are abstract or interfaces", - check.notRunnable - ); - exitCode += countAndPrintViolations( - "Found inner classes that are tests, which are excluded from the test runner", - check.innerClasses - ); - exitCode += countAndPrintViolations( - "Pure Unit-Test found must subclass [" + check.testClass.getSimpleName() + "]", - check.pureUnitTest - ); - exitCode += countAndPrintViolations( - "Classes ending with [Tests] must subclass [" + check.testClass.getSimpleName() + "]", - check.notImplementing - ); - exitCode += countAndPrintViolations( - "Classes ending with [Tests] or [IT] or extending [" + - check.testClass.getSimpleName() + "] must be in src/test/java", - check.testsInMain - ); - if (skipIntegTestsInDisguise == false) { - exitCode += countAndPrintViolations("Subclasses of " + check.integTestClass.getSimpleName() + - " should end with IT as they are integration tests", - check.integTestsInDisguise - ); - } - System.exit(exitCode); - } - - 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<>(); - private final Set> testsInMain = new HashSet<>(); - - private final Class testClass; - private final Class integTestClass; - - public NamingConventionsCheck(Class testClass, Class integTestClass) { - this.testClass = Objects.requireNonNull(testClass, "--test-class is required"); - this.integTestClass = integTestClass; - } - - public void checkTests(Path rootPath, boolean skipTestsInDisguised) throws IOException { - Files.walkFileTree(rootPath, new TestClassVisitor() { - @Override - protected void visitTestClass(Class clazz) { - if (skipTestsInDisguised == false && - integTestClass.isAssignableFrom(clazz) && - clazz != integTestClass) { - 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); - } - } - - @Override - 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); - } - } - }); - - } - - private static int countAndPrintViolations(String message, Set> set) { - if (false == set.isEmpty()) { - System.err.println(message + ":"); - for (Class bad : set) { - System.err.println(" * " + bad.getName()); - } - return 1; - } - return 0; - } - - /** - * 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. - */ - private static void assertViolation(String className, Set> set) { - 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); - } - } - - /** - * Fail the process with the provided message. - */ - 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); - } - } - - 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/test/java/org/elasticsearch/gradle/precommit/NamingConventionsTaskIT.java b/buildSrc/src/test/java/org/elasticsearch/gradle/precommit/NamingConventionsTaskIT.java deleted file mode 100644 index 745c63cd4dc..00000000000 --- a/buildSrc/src/test/java/org/elasticsearch/gradle/precommit/NamingConventionsTaskIT.java +++ /dev/null @@ -1,58 +0,0 @@ -package org.elasticsearch.gradle.precommit; - -import org.elasticsearch.gradle.test.GradleIntegrationTestCase; -import org.gradle.testkit.runner.BuildResult; - -import java.util.Arrays; -import java.util.HashSet; - -public class NamingConventionsTaskIT extends GradleIntegrationTestCase { - - public void testNameCheckFailsAsItShould() { - BuildResult result = getGradleRunner("namingConventionsSelfTest") - .withArguments("namingConventions", "-s", "-PcheckForTestsInMain=false") - .buildAndFail(); - - assertTaskFailed(result, ":namingConventions"); - assertOutputContains( - result.getOutput(), - // TODO: java9 Set.of - new HashSet<>( - Arrays.asList( - "Not all subclasses of UnitTestCase match the naming convention. Concrete classes must end with [Tests]:", - "* org.elasticsearch.test.WrongName", - "Found inner classes that are tests, which are excluded from the test runner:", - "* org.elasticsearch.test.NamingConventionsCheckInMainIT$InternalInvalidTests", - "Classes ending with [Tests] must subclass [UnitTestCase]:", - "* org.elasticsearch.test.NamingConventionsCheckInMainTests", - "* org.elasticsearch.test.NamingConventionsCheckInMainIT" - ) - ) - ); - } - - public void testNameCheckFailsAsItShouldWithMain() { - BuildResult result = getGradleRunner("namingConventionsSelfTest") - .withArguments("namingConventions", "-s", "-PcheckForTestsInMain=true") - .buildAndFail(); - - assertTaskFailed(result, ":namingConventions"); - assertOutputContains( - result.getOutput(), - // TODO: java9 Set.of - new HashSet<>( - Arrays.asList( - "Classes ending with [Tests] or [IT] or extending [UnitTestCase] must be in src/test/java:", - "* org.elasticsearch.test.NamingConventionsCheckBadClasses$DummyInterfaceTests", - "* org.elasticsearch.test.NamingConventionsCheckBadClasses$DummyInterfaceTests", - "* org.elasticsearch.test.NamingConventionsCheckBadClasses$DummyAbstractTests", - "* org.elasticsearch.test.NamingConventionsCheckBadClasses$InnerTests", - "* org.elasticsearch.test.NamingConventionsCheckBadClasses$NotImplementingTests", - "* org.elasticsearch.test.NamingConventionsCheckBadClasses$WrongNameTheSecond", - "* org.elasticsearch.test.NamingConventionsCheckBadClasses$WrongName" - ) - ) - ); - } - -} diff --git a/buildSrc/src/test/java/org/elasticsearch/gradle/precommit/TestingConventionsTasksIT.java b/buildSrc/src/test/java/org/elasticsearch/gradle/precommit/TestingConventionsTasksIT.java index dbe06287782..39ab8a6734c 100644 --- a/buildSrc/src/test/java/org/elasticsearch/gradle/precommit/TestingConventionsTasksIT.java +++ b/buildSrc/src/test/java/org/elasticsearch/gradle/precommit/TestingConventionsTasksIT.java @@ -105,4 +105,15 @@ public class TestingConventionsTasksIT extends GradleIntegrationTestCase { assertTaskSuccessful(result, ":valid_setup_with_base:testingConventions"); } + public void testTestsInMain() { + GradleRunner runner = getGradleRunner("testingConventions") + .withArguments("clean", ":tests_in_main:testingConventions", "-i", "-s"); + BuildResult result = runner.buildAndFail(); + assertOutputContains(result.getOutput(), + "Classes matching the test naming convention should be in test not main:", + " * NamingConventionIT", + " * NamingConventionTests" + ); + } + } diff --git a/buildSrc/src/testKit/jarHell/build.gradle b/buildSrc/src/testKit/jarHell/build.gradle index f96e8ac37e0..cd423c9f99f 100644 --- a/buildSrc/src/testKit/jarHell/build.gradle +++ b/buildSrc/src/testKit/jarHell/build.gradle @@ -8,7 +8,6 @@ dependenciesInfo.enabled = false forbiddenApisMain.enabled = false forbiddenApisTest.enabled = false thirdPartyAudit.enabled = false -namingConventions.enabled = false ext.licenseFile = file("$buildDir/dummy/license") ext.noticeFile = file("$buildDir/dummy/notice") diff --git a/buildSrc/src/testKit/namingConventionsSelfTest/build.gradle b/buildSrc/src/testKit/namingConventionsSelfTest/build.gradle deleted file mode 100644 index b1c56ddc804..00000000000 --- a/buildSrc/src/testKit/namingConventionsSelfTest/build.gradle +++ /dev/null @@ -1,24 +0,0 @@ -plugins { - id 'java' - id 'elasticsearch.build' -} - -dependencyLicenses.enabled = false -dependenciesInfo.enabled = false -forbiddenApisMain.enabled = false -forbiddenApisTest.enabled = false -jarHell.enabled = false -thirdPartyAudit.enabled = false - -ext.licenseFile = file("$buildDir/dummy/license") -ext.noticeFile = file("$buildDir/dummy/notice") - -dependencies { - compile "junit:junit:4.12" -} - -namingConventions { - checkForTestsInMain = project.property("checkForTestsInMain") == "true" - testClass = 'org.elasticsearch.test.NamingConventionsCheckBadClasses$UnitTestCase' - integTestClass = 'org.elasticsearch.test.NamingConventionsCheckBadClasses$IntegTestCase' -} diff --git a/buildSrc/src/testKit/namingConventionsSelfTest/settings.gradle b/buildSrc/src/testKit/namingConventionsSelfTest/settings.gradle deleted file mode 100644 index e69de29bb2d..00000000000 diff --git a/buildSrc/src/testKit/namingConventionsSelfTest/src/main/java/org/elasticsearch/test/NamingConventionsCheckBadClasses.java b/buildSrc/src/testKit/namingConventionsSelfTest/src/main/java/org/elasticsearch/test/NamingConventionsCheckBadClasses.java deleted file mode 100644 index 4fc88b3afc5..00000000000 --- a/buildSrc/src/testKit/namingConventionsSelfTest/src/main/java/org/elasticsearch/test/NamingConventionsCheckBadClasses.java +++ /dev/null @@ -1,62 +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.test; - -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 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 abstract static class DummyAbstractTests extends UnitTestCase { - } - - public interface DummyInterfaceTests { - } - - public static final class InnerTests extends UnitTestCase { - public void testDummy() {} - } - - 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 { - } -} diff --git a/buildSrc/src/testKit/namingConventionsSelfTest/src/test/java/org/elasticsearch/test/NamingConventionsCheckInMainIT.java b/buildSrc/src/testKit/namingConventionsSelfTest/src/test/java/org/elasticsearch/test/NamingConventionsCheckInMainIT.java deleted file mode 100644 index 438f8015419..00000000000 --- a/buildSrc/src/testKit/namingConventionsSelfTest/src/test/java/org/elasticsearch/test/NamingConventionsCheckInMainIT.java +++ /dev/null @@ -1,31 +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.test; - -/** - * This class should fail the naming conventions self test. - */ -public class NamingConventionsCheckInMainIT { - - public static class InternalInvalidTests extends NamingConventionsCheckBadClasses.UnitTestCase { - - } - -} diff --git a/buildSrc/src/testKit/testingConventions/build.gradle b/buildSrc/src/testKit/testingConventions/build.gradle index d1a21a1ead0..00522450991 100644 --- a/buildSrc/src/testKit/testingConventions/build.gradle +++ b/buildSrc/src/testKit/testingConventions/build.gradle @@ -67,6 +67,10 @@ project(':valid_setup_no_base') { } } +project(':tests_in_main') { + +} + project (':valid_setup_with_base') { task randomized(type: com.carrotsearch.gradle.junit4.RandomizedTestingTask) { include "**/*IT.class" diff --git a/buildSrc/src/testKit/testingConventions/settings.gradle b/buildSrc/src/testKit/testingConventions/settings.gradle index 2baec09d27c..35d73cfd204 100644 --- a/buildSrc/src/testKit/testingConventions/settings.gradle +++ b/buildSrc/src/testKit/testingConventions/settings.gradle @@ -4,4 +4,5 @@ include 'empty_test_task' include 'all_classes_in_tasks' include 'not_implementing_base' include 'valid_setup_no_base' -include 'valid_setup_with_base' \ No newline at end of file +include 'valid_setup_with_base' +include 'tests_in_main' \ No newline at end of file diff --git a/buildSrc/src/testKit/namingConventionsSelfTest/src/test/java/org/elasticsearch/test/WrongName.java b/buildSrc/src/testKit/testingConventions/tests_in_main/src/main/java/org/elasticsearch/gradle/testkit/NamingConventionIT.java similarity index 81% rename from buildSrc/src/testKit/namingConventionsSelfTest/src/test/java/org/elasticsearch/test/WrongName.java rename to buildSrc/src/testKit/testingConventions/tests_in_main/src/main/java/org/elasticsearch/gradle/testkit/NamingConventionIT.java index 64d6a237f8f..48a4f7adfd9 100644 --- a/buildSrc/src/testKit/namingConventionsSelfTest/src/test/java/org/elasticsearch/test/WrongName.java +++ b/buildSrc/src/testKit/testingConventions/tests_in_main/src/main/java/org/elasticsearch/gradle/testkit/NamingConventionIT.java @@ -16,11 +16,8 @@ * specific language governing permissions and limitations * under the License. */ +package org.elasticsearch.gradle.testkit; -package org.elasticsearch.test; +public class NamingConventionIT { -/** - * This class should fail the naming conventions self test. - */ -public class WrongName extends NamingConventionsCheckBadClasses.UnitTestCase { -} +} \ No newline at end of file diff --git a/buildSrc/src/testKit/namingConventionsSelfTest/src/test/java/org/elasticsearch/test/NamingConventionsCheckInMainTests.java b/buildSrc/src/testKit/testingConventions/tests_in_main/src/main/java/org/elasticsearch/gradle/testkit/NamingConventionTests.java similarity index 84% rename from buildSrc/src/testKit/namingConventionsSelfTest/src/test/java/org/elasticsearch/test/NamingConventionsCheckInMainTests.java rename to buildSrc/src/testKit/testingConventions/tests_in_main/src/main/java/org/elasticsearch/gradle/testkit/NamingConventionTests.java index 27c0b41eb3f..6afb89ddf56 100644 --- a/buildSrc/src/testKit/namingConventionsSelfTest/src/test/java/org/elasticsearch/test/NamingConventionsCheckInMainTests.java +++ b/buildSrc/src/testKit/testingConventions/tests_in_main/src/main/java/org/elasticsearch/gradle/testkit/NamingConventionTests.java @@ -16,11 +16,8 @@ * specific language governing permissions and limitations * under the License. */ +package org.elasticsearch.gradle.testkit; -package org.elasticsearch.test; +public class NamingConventionTests { -/** - * This class should fail the naming conventions self test. - */ -public class NamingConventionsCheckInMainTests { } diff --git a/client/rest/build.gradle b/client/rest/build.gradle index a6d8eb8467d..6b22b7b9099 100644 --- a/client/rest/build.gradle +++ b/client/rest/build.gradle @@ -71,12 +71,6 @@ forbiddenApisTest { // TODO: Not anymore. Now in :libs:core jarHell.enabled=false -namingConventions { - testClass = 'org.elasticsearch.client.RestClientTestCase' - //we don't have integration tests - skipIntegTestInDisguise = true -} - testingConventions { naming.clear() naming { diff --git a/client/sniffer/build.gradle b/client/sniffer/build.gradle index 9f2dd73c5c8..382a3f3c9d1 100644 --- a/client/sniffer/build.gradle +++ b/client/sniffer/build.gradle @@ -72,12 +72,6 @@ dependencyLicenses { // TODO: Not anymore. Now in :libs:core jarHell.enabled=false -namingConventions { - testClass = 'org.elasticsearch.client.RestClientTestCase' - //we don't have integration tests - skipIntegTestInDisguise = true -} - testingConventions { naming.clear() naming { diff --git a/client/test/build.gradle b/client/test/build.gradle index e8963b76c5e..f184cfbb73c 100644 --- a/client/test/build.gradle +++ b/client/test/build.gradle @@ -51,8 +51,6 @@ jarHell.enabled=false dependencyLicenses.enabled = false dependenciesInfo.enabled = false -namingConventions.enabled = false - //we aren't releasing this jar thirdPartyAudit.enabled = false unitTest.enabled = false diff --git a/client/transport/build.gradle b/client/transport/build.gradle index 7516e5eb89c..e0292cd5574 100644 --- a/client/transport/build.gradle +++ b/client/transport/build.gradle @@ -47,12 +47,6 @@ forbiddenApisTest { replaceSignatureFiles 'jdk-signatures', 'es-all-signatures' } -namingConventions { - testClass = 'com.carrotsearch.randomizedtesting.RandomizedTest' - //we don't have integration tests - skipIntegTestInDisguise = true -} - testingConventions { naming.clear() naming { diff --git a/distribution/tools/java-version-checker/build.gradle b/distribution/tools/java-version-checker/build.gradle index 48014a42a4d..03ac32d20b7 100644 --- a/distribution/tools/java-version-checker/build.gradle +++ b/distribution/tools/java-version-checker/build.gradle @@ -8,7 +8,6 @@ forbiddenApisMain { } unitTest.enabled = false -namingConventions.enabled = false javadoc.enabled = false loggerUsageCheck.enabled = false jarHell.enabled = false diff --git a/distribution/tools/launchers/build.gradle b/distribution/tools/launchers/build.gradle index 4c7d171663a..b7b12170f66 100644 --- a/distribution/tools/launchers/build.gradle +++ b/distribution/tools/launchers/build.gradle @@ -33,11 +33,6 @@ tasks.withType(CheckForbiddenApis) { replaceSignatureFiles 'jdk-signatures' } -namingConventions { - testClass = 'org.elasticsearch.tools.launchers.LaunchersTestCase' - skipIntegTestInDisguise = true -} - testingConventions { naming.clear() naming { diff --git a/libs/secure-sm/build.gradle b/libs/secure-sm/build.gradle index 97b6652fc12..d9a6e30b83a 100644 --- a/libs/secure-sm/build.gradle +++ b/libs/secure-sm/build.gradle @@ -63,10 +63,6 @@ if (isEclipse) { // JAR hell is part of core which we do not want to add as a dependency jarHell.enabled = false -namingConventions { - testClass = 'junit.framework.TestCase' -} - testingConventions { naming.clear() naming { diff --git a/qa/vagrant/build.gradle b/qa/vagrant/build.gradle index d1ded46d2b6..bd5f3e7a2ac 100644 --- a/qa/vagrant/build.gradle +++ b/qa/vagrant/build.gradle @@ -72,6 +72,8 @@ forbiddenApisMain { // we don't have additional tests for the tests themselves tasks.unitTest.enabled = false +// Tests are destructive and meant to run in a VM, they don't adhere to general conventions +testingConventions.enabled = false // this project doesn't get published tasks.dependencyLicenses.enabled = false diff --git a/test/framework/build.gradle b/test/framework/build.gradle index 26a36852d37..fbc87988837 100644 --- a/test/framework/build.gradle +++ b/test/framework/build.gradle @@ -64,12 +64,6 @@ thirdPartyAudit.ignoreMissingClasses ( 'org.jmock.core.Constraint' ) -task namingConventionsMain(type: org.elasticsearch.gradle.precommit.NamingConventionsTask) { - checkForTestsInMain = true - javaHome = project.compilerJavaHome -} -precommit.dependsOn namingConventionsMain - unitTest { systemProperty 'tests.gradle_index_compat_versions', bwcVersions.indexCompatible.join(',') systemProperty 'tests.gradle_wire_compat_versions', bwcVersions.wireCompatible.join(',') diff --git a/x-pack/qa/openldap-tests/build.gradle b/x-pack/qa/openldap-tests/build.gradle index 8c87f7e6164..6e2c91dff75 100644 --- a/x-pack/qa/openldap-tests/build.gradle +++ b/x-pack/qa/openldap-tests/build.gradle @@ -29,8 +29,3 @@ if (project.rootProject.vagrantSupported) { unitTest.enabled = false testingConventions.enabled = false } - -namingConventions { - // integ tests use Tests instead of IT - skipIntegTestInDisguise = true -} diff --git a/x-pack/transport-client/build.gradle b/x-pack/transport-client/build.gradle index 87a626be65d..90e3f96418f 100644 --- a/x-pack/transport-client/build.gradle +++ b/x-pack/transport-client/build.gradle @@ -23,12 +23,6 @@ forbiddenApisTest { replaceSignatureFiles 'jdk-signatures', 'es-all-signatures' } -namingConventions { - testClass = 'com.carrotsearch.randomizedtesting.RandomizedTest' - //we don't have integration tests - skipIntegTestInDisguise = true -} - testingConventions { naming.clear() naming {