Skip to content
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

Merged
merged 8 commits into from
Nov 14, 2012
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,5 @@ MaxCore.max
*.iml
*.iws
out
java.hprof.txt
java.hprof.txt
.gitattributes
29 changes: 27 additions & 2 deletions src/main/java/org/junit/rules/ExpectedException.java
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,8 @@ public static ExpectedException none() {
private boolean handleAssumptionViolatedExceptions = false;

private boolean handleAssertionErrors = false;

private String missingExceptionMessage;

private ExpectedException() {
}
Expand All @@ -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
Copy link
Member

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."?

* not throw the expected exception.
* @param providedMessage exception detail message
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter is now called message.

* @return self
*/
public ExpectedException setMissingExceptionMessage(String providedMessage) {
Copy link
Member

Choose a reason for hiding this comment

The 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 withMissingExceptionMessage. That way you can write

thrown.expect(SomeException.class).withMissingExceptionMessage("...");

What do you think?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, since every parameter is provided to its method, how about simply message?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to make sure the new method is not confused with expectMessage. Thus, I think message is too short. How about reportMissingExceptionAs?

Copy link
Author

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

The 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 providedMessage vs. message.

I think reportMissingExceptionAs or reportMissingExceptionWithMessage is more in line with the existing handle… methods. Feel free to go ahead and pick one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dharkness
@coreyjv
@marcphilipp

I vote for simplicity:

withMessage(:String): ExpectedException
withLocalizedMessage(:String): ExpectedException

Now everybody knows that you are referring to Throwable#getMessage() and Throwable#getLocalizedMessage().
Try to be consistent with Java API.

Pls create
ThrowableLocalizedMessageMatcher

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.
What about "NotThrown" or "Uncaught" instead of "Missing". The method would state for specific operation that you expect when something is going to happen when a test is doing wrong; means not throwing an exception results in an error message.

missingExceptionMessage = providedMessage;
Copy link
Member

Choose a reason for hiding this comment

The 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) {
Expand Down Expand Up @@ -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)
Expand All @@ -200,4 +221,8 @@ private void handleException(Throwable e) throws Throwable {
throw e;
}
}

private boolean isMissingExceptionMessageEmpty() {
return missingExceptionMessage == null || missingExceptionMessage.isEmpty();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,8 @@ public static Collection<Object[]> testsWithEventMatcher() {
containsString("exception with cause is <java.lang.NullPointerException: expected cause>"),
containsString("cause was <java.lang.NullPointerException: an unexpected cause>"),
containsString("Stacktrace was: java.lang.IllegalArgumentException: Ack!"),
containsString("Caused by: java.lang.NullPointerException: an unexpected cause")))}
containsString("Caused by: java.lang.NullPointerException: an unexpected cause")))},
{ CustomMessageWithoutExpectedException.class, hasSingleFailureWithMessage(ARBITRARY_MESSAGE) }
});
}

Expand Down Expand Up @@ -368,4 +369,16 @@ public void throwWithCause() {
throw new IllegalArgumentException("Ack!", new NullPointerException("an unexpected cause"));
}
}

public static class CustomMessageWithoutExpectedException {

@Rule
public ExpectedException thrown = ExpectedException.none();

@Test
public void noThrow() {
thrown.expect(IllegalArgumentException.class);
thrown.setMissingExceptionMessage(ARBITRARY_MESSAGE);
}
}
}