Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
}
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/org/junit/runners/ParentRunner.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
}

Expand Down
39 changes: 39 additions & 0 deletions src/main/java/org/junit/runners/model/InvalidTestClassError.java
Original file line number Diff line number Diff line change
@@ -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;
}
}
Original file line number Diff line number Diff line change
@@ -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 {

Expand All @@ -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() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please note the integration test here. I'm not sure if this is a good place for it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine. Thanks!

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";
}
}
}
1 change: 1 addition & 0 deletions src/test/java/org/junit/runners/model/AllModelTests.java
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
@SuiteClasses({
FrameworkFieldTest.class,
FrameworkMethodTest.class,
InvalidTestClassErrorTest.class,
TestClassTest.class
})
public class AllModelTests {
Expand Down
Original file line number Diff line number Diff line change
@@ -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 {
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)
Expand All @@ -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")));
Expand Down Expand Up @@ -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"),
Expand Down Expand Up @@ -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"),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"));
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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
Expand Down
4 changes: 3 additions & 1 deletion src/test/java/org/junit/tests/validation/ValidationTest.java
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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"));
}
}