Conversation
| public void after() | ||
| throws Exception | ||
|
|
||
| @Test |
There was a problem hiding this comment.
Why not use the expected property in the annotation?
There was a problem hiding this comment.
@Expected was a bad design now replaced in JUnit 4.13 with assertThrows. The problem is that @Expected captures unexpected exceptions thrown from different parts of the method body and thus hides test failures.
There was a problem hiding this comment.
You can use ExpectedException which is the Rule in JUnit4.
There was a problem hiding this comment.
Same issue. This is not a full replacement for the pattern below and risks false negatives. See https://github.com/junit-team/junit4/wiki/Exception-testing
There was a problem hiding this comment.
@elharo
I am a practicle man. You can of course use [1] with anonymous class instead of Lambda but you can use a traditional ExpectedException with the old version of JUnit.
[1]: https://junit.org/junit4/javadoc/4.13/org/junit/Assert.html#assertThrows(java.lang.String,%20java.lang.Class,%20org.junit.function.ThrowingRunnable)
import org.junit.rules.ExpectedException;
import static org.hamcrest.Matchers.equalTo;
@Rule
public final ExpectedException e = ExpectedException.none();
@Test
public void testNoStartTag() throws IOException
{
e.expect( IllegalArgumentException.class );
e.expectMessage( equalTo( "Element name cannot be empty" ) );
writer.startElement( "" );
}
There was a problem hiding this comment.
That has the same problem. It does not guarantee the expected exception was thrown by the right method. JUnit has rightly backed away from this style of testing exceptions.
There was a problem hiding this comment.
@elharo
In practice the last line with main code must be followed by e.expect. So yes there is guarantee. It is clear that only that one line has to throw the exception. If it does not, then the ExpectedException fails the build. That's why it was designed in JUnit4. You can use the new assertThrows but it has another meaning - wrapping the exception and checking a new errors or exceptions.
There was a problem hiding this comment.
This works and it follows the recommendation of the JUnit Wiki and the design of JUnit from 4.13 onward. I'd like to move ahead with fixing this bug. Can I get an approval for this?
|
@elharo Is this PR still valid and needed? We're looking to release maven-shared-utils, and I'm wondering if we should include this. |
|
Yes, I believe this PR is still valid and useful though it looks like I need to merge in master. Give me a minute. |
| public void after() | ||
| throws Exception | ||
|
|
||
| @Test |
There was a problem hiding this comment.
This works and it follows the recommendation of the JUnit Wiki and the design of JUnit from 4.13 onward. I'd like to move ahead with fixing this bug. Can I get an approval for this?
|
@elharo Hey, you got your approval, but we can't merge anymore due to conflicts. Can you please rebase on master and squash commits? |
@Tibor17 Likely not a full fix, but it should help locate the root cause.