Merge pull request junit-team#764 from ferstl/classrule-in-public-class

Verify that all (base-) classes declaring @classrules are public

Author: David Saff

src/main/java/org/junit/internal/runners/rules/RuleFieldValidator.java

@@ -1,6 +1,7 @@
 package org.junit.internal.runners.rules;
 
 import java.lang.annotation.Annotation;
+import java.lang.reflect.Modifier;
 import java.util.List;
 
 import org.junit.ClassRule;
@@ -70,13 +71,19 @@ public void validate(TestClass target, List<Throwable> errors) {
     }
 
     private void validateMember(FrameworkMember<?> member, List<Throwable> errors) {
+        validatePublicClass(member, errors);
         validateStatic(member, errors);
         validatePublic(member, errors);
         validateTestRuleOrMethodRule(member, errors);
     }
+    
+    private void validatePublicClass(FrameworkMember<?> member, List<Throwable> errors) {
+        if (fStaticMembers && !isDeclaringClassPublic(member)) {
+            addError(errors, member, " must be declared in a public class.");
+        }
+    }
 
-    private void validateStatic(FrameworkMember<?> member,
-            List<Throwable> errors) {
+    private void validateStatic(FrameworkMember<?> member, List<Throwable> errors) {
         if (fStaticMembers && !member.isStatic()) {
             addError(errors, member, "must be static.");
         }
@@ -100,11 +107,14 @@ private void validateTestRuleOrMethodRule(FrameworkMember<?> member,
         }
     }
 
+    private boolean isDeclaringClassPublic(FrameworkMember<?> member) {
+        return Modifier.isPublic(member.getDeclaringClass().getModifiers());
+    }
+
     private boolean isTestRule(FrameworkMember<?> member) {
         return TestRule.class.isAssignableFrom(member.getType());
     }
 
-    @SuppressWarnings("deprecation")
     private boolean isMethodRule(FrameworkMember<?> member) {
         return MethodRule.class.isAssignableFrom(member.getType());
     }

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

@@ -61,6 +61,11 @@ public Field getField() {
     public Class<?> getType() {
         return fField.getType();
     }
+    
+    @Override
+    public Class<?> getDeclaringClass() {
+        return fField.getDeclaringClass();
+    }
 
     /**
      * Attempts to retrieve the value of this field on {@code target}

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

@@ -10,7 +10,7 @@
  */
 public abstract class FrameworkMember<T extends FrameworkMember<T>> {
     /**
-     * Returns the annotations on this method
+     * Returns the annotations on this member
      */
     abstract Annotation[] getAnnotations();
 
@@ -32,4 +32,6 @@ boolean isShadowedBy(List<T> members) {
     public abstract String getName();
 
     public abstract Class<?> getType();
+
+    public abstract Class<?> getDeclaringClass();
 }

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

@@ -84,14 +84,14 @@ public void validatePublicVoidNoArg(boolean isStatic, List<Throwable> errors) {
      * <li>is not static (given {@code isStatic is true}).
      */
     public void validatePublicVoid(boolean isStatic, List<Throwable> errors) {
-        if (Modifier.isStatic(fMethod.getModifiers()) != isStatic) {
+        if (isStatic() != isStatic) {
             String state = isStatic ? "should" : "should not";
             errors.add(new Exception("Method " + fMethod.getName() + "() " + state + " be static"));
         }
-        if (!Modifier.isPublic(fMethod.getDeclaringClass().getModifiers())) {
-            errors.add(new Exception("Class " + fMethod.getDeclaringClass().getName() + " should be public"));
+        if (!Modifier.isPublic(getDeclaringClass().getModifiers())) {
+            errors.add(new Exception("Class " + getDeclaringClass().getName() + " should be public"));
         }
-        if (!Modifier.isPublic(fMethod.getModifiers())) {
+        if (!isPublic()) {
             errors.add(new Exception("Method " + fMethod.getName() + "() should be public"));
         }
         if (fMethod.getReturnType() != Void.TYPE) {
@@ -130,6 +130,14 @@ public Class<?> getType() {
         return getReturnType();
     }
 
+    /**
+     * Returns the class where the method is actually declared
+     */
+    @Override
+    public Class<?> getDeclaringClass() {
+        return fMethod.getDeclaringClass();
+    }
+
     public void validateNoTypeParametersOnArgs(List<Throwable> errors) {
         new NoGenericTypeParametersValidator(fMethod).validate(errors);
     }
@@ -197,7 +205,7 @@ public Annotation[] getAnnotations() {
     public <T extends Annotation> T getAnnotation(Class<T> annotationType) {
         return fMethod.getAnnotation(annotationType);
     }
-    
+
     public List<ParameterSignature> getParameterSignatures() {
         return ParameterSignature.signatures(fMethod);
     }

src/test/java/org/junit/tests/experimental/rules/RuleFieldValidatorTest.java

@@ -47,6 +47,18 @@ public static class TestWithNonStaticClassRule {
         public TestRule temporaryFolder = new TemporaryFolder();
     }
 
+    @Test
+    public void rejectClassRuleInNonPublicClass() {
+        TestClass target = new TestClass(NonPublicTestWithClassRule.class);
+        CLASS_RULE_VALIDATOR.validate(target, errors);
+        assertOneErrorWithMessage("The @ClassRule 'temporaryFolder'  must be declared in a public class.");
+    }
+
+    static class NonPublicTestWithClassRule {
+        @ClassRule
+        public static TestRule temporaryFolder = new TemporaryFolder();
+    }
+
     @Test
     public void acceptNonStaticTestRule() {
         TestClass target = new TestClass(TestWithNonStaticTestRule.class);
@@ -70,7 +82,7 @@ public static class TestWithStaticTestRule {
         @Rule
         public static TestRule temporaryFolder = new TemporaryFolder();
     }
-
+    
     @Test
     public void acceptMethodRule() throws Exception {
         TestClass target = new TestClass(TestWithMethodRule.class);