honeymoose
/
OpenSearch
mirror of https://github.com/honeymoose/OpenSearch.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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.
This commit is contained in:
Nik Everett 2017-04-25 21:11:47 -04:00 committed by GitHub
parent b744dc3bcc
commit fc97e25b56
10 changed files with 255 additions and 95 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
buildSrc/build.gradle
View File

@ -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
}

50
buildSrc/src/main/groovy/org/elasticsearch/gradle/precommit/NamingConventionsTask.groovy
View File

@ -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') }

232
buildSrc/src/main/java/org/elasticsearch/test/NamingConventionsCheck.java
View File

@ -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<Class<?>> integTestsInDisguise = new HashSet<>();
private final Set<Class<?>> notRunnable = new HashSet<>();
private final Set<Class<?>> innerClasses = new HashSet<>();
private final Set<Class<?>> 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<Path>() {
/**
* 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<Class<?>> 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<Path> {
/**
* 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;
}
}
}

26
buildSrc/src/main/java/org/elasticsearch/test/NamingConventionsCheckInMainIT.java Normal file
View File

@ -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 {
}

26
buildSrc/src/main/java/org/elasticsearch/test/NamingConventionsCheckInMainTests.java Normal file
View File

@ -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 {
}

5
test/framework/build.gradle
View File

@ -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

2
test/framework/src/main/java/org/elasticsearch/AnalysisFactoryTestCase.java
View File

@ -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("_(.)");

2
test/framework/src/main/java/org/elasticsearch/test/disruption/LongGCDisruptionTest.java → test/framework/src/test/java/org/elasticsearch/test/disruption/LongGCDisruptionTests.java
View File

@ -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();

0
test/framework/src/main/java/org/elasticsearch/test/disruption/NetworkDisruptionIT.java → test/framework/src/test/java/org/elasticsearch/test/disruption/NetworkDisruptionIT.java
View File

0
test/framework/src/main/java/org/elasticsearch/test/disruption/NetworkDisruptionTests.java → test/framework/src/test/java/org/elasticsearch/test/disruption/NetworkDisruptionTests.java
View File