Merge pull request #623 from pimterry/errors-on-non-static-datapoints-#125

Added validation that all datapoint methods and fields are public and static to theory initialization

Author: David Saff

Diff for: src/main/java/org/junit/experimental/theories/Theories.java

@@ -2,6 +2,7 @@
 
 import java.lang.reflect.Field;
 import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
 import java.util.ArrayList;
 import java.util.List;
@@ -26,13 +27,14 @@ public Theories(Class<?> klass) throws InitializationError {
     protected void collectInitializationErrors(List<Throwable> errors) {
         super.collectInitializationErrors(errors);
         validateDataPointFields(errors);
+        validateDataPointMethods(errors);
     }
 
     private void validateDataPointFields(List<Throwable> errors) {
         Field[] fields = getTestClass().getJavaClass().getDeclaredFields();
 
         for (Field field : fields) {
-            if (field.getAnnotation(DataPoint.class) == null) {
+            if (field.getAnnotation(DataPoint.class) == null && field.getAnnotation(DataPoints.class) == null) {
                 continue;
             }
             if (!Modifier.isStatic(field.getModifiers())) {
@@ -44,6 +46,22 @@ private void validateDataPointFields(List<Throwable> errors) {
         }
     }
 
+    private void validateDataPointMethods(List<Throwable> errors) {
+        Method[] methods = getTestClass().getJavaClass().getDeclaredMethods();
+        
+        for (Method method : methods) {
+            if (method.getAnnotation(DataPoint.class) == null && method.getAnnotation(DataPoints.class) == null) {
+                continue;
+            }
+            if (!Modifier.isStatic(method.getModifiers())) {
+                errors.add(new Error("DataPoint method " + method.getName() + " must be static"));
+            }
+            if (!Modifier.isPublic(method.getModifiers())) {
+                errors.add(new Error("DataPoint method " + method.getName() + " must be public"));
+            }
+        }
+    }
+
     @Override
     protected void validateConstructor(List<Throwable> errors) {
         validateOnlyOneConstructor(errors);

Diff for: src/test/java/org/junit/tests/experimental/theories/runner/UnsuccessfulWithDataPointFields.java

@@ -2,24 +2,26 @@
 
 import static org.hamcrest.CoreMatchers.allOf;
 import static org.hamcrest.CoreMatchers.is;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertThat;
 import static org.junit.experimental.results.PrintableResult.testResult;
 import static org.junit.experimental.results.ResultMatchers.failureCountIs;
 import static org.junit.experimental.results.ResultMatchers.hasFailureContaining;
 import static org.junit.experimental.results.ResultMatchers.hasSingleFailureContaining;
-
 import org.hamcrest.CoreMatchers;
 import org.junit.Test;
 import org.junit.experimental.results.PrintableResult;
 import org.junit.experimental.theories.DataPoint;
+import org.junit.experimental.theories.DataPoints;
 import org.junit.experimental.theories.Theories;
 import org.junit.experimental.theories.Theory;
 import org.junit.runner.RunWith;
 import org.junit.runners.model.TestClass;
 
 public class UnsuccessfulWithDataPointFields {
     @RunWith(Theories.class)
-    public static class HasATheory {
+    public static class HasAFailingTheory {
         @DataPoint
         public static int ONE = 1;
 
@@ -31,19 +33,19 @@ public void everythingIsZero(int x) {
 
     @Test
     public void theoryClassMethodsShowUp() throws Exception {
-        assertThat(new Theories(HasATheory.class).getDescription()
+        assertThat(new Theories(HasAFailingTheory.class).getDescription()
                 .getChildren().size(), is(1));
     }
 
     @Test
     public void theoryAnnotationsAreRetained() throws Exception {
-        assertThat(new TestClass(HasATheory.class).getAnnotatedMethods(
+        assertThat(new TestClass(HasAFailingTheory.class).getAnnotatedMethods(
                 Theory.class).size(), is(1));
     }
 
     @Test
     public void canRunTheories() throws Exception {
-        assertThat(testResult(HasATheory.class),
+        assertThat(testResult(HasAFailingTheory.class),
                 hasSingleFailureContaining("Expected"));
     }
 
@@ -83,75 +85,168 @@ public void nullsUsedUnlessProhibited() throws Exception {
         assertThat(testResult(NullsOK.class),
                 hasSingleFailureContaining("null"));
     }
-
+    
     @RunWith(Theories.class)
-    public static class DataPointsMustBeStatic {
+    public static class TheoriesMustBePublic {
         @DataPoint
-        public int THREE = 3;
+        public static int THREE = 3;
 
-        @DataPoint
-        public int FOUR = 4;
+        @Theory
+        void numbers(int x) {
+
+        }
+    }
+
+    @Test
+    public void theoriesMustBePublic() {
+        assertThat(
+                testResult(TheoriesMustBePublic.class),
+                hasSingleFailureContaining("public"));
+    }    
 
+    @RunWith(Theories.class)
+    public static class DataPointFieldsMustBeStatic {
+        @DataPoint
+        public int THREE = 3;
+        
+        @DataPoints
+        public int[] FOURS = new int[] { 4 };
+        
         @Theory
         public void numbers(int x) {
 
         }
     }
 
     @Test
-    public void dataPointsMustBeStatic() {
+    public void dataPointFieldsMustBeStatic() {
         assertThat(
-                testResult(DataPointsMustBeStatic.class),
+                testResult(DataPointFieldsMustBeStatic.class),
                 CoreMatchers.<PrintableResult>both(failureCountIs(2))
                         .and(
                                 hasFailureContaining("DataPoint field THREE must be static"))
                         .and(
-                                hasFailureContaining("DataPoint field FOUR must be static")));
+                                hasFailureContaining("DataPoint field FOURS must be static")));
     }
-
+    
     @RunWith(Theories.class)
-    public static class TheoriesMustBePublic {
+    public static class DataPointMethodsMustBeStatic {
         @DataPoint
-        public static int THREE = 3;
+        public int singleDataPointMethod() {
+            return 1;
+        }
+        
+        @DataPoints
+        public int[] dataPointArrayMethod() {
+            return new int[] { 1, 2, 3 };
+        }
 
         @Theory
-        void numbers(int x) {
-
+        public void numbers(int x) {
+            
         }
     }
-
+    
     @Test
-    public void theoriesMustBePublic() {
+    public void dataPointMethodsMustBeStatic() {
         assertThat(
-                testResult(TheoriesMustBePublic.class),
-                hasSingleFailureContaining("public"));
+                testResult(DataPointMethodsMustBeStatic.class),
+                CoreMatchers.<PrintableResult>both(failureCountIs(2))
+                .and(
+                        hasFailureContaining("DataPoint method singleDataPointMethod must be static"))
+                .and(
+                        hasFailureContaining("DataPoint method dataPointArrayMethod must be static")));
     }
 
     @RunWith(Theories.class)
-    public static class DataPointsMustBePublic {
+    public static class DataPointFieldsMustBePublic {
         @DataPoint
         static int THREE = 3;
+        
+        @DataPoints
+        static int[] THREES = new int[] { 3 };
 
         @DataPoint
         protected static int FOUR = 4;
+        
+        @DataPoints
+        protected static int[] FOURS = new int[] { 4 };
 
-        @SuppressWarnings("unused")
         @DataPoint
         private static int FIVE = 5;
+        
+        @DataPoints
+        private static int[] FIVES = new int[] { 5 };
 
         @Theory
         public void numbers(int x) {
-
+        	
         }
     }
 
     @Test
-    public void dataPointsMustBePublic() {
-        assertThat(
-                testResult(DataPointsMustBePublic.class),
-                allOf(failureCountIs(3),
-                        hasFailureContaining("DataPoint field THREE must be public"),
-                        hasFailureContaining("DataPoint field FOUR must be public"),
-                        hasFailureContaining("DataPoint field FIVE must be public")));
+    public void dataPointFieldsMustBePublic() {
+        PrintableResult result = testResult(DataPointFieldsMustBePublic.class);        
+        assertEquals(6, result.failureCount());
+
+        assertThat(result,
+                allOf(hasFailureContaining("DataPoint field THREE must be public"),
+                      hasFailureContaining("DataPoint field THREES must be public"),
+                      hasFailureContaining("DataPoint field FOUR must be public"),
+                      hasFailureContaining("DataPoint field FOURS must be public"),
+                      hasFailureContaining("DataPoint field FIVE must be public"),
+                      hasFailureContaining("DataPoint field FIVES must be public")));
+    }
+
+    @RunWith(Theories.class)
+    public static class DataPointMethodsMustBePublic {
+        @DataPoint
+        static int three() {
+            return 3;
+        }
+        
+        @DataPoints
+        static int[] threes() { 
+            return new int[] { 3 };
+        }
+
+        @DataPoint
+        protected static int four() {
+            return 4;
+        }
+        
+        @DataPoints
+        protected static int[] fours() {
+            return new int[] { 4 };
+        }
+
+        @DataPoint
+        private static int five() {
+            return 5;
+        }
+        
+        @DataPoints
+        private static int[] fives() {
+            return new int[] { 5 };
+        }
+
+        @Theory
+        public void numbers(int x) {
+        	
+        }
+    }
+    
+    @Test
+    public void dataPointMethodsMustBePublic() {
+        PrintableResult result = testResult(DataPointMethodsMustBePublic.class);        
+        assertEquals(6, result.failureCount());
+
+        assertThat(result,
+                allOf(hasFailureContaining("DataPoint method three must be public"),
+                      hasFailureContaining("DataPoint method threes must be public"),
+                      hasFailureContaining("DataPoint method four must be public"),
+                      hasFailureContaining("DataPoint method fours must be public"),
+                      hasFailureContaining("DataPoint method five must be public"),
+                      hasFailureContaining("DataPoint method fives must be public")));
     }
 }