Ensure exceptions from methodBlock() don't result in unrooted tests #1082
Conversation
|
Thanks? Could you add a test? |
|
Sure. I just introduced |
| * Simple {@link RunListener} which tracks how many times certain JUnit callback | ||
| * methods were called: only intended for the integration test suite. | ||
| * <p>This code has been copied directly from the Spring Framework. | ||
| * @author Sam Brannen |
There was a problem hiding this comment.
Are you the author of this class in the Spring Framework? Otherwise, this might be a bit fishy license-wise, right?
There was a problem hiding this comment.
I am in fact the author of the code in question, but as far as I understand that is irrelevant since the ASL license header is included in the file along with the @author tags.
Now, I am not a lawyer, but I don't think there is any issue here since:
- The original ASL 2.0 license header is present in the file I submitted.
- The code in question is not distributed per se with JUnit JARs; it resides in the test suite.
There was a problem hiding this comment.
But JUnit isn't released under ASL. Could we use a simpler runner where each of the methods appends a hard-coded string to an ArrayList, then you can do assertions on the list.
There was a problem hiding this comment.
I have rewritten the TrackingRunListener from scratch. Any remaining similarities to code in Spring's testing suite are therefore coincidental due to the nature of the task at hand.
|
I think you manually need to add the test to the test suite in order for maven to run the test |
|
OK. I added |
| /* | ||
| * Copyright 2002-2015 the original author or authors. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); |
There was a problem hiding this comment.
JUnit isn't licensed under the Apache License. Please remove this entire comment block
There was a problem hiding this comment.
This has been replaced with a license header compliant with the Eclipse Foundation.
There was a problem hiding this comment.
We don't have copyright and licenses in any of the other files that I'm aware of. I'll remove it when I merge this.
The introduction of the runLeaf() method in BlockJUnit4ClassRunner in JUnit 4.9 introduced a regression with regard to exception handling. Specifically, the invocation of methodBlock() is no longer executed within a try-catch block as was the case in previous versions of JUnit. Custom modifications to methodBlock() or the methods it invokes may in fact throw exceptions. In such cases, exceptions thrown from methodBlock() cause the current test execution to abort immediately. As a result, the failing test method is unrooted in test reports, and subsequent test methods are never invoked. Furthermore, RunListeners registered with JUnit are not notified. This commit addresses this issue by wrapping the invocation of methodBlock() within a try-catch block. If an exception is not thrown, the resulting Statement is passed to runLeaf(). If an exception is thrown, it is wrapped in a Fail statement which is passed to runLeaf(). Issue: junit-team#1066
|
Thanks for the changes. I think the remaining (quite minor) issues can be resolved when we merge this. I'll try to find time Tuesday night to do that. |
|
Which Tuesday? ;-) |
|
Merged. Thanks! Sorry for the delay; both home and work life have been busy for me |
|
Great! Thanks for merging it. |
|
@sbrannen do you mind updating the JUnit 4.13 release notes to mention this pull? |
|
Mentioned this pull request in the release notes here. |
The introduction of the runLeaf() method in BlockJUnit4ClassRunner in
JUnit 4.9 introduced a regression with regard to exception handling.
Specifically, the invocation of methodBlock() is no longer executed
within a try-catch block as was the case in previous versions of JUnit.
Custom modifications to methodBlock() or the methods it invokes may in
fact throw exceptions. In such cases, exceptions thrown from
methodBlock() cause the current test execution to abort immediately. As
a result, the failing test method is unrooted in test reports, and
subsequent test methods are never invoked. Furthermore, RunListeners
registered with JUnit are not notified.
This commit addresses this issue by wrapping the invocation of
methodBlock() within a try-catch block. If an exception is not thrown,
the resulting Statement is passed to runLeaf(). If an exception is
thrown, it is wrapped in a Fail statement which is passed to runLeaf().
Issue: #1066