Skip to content

Handling of StoppedByUserException will correctly stop execution in case of any additional errors coming from @AfterClass methods or class rules. #1131

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Handling of StoppedByUserException will correctly stop execution in case of any additional errors coming from @AfterClass methods or class rules. #1131

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
54 changes: 51 additions & 3 deletions src/main/java/org/junit/runners/ParentRunner.java
View file
Open in desktop
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.MultipleFailureException;
import org.junit.runners.model.RunnerScheduler;
import org.junit.runners.model.Statement;
import org.junit.runners.model.TestClass;
Expand Down Expand Up @@ -363,13 +364,60 @@ public void run(final RunNotifier notifier) {
statement.evaluate();
} catch (AssumptionViolatedException e) {
testNotifier.addFailedAssumption(e);
Copy link
Member

Choose a reason for hiding this comment

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

Why was the catch block for StoppedByUserException removed?

Copy link
Author

Choose a reason for hiding this comment

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

Now StoppedByUserException is handled in the processExecutionFailures() method, despite single or multiple exceptions came from evaluating class block statement.

} catch (StoppedByUserException e) {
throw e;
} catch (Throwable e) {
testNotifier.addFailure(e);
processExecutionFailures(testNotifier, gatherAllFailures(e));
}
Copy link
Member

Choose a reason for hiding this comment

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

I have a simpler suggestion that still works. We could add a method to RunNotifier called stopRequested() that returns true if pleaseStop() was called. Right here, after calling addFailure(), we would call that method, and if it returns true throw StoppedByUserException

Note that in #1118 someone is proposing adding fireTestSuiteStarted() and fireTestSuiteFinished() to RunNotifier. Those methods would be other good places to throw StoppedByUserException()

Copy link
Author

Choose a reason for hiding this comment

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

I agree, this was one of my first attempt to fix this issue, but returning to my points in the previous conversation - if we will simply pass MultipleFailureException to notifier we will get behavior changes, therefore I decided to unpack all failures manually and notify listeners about all errors except StoppedByUserException.
Other solution can be add filtering inside EachTestNotifier - modify addFailure() method to skip StoppedByUserException and pass all other errors. This can make code even simpler, but again this will lead to behavior changes - such alteration will make JUnit to completely ignore StoppedByUserException thrown from the test method manually, which I believe is not acceptable.

Copy link
Member

Choose a reason for hiding this comment

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

@nikitamerinov So is there a problem with my suggestion to have ParentRumner notice that a stop was requested and throw a new StoppedByUserException (or, better yet, do this in a listener after we add suite start/stop calls?

Copy link
Author

Choose a reason for hiding this comment

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

@kcooney Partially. Your suggestion will prevent other classes from execution, but we still need to do something with notifying clients about StoppedByUserException.

Copy link
Member

Choose a reason for hiding this comment

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

@nikitamerinov if we don't want to notify users about that StoppedByUserException when it is part of a MultiFailureException, you can update EachTestNotifier to do that

Copy link
Author

Choose a reason for hiding this comment

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

As it was said, updating EachTestNotifier with these changes will make JUnit ignoring StoppedByUserException thrown manually from test method.

But, in this case, we can distinguish StoppedByUserException thrown from test method by checking Description.isTest() inside EachTestNotifier. Looks like it can be an appropriate solution.

@kcooney Are we ok with that approach?

}

/**
* Goes through all execution failures trying to find and re-throw
* {@link StoppedByUserException} while adding all other errors to
* test notifier.
*
* @param testNotifier notifier to add failures
* @param failures list of execution failures
*/
private void processExecutionFailures(EachTestNotifier testNotifier,
List<Throwable> failures) {
StoppedByUserException stoppedByUserException = null;

for (Throwable failure : failures) {
if (failure instanceof StoppedByUserException) {
stoppedByUserException = (StoppedByUserException) failure;
} else {
testNotifier.addFailure(failure);
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 a behavior change. You are sending to the notifier each of the failures that was contained in the MultipleFailure exception, when before it was notified once.

It sounds the problem is that a StoppedByUserException wrapped in a MultipleFailureException doesn't cause run(RunNotifier) to exit. Couldn't you check if any of the exceptions inside of the MultipleFailureException is a StoppedByUserException, and if so, throw that StoppedByUserException after sending the MultipleFailureException to the notifier?

Also, where is the MultiFailureException being created that contains the StoppedByUserException? It's possible that code is in error.

Copy link
Author

Choose a reason for hiding this comment

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

This is a behavior change. You are sending to the notifier each of the failures ...

Behavior change from what point of view? Before and after my changes no clients are getting multiple exceptions at once. As I said in item 2 in my pull request message, class EachTestNotifier does the same as I do - it unpacks all exceptions and notifies clients about them one by one.

It sounds the problem is that a StoppedByUserException wrapped ...

Sending MultipleFailureException to notifier before finding StoppedByUserException will lead to behavior changes. Clients will get notifications about StoppedByUserException what is never happened before. My first item in the pull request message was about that. If we are ok with these behavior changes, then it can simplify the code.

Also, where is the MultiFailureException being created that contains ...

You can get full example from #1125 or check new test I've added. Basically, StoppedByUserException is combined with other exceptions while evaluating 'after' methods. I don't think that this is an error, since JUnit guarantees to invoke all 'after' methods in case of user stop request and if any of 'after' methods will throw exception I believe clients should get notification about them.

}
}

if (stoppedByUserException != null) {
throw stoppedByUserException;
}
}

