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.

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

Conversation

ghost
Copy link

@ghost ghost commented Apr 25, 2015

Due to #1125.

Several points:

  1. Original code did not notify listeners about StoppedByUserException. I took the same approach for handling this exception with other execution errors - StoppedByUserException will not be delivered to listeners while all other errors will. Though I think listeners should get notification about reason of execution interruption.
  2. Method for unpacking all exceptions looks a little bit helpful and at the moment the same logic is used in EachTestNotifier. I was thinking about moving that method to MultipleFailureException class to make it common and allow usage of one in different places (e.g. writers of custom runners can get benefit from one), but in that case method will become a part of the public API, and I'm not sure that JUnit needs that.
  3. I don't like methods' names, but was unable to find something more appropriate.

…ase of any additional errors coming from @afterclass methods or class rules.

Due to #1125
@@ -363,13 +364,60 @@ public void run(final RunNotifier notifier) {
statement.evaluate();
} catch (AssumptionViolatedException e) {
testNotifier.addFailedAssumption(e);
} catch (StoppedByUserException 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.

@ghost
Copy link
Author

ghost commented Apr 27, 2015

Closing this pull request since original fix was not accepted. Discussion with kcooney helped to figure out better approach to fix the issue.

@kcooney I believe it will be more appropriate if JUnit team will fix this issue when #1118 will be merged successfully.

@ghost ghost closed this Apr 27, 2015
@kcooney
Copy link
Member

kcooney commented Apr 27, 2015

@NarendraPathai good point about tests throwing StoppedByUserException directly. I don't see a reason why someone would have a test do that, but if they did, failing the test is the most reasonable thing to do.

Perhaps it's best if we don't worry about listeners being notified too often (or not often enough) of the StoppedByUserException. The listeners that are tracking the pass/fail status of tests should "notice" that some tests weren't run, and presumably the user would know why.

I hope I didn't push you away with all of the comments. There is no one correct solution to this, and making changes to the core of JUnit without adversely affecting existing code is tricky.

Note that the majority of changes to JUnit aren't made by the JUnit team. The JUnit team reviews changes, do merges, update release notes and do releases, but most functional changes are from contributors like you. If you want to take another stab at this after #1118 is merged, please feel free.

@ghost ghost deleted the stop_request_fix branch April 27, 2015 17:19
@ghost ghost restored the stop_request_fix branch April 27, 2015 17:19
@ghost
Copy link
Author

ghost commented Apr 28, 2015

Sure, I understand. From my point of view, I just think that person who has suggested better approach is the first candidate to implement that approach :-) And if you will not be able to do that, then someone else will be.

@ghost ghost deleted the stop_request_fix branch April 28, 2015 08:23
@kcooney
Copy link
Member

kcooney commented Apr 28, 2015

I think it depends on how difficult it is to implement the "better" approach, how much time everyone has, and how important the feature or fix is.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants