Merge pull request #866 from stefanbirkner/verify-public-test-class

Verify public test class

.travis.yml

@@ -1,5 +1,6 @@
 language: java
 jdk:
   - oraclejdk7
+  - oraclejdk8
   - openjdk7
   - openjdk6

src/main/java/org/junit/runners/ParentRunner.java

@@ -6,6 +6,7 @@
 import java.lang.annotation.Annotation;
 import java.lang.reflect.Method;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
@@ -38,6 +39,8 @@
 import org.junit.runners.model.Statement;
 import org.junit.runners.model.TestClass;
 import org.junit.validator.AnnotationsValidator;
+import org.junit.validator.PublicClassValidator;
+import org.junit.validator.TestClassValidator;
 
 /**
  * Provides most of the functionality specific to a Runner that implements a
@@ -54,14 +57,15 @@
  */
 public abstract class ParentRunner<T> extends Runner implements Filterable,
         Sortable {
+    private static final List<TestClassValidator> VALIDATORS = Arrays.asList(
+            new AnnotationsValidator(), new PublicClassValidator());
+
     private final Object fChildrenLock = new Object();
     private final TestClass fTestClass;
 
     // Guarded by fChildrenLock
     private volatile Collection<T> fFilteredChildren = null;
 
-    private final AnnotationsValidator fAnnotationsValidator= new AnnotationsValidator();
-
     private volatile RunnerScheduler fScheduler = new RunnerScheduler() {
         public void schedule(Runnable childStatement) {
             childStatement.run();
@@ -121,7 +125,15 @@ protected void collectInitializationErrors(List<Throwable> errors) {
         validatePublicVoidNoArgMethods(BeforeClass.class, true, errors);
         validatePublicVoidNoArgMethods(AfterClass.class, true, errors);
         validateClassRules(errors);
-        errors.addAll(fAnnotationsValidator.validateTestClass(getTestClass()));
+        applyValidators(errors);
+    }
+
+    private void applyValidators(List<Throwable> errors) {
+        if (getTestClass().getJavaClass() != null) {
+            for (TestClassValidator each : VALIDATORS) {
+                errors.addAll(each.validateTestClass(getTestClass()));
+            }
+        }
     }
 
     /**

src/main/java/org/junit/runners/model/FrameworkMethod.java

@@ -3,7 +3,6 @@
 import java.lang.annotation.Annotation;
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
-import java.lang.reflect.Modifier;
 import java.lang.reflect.Type;
 import java.util.List;
 
@@ -91,9 +90,6 @@ public void validatePublicVoid(boolean isStatic, List<Throwable> errors) {
             String state = isStatic ? "should" : "should not";
             errors.add(new Exception("Method " + fMethod.getName() + "() " + state + " be static"));
         }
-        if (!Modifier.isPublic(getDeclaringClass().getModifiers())) {
-            errors.add(new Exception("Class " + getDeclaringClass().getName() + " should be public"));
-        }
         if (!isPublic()) {
             errors.add(new Exception("Method " + fMethod.getName() + "() should be public"));
         }

src/main/java/org/junit/runners/model/TestClass.java

@@ -7,6 +7,7 @@
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Field;
 import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
@@ -248,6 +249,10 @@ public <T> List<T> getAnnotatedMethodValues(Object test,
         return results;
     }
 
+    public boolean isPublic() {
+        return Modifier.isPublic(fClass.getModifiers());
+    }
+
     public boolean isANonStaticInnerClass() {
         return fClass.isMemberClass() && !isStatic(fClass.getModifiers());
     }

src/main/java/org/junit/validator/AnnotationsValidator.java

@@ -18,7 +18,7 @@
  * 
  * @since 4.12
  */
-public final class AnnotationsValidator {
+public final class AnnotationsValidator implements TestClassValidator {
     private static final List<AnnotatableValidator<?>> VALIDATORS = Arrays.<AnnotatableValidator<?>>asList(
             new ClassValidator(), new MethodValidator(), new FieldValidator());
 

src/main/java/org/junit/validator/PublicClassValidator.java

@@ -0,0 +1,33 @@
+package org.junit.validator;
+
+import static java.util.Collections.emptyList;
+import static java.util.Collections.singletonList;
+
+import java.util.List;
+
+import org.junit.runners.model.TestClass;
+
+/**
+ * Validates that a {@link TestClass} is public.
+ * 
+ * @since 4.12
+ */
+public class PublicClassValidator implements TestClassValidator {
+    private static final List<Exception> NO_VALIDATION_ERRORS = emptyList();
+
+    /**
+     * Validate that the specified {@link TestClass} is public.
+     * 
+     * @param testClass the {@link TestClass} that is validated.
+     * @return an empty list if the class is public or a list with a single
+     *         exception otherwise.
+     */
+    public List<Exception> validateTestClass(TestClass testClass) {
+        if (testClass.isPublic()) {
+            return NO_VALIDATION_ERRORS;
+        } else {
+            return singletonList(new Exception("The class "
+                    + testClass.getName() + " is not public."));
+        }
+    }
+}

src/main/java/org/junit/validator/TestClassValidator.java

@@ -0,0 +1,21 @@
+package org.junit.validator;
+
+import java.util.List;
+
+import org.junit.runners.model.TestClass;
+
+/**
+ * Validates a single facet of a test class.
+ * 
+ * @since 4.12
+ */
+public interface TestClassValidator {
+    /**
+     * Validate a single facet of a test class.
+     * 
+     * @param testClass
+     *            the {@link TestClass} that is validated.
+     * @return the validation errors found by the validator.
+     */
+    public List<Exception> validateTestClass(TestClass testClass);
+}

src/test/java/org/junit/runners/model/TestClassTest.java

@@ -201,4 +201,24 @@ public void hasHashCodeWithoutJavaClass() {
         testClass.hashCode();
         // everything is fine if no exception is thrown.
     }
+
+    public static class PublicClass {
+
+    }
+
+    @Test
+    public void identifiesPublicModifier() {
+        TestClass tc = new TestClass(PublicClass.class);
+        assertEquals("Wrong flag 'public',", true, tc.isPublic());
+    }
+
+    static class NonPublicClass {
+
+    }
+
+    @Test
+    public void identifiesNonPublicModifier() {
+        TestClass tc = new TestClass(NonPublicClass.class);
+        assertEquals("Wrong flag 'public',", false, tc.isPublic());
+    }
 }

src/test/java/org/junit/tests/AllTests.java

@@ -99,8 +99,8 @@
 import org.junit.tests.running.methods.TimeoutTest;
 import org.junit.tests.validation.BadlyFormedClassesTest;
 import org.junit.tests.validation.FailedConstructionTest;
-import org.junit.tests.validation.InaccessibleBaseClassTest;
 import org.junit.tests.validation.ValidationTest;
+import org.junit.validator.PublicClassValidatorTest;
 
 // These test files need to be cleaned. See
 // https://sourceforge.net/pm/task.php?func=detailtask&project_task_id=136507&group_id=15278&group_project_id=51407
@@ -142,7 +142,6 @@
         JUnit38ClassRunnerTest.class,
         SystemExitTest.class,
         JUnitCoreReturnsCorrectExitCodeTest.class,
-        InaccessibleBaseClassTest.class,
         SuiteMethodTest.class,
         BadlyFormedClassesTest.class,
         IgnoreClassTest.class,
@@ -204,7 +203,8 @@
         FrameworkMethodTest.class,
         FailOnTimeoutTest.class,
         JUnitCoreTest.class,
-        TestWithParametersTest.class
+        TestWithParametersTest.class,
+        PublicClassValidatorTest.class
 })
 public class AllTests {
     public static Test suite() {

src/test/java/org/junit/tests/running/classes/ParentRunnerTest.java

@@ -84,7 +84,7 @@ public boolean matchesSafely(List<?> item) {
     }
 
     private static class Exclude extends Filter {
-        private String methodName;
+        private final String methodName;
 
         public Exclude(String methodName) {
             this.methodName = methodName;
@@ -127,6 +127,18 @@ public void failWithHelpfulMessageForNonStaticClassRule() {
                 "The @ClassRule 'temporaryFolder' must be static.");
     }
 
+    static class NonPublicTestClass {
+        public NonPublicTestClass() {
+        }
+    }
+
+    @Test
+    public void cannotBeCreatedWithNonPublicTestClass() {
+        assertClassHasFailureMessage(
+                NonPublicTestClass.class,
+                "The class org.junit.tests.running.classes.ParentRunnerTest$NonPublicTestClass is not public.");
+    }
+
     private void assertClassHasFailureMessage(Class<?> klass, String message) {
         JUnitCore junitCore = new JUnitCore();
         Request request = Request.aClass(klass);

src/test/java/org/junit/validator/PublicClassValidatorTest.java

@@ -0,0 +1,41 @@
+package org.junit.validator;
+
+import static org.hamcrest.CoreMatchers.equalTo;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+import java.util.Collections;
+import java.util.List;
+
+import org.junit.Test;
+import org.junit.runners.model.TestClass;
+
+public class PublicClassValidatorTest {
+    private final PublicClassValidator validator = new PublicClassValidator();
+
+    public static class PublicClass {
+
+    }
+
+    @Test
+    public void acceptsPublicClass() {
+        TestClass testClass = new TestClass(PublicClass.class);
+        List<Exception> validationErrors = validator
+                .validateTestClass(testClass);
+        assertThat(validationErrors,
+                is(equalTo(Collections.<Exception> emptyList())));
+    }
+
+    static class NonPublicClass {
+
+    }
+
+    @Test
+    public void rejectsNonPublicClass() {
+        TestClass testClass = new TestClass(NonPublicClass.class);
+        List<Exception> validationErrors = validator
+                .validateTestClass(testClass);
+        assertThat("Wrong number of errors.", validationErrors.size(),
+                is(equalTo(1)));
+    }
+}