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

Ensure exceptions from methodBlock() don't result in unrooted tests #1082

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 13 additions & 4 deletions src/main/java/org/junit/runners/BlockJUnit4ClassRunner.java
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,9 @@
* @since 4.5
*/
public class BlockJUnit4ClassRunner extends ParentRunner<FrameworkMethod> {

private final ConcurrentHashMap<FrameworkMethod, Description> methodDescriptions = new ConcurrentHashMap<FrameworkMethod, Description>();

/**
* Creates a BlockJUnit4ClassRunner to run {@code testClass}
*
Expand All @@ -75,10 +77,17 @@ protected void runChild(final FrameworkMethod method, RunNotifier notifier) {
if (isIgnored(method)) {
notifier.fireTestIgnored(description);
} else {
runLeaf(methodBlock(method), description, notifier);
Statement statement;
try {
statement = methodBlock(method);
}
catch (Throwable ex) {
statement = new Fail(ex);
}
runLeaf(statement, description, notifier);
}
}

/**
* Evaluates whether {@link FrameworkMethod}s are ignored based on the
* {@link Ignore} annotation.
Expand Down Expand Up @@ -390,10 +399,10 @@ private List<org.junit.rules.MethodRule> getMethodRules(Object target) {
protected List<MethodRule> rules(Object target) {
List<MethodRule> rules = getTestClass().getAnnotatedMethodValues(target,
Rule.class, MethodRule.class);

rules.addAll(getTestClass().getAnnotatedFieldValues(target,
Rule.class, MethodRule.class));

return rules;
}

Expand Down
102 changes: 102 additions & 0 deletions src/test/java/org/junit/runners/CustomBlockJUnit4ClassRunnerTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
/*
* Copyright 2015 the original author or authors.
*
* All rights reserved. This program and the accompanying materials are
* made available under the terms of the Eclipse Public License v1.0 which
* accompanies this distribution and is available at
*
* http://www.eclipse.org/legal/epl-v10.html
*/

package org.junit.runners;

import static org.junit.Assert.assertEquals;

import java.util.concurrent.atomic.AtomicInteger;

import org.junit.Ignore;
import org.junit.Test;
import org.junit.runner.Description;
import org.junit.runner.notification.Failure;
import org.junit.runner.notification.RunListener;
import org.junit.runner.notification.RunNotifier;
import org.junit.runners.model.FrameworkMethod;
import org.junit.runners.model.InitializationError;
import org.junit.runners.model.Statement;

/**
* Tests that verify proper behavior for custom runners that extend
* {@link BlockJUnit4ClassRunner}.
*
* @author Sam Brannen
* @since 4.13
*/
public class CustomBlockJUnit4ClassRunnerTest {

@Test
public void exceptionsFromMethodBlockMustNotResultInUnrootedTests() throws Exception {
TrackingRunListener listener = new TrackingRunListener();
RunNotifier notifier = new RunNotifier();
notifier.addListener(listener);

new CustomBlockJUnit4ClassRunner(CustomBlockJUnit4ClassRunnerTestCase.class).run(notifier);
assertEquals("tests started.", 2, listener.testStartedCount.get());
assertEquals("tests failed.", 1, listener.testFailureCount.get());
assertEquals("tests finished.", 2, listener.testFinishedCount.get());
}


@Ignore("This test case is run manually by the enclosing test class")
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need the @Ignore

Copy link
Member Author

Choose a reason for hiding this comment

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

Define "need". ;)

When executing tests via org.junit.tests.AllTests, you are correct: the @Ignore is unnecessary.

However, when executing tests within an IDE such as Eclipse, it is very useful to have such internal test classes disabled by default. In that manner, there are no false negatives when executing all tests within a given package, class, etc. Otherwise, such tests that always fail (intentionally) will produce undesirable noise and distract developers from the task at hand.

For the concrete case of CustomBlockJUnit4ClassRunnerTestCase, all test methods within that class are in fact no-ops and would therefore never fail if executed as is; however, having additional passing tests that provide zero value is well... of zero value.

Copy link
Member

Choose a reason for hiding this comment

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

When I run tests in Eclipse, it doesn't run nested classes. We use nested classes in many of our tests, and I haven't seen us use @Ignore (and, in fact, some of the tests depend on there not being @Ignore present)

public static class CustomBlockJUnit4ClassRunnerTestCase {
@Test public void shouldPass() { /* no-op */ }
@Test public void throwException() { /* no-op */ }
}

/**
* Custom extension of {@link BlockJUnit4ClassRunner} that always throws
* an exception from the {@code methodBlock()} if a test method is named
* exactly {@code "throwException"}.
*/
private static class CustomBlockJUnit4ClassRunner extends BlockJUnit4ClassRunner {

CustomBlockJUnit4ClassRunner(Class<?> testClass) throws InitializationError {
super(testClass);
}

@Override
protected Statement methodBlock(FrameworkMethod method) {
if ("throwException".equals(method.getName())) {
throw new RuntimeException("throwException() test method invoked");
}
return super.methodBlock(method);
}
}

/**
* Simple {@link RunListener} that tracks the number of times that
* certain callbacks are invoked.
*/
private static class TrackingRunListener extends RunListener {

final AtomicInteger testStartedCount = new AtomicInteger();
final AtomicInteger testFailureCount = new AtomicInteger();
final AtomicInteger testFinishedCount = new AtomicInteger();


@Override
public void testStarted(Description description) throws Exception {
testStartedCount.incrementAndGet();
}

@Override
public void testFailure(Failure failure) throws Exception {
testFailureCount.incrementAndGet();
}

@Override
public void testFinished(Description description) throws Exception {
testFinishedCount.incrementAndGet();
}
}

}
2 changes: 2 additions & 0 deletions src/test/java/org/junit/tests/AllTests.java
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import org.junit.runner.notification.ConcurrentRunNotifierTest;
import org.junit.runner.notification.RunNotifierTest;
import org.junit.runner.notification.SynchronizedRunListenerTest;
import org.junit.runners.CustomBlockJUnit4ClassRunnerTest;
import org.junit.runners.Suite;
import org.junit.runners.Suite.SuiteClasses;
import org.junit.runners.model.FrameworkFieldTest;
Expand Down Expand Up @@ -203,6 +204,7 @@
RuleMemberValidatorTest.class,
RuleChainTest.class,
BlockJUnit4ClassRunnerTest.class,
CustomBlockJUnit4ClassRunnerTest.class,
MethodSorterTest.class,
TestedOnSupplierTest.class,
StacktracePrintingMatcherTest.class,
Expand Down