/**
* Returns all failures from the specified exception going through each
* encountered {@link MultipleFailureException} recursively.
*
* @param throwable input exception
* @return list of all failures
*/
private List<Throwable> gatherAllFailures(Throwable throwable) {
List<Throwable> result = new ArrayList<Throwable>();

if (throwable instanceof MultipleFailureException) {
MultipleFailureException multipleFailureException =
(MultipleFailureException) throwable;

for (Throwable failure : multipleFailureException.getFailures()) {
result.addAll(gatherAllFailures(failure));
}
} else {
result.add(throwable);
}

return result;
}

//
// Implementation of Filterable and Sortable
//
Expand Down
88 changes: 83 additions & 5 deletions src/test/java/org/junit/tests/listening/UserStopTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,41 @@
package org.junit.tests.listening;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.fail;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;

import org.junit.AfterClass;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.ClassRule;
import org.junit.Test;
import org.junit.runner.Request;
import org.junit.runner.RunWith;
import org.junit.runner.notification.Failure;
import org.junit.runner.notification.RunListener;
import org.junit.runner.notification.RunNotifier;
import org.junit.runner.notification.StoppedByUserException;
import org.junit.runners.Suite;
import org.junit.runners.Suite.SuiteClasses;

public class UserStopTest {
private RunNotifier fNotifier;
private RunNotifier runNotifier;
private static final List<String> invocations = new ArrayList<String>();

@Before
public void createNotifier() {
fNotifier = new RunNotifier();
fNotifier.pleaseStop();
invocations.clear();

runNotifier = new RunNotifier();
runNotifier.pleaseStop();
}

@Test(expected = StoppedByUserException.class)
public void userStop() {
fNotifier.fireTestStarted(null);
runNotifier.fireTestStarted(null);
}

public static class OneTest {
Expand All @@ -28,6 +46,66 @@ public void foo() {

@Test(expected = StoppedByUserException.class)
public void stopClassRunner() throws Exception {
Request.aClass(OneTest.class).getRunner().run(fNotifier);
Request.aClass(OneTest.class).getRunner().run(runNotifier);
}

@RunWith(Suite.class)
@SuiteClasses({TestClassWithExceptionInAfterClass.class,
TestClassThatShouldNotBeExecuted.class})
public static class SuiteExample {}

public static class TestClassWithExceptionInAfterClass {
@Test
public void test() {}

@AfterClass
public static void afterClass() {
throw new RuntimeException("Exception");
}
}

public static class TestClassThatShouldNotBeExecuted {
@BeforeClass
public static void beforeClass() {
invocations.add("beforeClass");
}

@Test
public void test() {
invocations.add("test");
}

@AfterClass
public static void afterClass() {
invocations.add("afterClass");
}
}

/**
* Verifies that stop request is properly handled for suite execution in
* case of additional execution errors coming from {@link AfterClass}
* method or {@link ClassRule}, i.e.: <br />
* - no methods from subsequent classes should be executed <br />
* - listeners still should be notified about execution errors except
* {@link StoppedByUserException}.
*/
@Test
public void testHandlingStopRequestWithAdditionalExecutionErrors() {
final List<String> failures = new ArrayList<String>();

runNotifier.addListener(new RunListener() {
@Override
public void testFailure(Failure failure) throws Exception {
failures.add(failure.getMessage());
}
});

try {
Request.aClass(SuiteExample.class).getRunner().run(runNotifier);
fail();
} catch (StoppedByUserException e) {
assertEquals(0, invocations.size());
assertEquals(Arrays.asList("Exception"), failures);
}
}
}