-
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
Pull request for issue #144 #542
Changes from 2 commits
8f0902e
3efad73
a2ca184
bcf2c17
3791a96
b521f0c
467dd07
2246c5a
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 |
---|---|---|
|
@@ -9,4 +9,5 @@ MaxCore.max | |
*.iml | ||
*.iws | ||
out | ||
java.hprof.txt | ||
java.hprof.txt | ||
.gitattributes |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -96,6 +96,8 @@ public static ExpectedException none() { | |
private boolean handleAssumptionViolatedExceptions = false; | ||
|
||
private boolean handleAssertionErrors = false; | ||
|
||
private String missingExceptionMessage; | ||
|
||
private ExpectedException() { | ||
} | ||
|
@@ -109,6 +111,17 @@ public ExpectedException handleAssumptionViolatedExceptions() { | |
handleAssumptionViolatedExceptions = true; | ||
return this; | ||
} | ||
|
||
/** | ||
* Specifies the detail message for an exception to be thrown if the test does | ||
* not throw the expected exception. | ||
* @param providedMessage exception detail message | ||
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 parameter is now called |
||
* @return self | ||
*/ | ||
public ExpectedException setMissingExceptionMessage(String providedMessage) { | ||
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. Since it's a builder-style method, I would prefer it to be called thrown.expect(SomeException.class).withMissingExceptionMessage("..."); What do you think? 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. Also, since every parameter is provided to its method, how about simply 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. We have to make sure the new method is not confused with 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. @marcphilipp, maybe it's too early in my time zone but I believe @dharkness was referring to the parameter name. If not I apologize. What you prefer the method name reportMissingExceptionAs or withMissingExceptionMessage? 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. Maybe it's too early in my time zone… you are certainly right regarding I think 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. @dharkness I vote for simplicity: withMessage(:String): ExpectedException Now everybody knows that you are referring to Throwable#getMessage() and Throwable#getLocalizedMessage(). Pls create 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. @Tibor17 In fact, we are not referring to Throwable#getMessage() and Throwable#getLocalizedMessage() at all. This pull request is about the failure message that is passed to Assert#fail() when no exception is thrown by the code under test but one expects an exception. 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. Now i see. thx for clarifying. Maybe now it's the time to re-think of the method name which would tell me as a user of what "missing" means. |
||
missingExceptionMessage = providedMessage; | ||
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 indentation looks off here and in the other modified paces, it should be 4 spaces. |
||
return this; | ||
} | ||
|
||
public Statement apply(Statement base, | ||
org.junit.runner.Description description) { | ||
|
@@ -180,8 +193,16 @@ public void evaluate() throws Throwable { | |
} | ||
|
||
private void failDueToMissingException() throws AssertionError { | ||
String expectation = StringDescription.toString(fMatcherBuilder.build()); | ||
fail("Expected test to throw " + expectation); | ||
String failureMessage; | ||
|
||
if ( isMissingExceptionMessageEmpty() ) { | ||
String expectation = StringDescription.toString(fMatcherBuilder.build()); | ||
failureMessage = "Expected test to throw " + expectation; | ||
} else { | ||
failureMessage = missingExceptionMessage; | ||
} | ||
|
||
fail(failureMessage); | ||
} | ||
|
||
private void optionallyHandleException(Throwable e, boolean handleException) | ||
|
@@ -200,4 +221,8 @@ private void handleException(Throwable e) throws Throwable { | |
throw e; | ||
} | ||
} | ||
|
||
private boolean isMissingExceptionMessageEmpty() { | ||
return missingExceptionMessage == null || missingExceptionMessage.isEmpty(); | ||
} | ||
} |
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.
How about "Specifies the failure message for tests that are expected to throw an exception but do not throw any."?