-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Tests expecting AssumptionViolatedException should be marked as passed, not skipped #1291
Changes from 7 commits
c71c878
939ade5
6def963
e6aee89
4a79e75
ca669b6
aa582cb
95800b6
0ea179f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,81 @@ | ||
package org.junit.internal.runners.statements; | ||
|
||
import org.junit.Test; | ||
import org.junit.internal.AssumptionViolatedException; | ||
import org.junit.runners.model.Statement; | ||
|
||
import static org.hamcrest.CoreMatchers.containsString; | ||
import static org.hamcrest.CoreMatchers.equalTo; | ||
import static org.junit.Assert.assertThat; | ||
import static org.junit.Assert.fail; | ||
|
||
/** | ||
* Integration tests can be found in {@link org.junit.tests.running.methods.ExpectedTest}. | ||
* See e.g. {@link org.junit.tests.running.methods.ExpectedTest#expectsAssumptionViolatedException()} | ||
*/ | ||
public class ExpectExceptionTest { | ||
|
||
@Test | ||
public void whenExpectingAssumptionViolatedExceptionStatementsThrowingItShouldPass() { | ||
Statement statementThrowingAssumptionViolatedException = new Fail(new AssumptionViolatedException("expected")); | ||
ExpectException expectException = new ExpectException(statementThrowingAssumptionViolatedException, AssumptionViolatedException.class); | ||
|
||
try { | ||
expectException.evaluate(); | ||
// then no exception should be thrown | ||
} catch (Throwable e) { | ||
fail("should not throw anything, but was thrown: " + e); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Edit: Woops! I see why you need to do an explicit catch; without your fix this test would not fail because of the assumption failure exception being thrown. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ya right! Tricky uh? :) Testing JUnit with JUnit is tricky itself |
||
} | ||
} | ||
|
||
@Test | ||
public void whenExpectingAssumptionViolatedExceptionStatementsThrowingSubclassShouldPass() { | ||
Statement statementThrowingAssumptionViolatedExceptionSubclass = new Fail(new org.junit.AssumptionViolatedException("expected")); | ||
ExpectException expectException = new ExpectException(statementThrowingAssumptionViolatedExceptionSubclass, AssumptionViolatedException.class); | ||
|
||
try { | ||
expectException.evaluate(); | ||
// then no exception should be thrown | ||
} catch (Throwable e) { | ||
fail("should not throw anything, but was thrown: " + e); | ||
} | ||
} | ||
|
||
@Test | ||
public void whenExpectingAssumptionViolatedExceptionStatementsThrowingDifferentExceptionShouldFail() { | ||
Statement statementThrowingSomeException = new Fail(new SomeException("not expected")); | ||
ExpectException expectException = new ExpectException(statementThrowingSomeException, AssumptionViolatedException.class); | ||
|
||
try { | ||
expectException.evaluate(); | ||
fail("should throw 'Unexpected exception' when statement throws an exception which is not the one expected"); | ||
} catch (Exception e) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a more specific exception that can be caught? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately not, at the minute. I was going to open a new PR to change the exception thrown in case of unexpected exception to be of a new type |
||
assertThat(e.getMessage(), equalTo("Unexpected exception, expected<org.junit.internal.AssumptionViolatedException> " + | ||
"but was<org.junit.internal.runners.statements.ExpectExceptionTest$SomeException>")); | ||
} | ||
} | ||
|
||
@Test | ||
public void whenExpectingAssumptionViolatedExceptionStatementsPassingShouldFail() throws Exception { | ||
ExpectException expectException = new ExpectException(new PassingStatement(), AssumptionViolatedException.class); | ||
|
||
try { | ||
expectException.evaluate(); | ||
fail("ExpectException should throw when the given statement passes"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This call throws
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the catch block I verify the exception message, so it works, but I agree it's not nice. As in the other comment, the best would be to have the SUT throw a more specific exception. I'm not sure about the name though.. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The idiom.I wrote above is quite common in JUnit's tests, so let's go with that. We can't simply rely on checking the exception message because it also can result in tests passing that shouldn't pass, plus the failure messages you get if the test fails are confusing. I don't think a new exception type is worth it, bu the other maintainers might disagree with me and agree with you :-( Verifying that a method or constructor call throws a specific exception would be easier if we could use lambdas and There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} catch (AssertionError e) { | ||
assertThat(e.getMessage(), containsString("Expected exception: " + AssumptionViolatedException.class.getName())); | ||
} | ||
} | ||
|
||
private static class PassingStatement extends Statement { | ||
public void evaluate() throws Throwable { | ||
// nop | ||
} | ||
} | ||
|
||
private static class SomeException extends RuntimeException { | ||
public SomeException(String message) { | ||
super(message); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to keep the long variable name for the Statement arg then please wrap this line to make it readable:
But I really prefer
delegate
ordelegateStatement
because that is what the role of the Statement is for the sut. The variable is declared one line up, so the long name isn't needed and makes the code harder to scan IMHO.