Provide better feedback to the user in case of invalid test classes (#…

…1286)

Only one exception per invalid test class is now fired, rather than one per validation error,
with all of the validation errors as part of its message.

Example:

    org.junit.runners.InvalidTestClassError: Invalid test class 'InvalidTest':
      1. Method staticAfterMethod() should not be static
      2. Method staticBeforeMethod() should not be static

Validation errors for the same test class now count only once in the failure count (Result#getFailureCount).

Implementation:
 - ParentRunner#validate throws InvalidTestClassError in case of validation errors
 - ErrorReportingRunner fires the InvalidTestClassError above without unpacking the causes

src/main/java/org/junit/internal/runners/ErrorReportingRunner.java

@@ -7,6 +7,7 @@
 import org.junit.runner.Runner;
 import org.junit.runner.notification.Failure;
 import org.junit.runner.notification.RunNotifier;
+import org.junit.runners.model.InvalidTestClassError;
 import org.junit.runners.model.InitializationError;
 
 import static java.util.Collections.singletonList;
@@ -65,6 +66,9 @@ private List<Throwable> getCauses(Throwable cause) {
         if (cause instanceof InvocationTargetException) {
             return getCauses(cause.getCause());
         }
+        if (cause instanceof InvalidTestClassError) {
+            return singletonList(cause);
+        }
         if (cause instanceof InitializationError) {
             return ((InitializationError) cause).getCauses();
         }

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

@@ -35,6 +35,7 @@
 import org.junit.runner.notification.StoppedByUserException;
 import org.junit.runners.model.FrameworkMethod;
 import org.junit.runners.model.InitializationError;
+import org.junit.runners.model.InvalidTestClassError;
 import org.junit.runners.model.RunnerScheduler;
 import org.junit.runners.model.Statement;
 import org.junit.runners.model.TestClass;
@@ -418,7 +419,7 @@ private void validate() throws InitializationError {
         List<Throwable> errors = new ArrayList<Throwable>();
         collectInitializationErrors(errors);
         if (!errors.isEmpty()) {
-            throw new InitializationError(errors);
+            throw new InvalidTestClassError(testClass.getJavaClass(), errors);
         }
     }
 

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

@@ -0,0 +1,39 @@
+package org.junit.runners.model;
+
+import java.util.List;
+
+/**
+ * Thrown by {@link org.junit.runner.Runner}s in case the class under test is not valid.
+ * <p>
+ * Its message conveniently lists all of the validation errors.
+ *
+ * @since 4.13
+ */
+public class InvalidTestClassError extends InitializationError {
+    private static final long serialVersionUID = 1L;
+
+    private final String message;
+
+    public InvalidTestClassError(Class<?> offendingTestClass, List<Throwable> validationErrors) {
+        super(validationErrors);
+        this.message = createMessage(offendingTestClass, validationErrors);
+    }
+
+    private static String createMessage(Class<?> testClass, List<Throwable> validationErrors) {
+        StringBuilder sb = new StringBuilder();
+        sb.append(String.format("Invalid test class '%s':", testClass.getName()));
+        int i = 1;
+        for (Throwable error : validationErrors) {
+            sb.append("\n  " + (i++) + ". " + error.getMessage());
+        }
+        return sb.toString();
+    }
+
+    /**
+     * @return a message with a list of all of the validation errors
+     */
+    @Override
+    public String getMessage() {
+        return message;
+    }
+}

src/test/java/org/junit/internal/runners/ErrorReportingRunnerTest.java

@@ -1,6 +1,24 @@
 package org.junit.internal.runners;
 
+import org.junit.After;
+import org.junit.Before;
 import org.junit.Test;
+import org.junit.runner.JUnitCore;
+import org.junit.runner.Result;
+import org.junit.runner.notification.Failure;
+import org.junit.runner.notification.RunNotifier;
+import org.junit.runners.model.InvalidTestClassError;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+
+import static org.hamcrest.CoreMatchers.allOf;
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.CoreMatchers.instanceOf;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.CoreMatchers.startsWith;
+import static org.hamcrest.MatcherAssert.assertThat;
 
 public class ErrorReportingRunnerTest {
 
@@ -23,5 +41,48 @@ public void cannotCreateWithNullClasses() {
     public void cannotCreateWithoutClass() {
         new ErrorReportingRunner(new RuntimeException());
     }
-
+
+    @Test
+    public void givenInvalidTestClassErrorAsCause() {
+        final List<Failure> firedFailures = new ArrayList<Failure>();
+        InvalidTestClassError testClassError = new InvalidTestClassError(TestClassWithErrors.class,
+                Arrays.asList(new Throwable("validation error 1"), new Throwable("validation error 2")));
+        ErrorReportingRunner sut = new ErrorReportingRunner(TestClassWithErrors.class, testClassError);
+
+        sut.run(new RunNotifier() {
+            @Override
+            public void fireTestFailure(Failure failure) {
+                super.fireTestFailure(failure);
+                firedFailures.add(failure);
+            }
+        });
+
+        assertThat(firedFailures.size(), is(1));
+        Throwable exception = firedFailures.get(0).getException();
+        assertThat(exception, instanceOf(InvalidTestClassError.class));
+        assertThat(((InvalidTestClassError) exception), is(testClassError));
+    }
+
+    @Test
+    public void givenInvalidTestClass_integrationTest() {
+        Result result = JUnitCore.runClasses(TestClassWithErrors.class);
+
+        assertThat(result.getFailureCount(), is(1));
+        Throwable failure = result.getFailures().get(0).getException();
+        assertThat(failure, instanceOf(InvalidTestClassError.class));
+        assertThat(failure.getMessage(), allOf(
+                startsWith("Invalid test class '" + TestClassWithErrors.class.getName() + "'"),
+                containsString("\n  1. "),
+                containsString("\n  2. ")
+        ));
+    }
+
+    private static class TestClassWithErrors {
+        @Before public static void staticBeforeMethod() {}
+        @After public static void staticAfterMethod() {}
+
+        @Test public String testMethodReturningString() {
+            return "this should not be allowed";
+        }
+    }
 }

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

@@ -8,6 +8,7 @@
 @SuiteClasses({
         FrameworkFieldTest.class,
         FrameworkMethodTest.class,
+        InvalidTestClassErrorTest.class,
         TestClassTest.class
 })
 public class AllModelTests {

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

@@ -0,0 +1,23 @@
+package org.junit.runners.model;
+
+import org.junit.Test;
+
+import static java.util.Arrays.asList;
+import static org.hamcrest.CoreMatchers.equalTo;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+public class InvalidTestClassErrorTest {
+
+    @Test
+    public void invalidTestClassErrorShouldListAllValidationErrorsInItsMessage() {
+        InvalidTestClassError sut = new InvalidTestClassError(SampleTestClass.class,
+                asList(new Throwable("validation error 1"), new Throwable("validation error 2")));
+
+        assertThat(sut.getMessage(), equalTo("Invalid test class '" + SampleTestClass.class.getName() + "':" +
+                "\n  1. validation error 1" +
+                "\n  2. validation error 2"));
+    }
+
+    private static class SampleTestClass {
+    }
+}

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

@@ -2,10 +2,8 @@
 
 import static org.hamcrest.CoreMatchers.allOf;
 import static org.hamcrest.CoreMatchers.is;
-import static org.junit.Assert.assertEquals;
 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;
@@ -121,11 +119,8 @@ public void numbers(int x) {
     public void dataPointFieldsMustBeStatic() {
         assertThat(
                 testResult(DataPointFieldsMustBeStatic.class),
-                CoreMatchers.<PrintableResult>both(failureCountIs(2))
-                        .and(
-                                hasFailureContaining("DataPoint field THREE must be static"))
-                        .and(
-                                hasFailureContaining("DataPoint field FOURS must be static")));
+                CoreMatchers.both(hasFailureContaining("DataPoint field THREE must be static"))
+                        .and(hasFailureContaining("DataPoint field FOURS must be static")));
     }
 
     @RunWith(Theories.class)
@@ -150,8 +145,7 @@ public void numbers(int x) {
     public void dataPointMethodsMustBeStatic() {
         assertThat(
                 testResult(DataPointMethodsMustBeStatic.class),
-                CoreMatchers.<PrintableResult>both(failureCountIs(2))
-                .and(
+                CoreMatchers.<PrintableResult>both(
                         hasFailureContaining("DataPoint method singleDataPointMethod must be static"))
                 .and(
                         hasFailureContaining("DataPoint method dataPointArrayMethod must be static")));
@@ -186,7 +180,6 @@ public void numbers(int x) {
     @Test
     public void dataPointFieldsMustBePublic() {
         PrintableResult result = testResult(DataPointFieldsMustBePublic.class);        
-        assertEquals(6, result.failureCount());
 
         assertThat(result,
                 allOf(hasFailureContaining("DataPoint field THREE must be public"),
@@ -238,7 +231,6 @@ 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"),

src/test/java/org/junit/tests/experimental/theories/runner/WithUnresolvedGenericTypeVariablesOnTheoryParms.java

@@ -159,7 +159,7 @@ public void forItem(T item) {
     @Test
     public void whereTypeVariablesAbound() {
         PrintableResult result = testResult(TypeVariablesAbound.class);
-        assertThat(result, failureCountIs(7));
+        assertThat(result, failureCountIs(1));
         assertThat(result, hasFailureContaining("unresolved type variable A"));
         assertThat(result, hasFailureContaining("unresolved type variable B"));
         assertThat(result, hasFailureContaining("unresolved type variable C"));

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

@@ -1,6 +1,7 @@
 package org.junit.tests.running.classes;
 
 import static java.util.Arrays.asList;
+import static org.hamcrest.CoreMatchers.allOf;
 import static org.hamcrest.CoreMatchers.containsString;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertThat;
@@ -186,11 +187,11 @@ private int fib(int x) {
     @Test
     public void failureOnInitialization() {
         Result result = JUnitCore.runClasses(BadIndexForAnnotatedFieldTest.class);
-        assertEquals(2, result.getFailureCount());
+        assertEquals(1, result.getFailureCount());
         List<Failure> failures = result.getFailures();
-        assertEquals("Invalid @Parameter value: 2. @Parameter fields counted: 1. Please use an index between 0 and 0.",
-                failures.get(0).getException().getMessage());
-        assertEquals("@Parameter(0) is never used.", failures.get(1).getException().getMessage());
+        assertThat(failures.get(0).getException().getMessage(), allOf(
+                containsString("Invalid @Parameter value: 2. @Parameter fields counted: 1. Please use an index between 0 and 0."),
+                containsString("@Parameter(0) is never used.")));
     }
 
     @RunWith(Parameterized.class)

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

@@ -1,5 +1,6 @@
 package org.junit.tests.running.classes;
 
+import static org.hamcrest.CoreMatchers.containsString;
 import static org.hamcrest.CoreMatchers.equalTo;
 import static org.hamcrest.CoreMatchers.is;
 import static org.junit.Assert.assertEquals;
@@ -149,9 +150,9 @@ private void assertClassHasFailureMessage(Class<?> klass, String message) {
         JUnitCore junitCore = new JUnitCore();
         Request request = Request.aClass(klass);
         Result result = junitCore.run(request);
-        assertThat(result.getFailureCount(), is(2)); //the second failure is no runnable methods
+        assertThat(result.getFailureCount(), is(1));
         assertThat(result.getFailures().get(0).getMessage(),
-                is(equalTo(message)));
+                containsString(message));
     }
 
     public static class AssertionErrorAtParentLevelTest {

src/test/java/org/junit/tests/validation/BadlyFormedClassesTest.java

@@ -1,6 +1,8 @@
 package org.junit.tests.validation;
 
+import static org.hamcrest.CoreMatchers.containsString;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertThat;
 
 import org.junit.Before;
 import org.junit.Test;
@@ -54,7 +56,7 @@ public void constructorException() {
 
     @Test
     public void noRunnableMethods() {
-        assertEquals("No runnable methods", exceptionMessageFrom(NoTests.class));
+        assertThat(exceptionMessageFrom(NoTests.class), containsString("No runnable methods"));
     }
 
     @Test

src/test/java/org/junit/tests/validation/ValidationTest.java

@@ -1,5 +1,7 @@
 package org.junit.tests.validation;
 
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.MatcherAssert.assertThat;
 import static org.junit.Assert.assertEquals;
 
 import org.junit.BeforeClass;
@@ -35,6 +37,6 @@ public void hereBecauseEveryTestClassNeedsATest() {
     @Test
     public void nonStaticBeforeClass() {
         Result result = JUnitCore.runClasses(NonStaticBeforeClass.class);
-        assertEquals("Method before() should be static", result.getFailures().get(0).getMessage());
+        assertThat(result.getFailures().get(0).getMessage(), containsString("Method before() should be static"));
     }
 }