Pull request for issue #144 #542
Conversation
Added additional public method, setMissingExceptionMessage, to allow a user to specify a custom message that would be thrown in the event a test did not throw the expected exception. If one is not provided the class falls back to the previous behavior.
The test method name was never updated. Updated the test method name to be consistent with what the test was executing.
| * @return self | ||
| */ | ||
| public ExpectedException setMissingExceptionMessage(String providedMessage) { | ||
| missingExceptionMessage = providedMessage; |
There was a problem hiding this comment.
The indentation looks off here and in the other modified paces, it should be 4 spaces.
|
@coreyjv Looks good! I made a few comments inline. |
|
Since I'm still new to this do you want me to make those adjustments or is that something you do in the merge? As far as the indentation I'm not sure what the problem is because I'm using the Eclipse project settings provided. Are there any in particular you'd like me to check? Thanks! |
|
You don't have to submit another request as you can simply make Unfortunately, the formatter setting are still not checked in, there |
Changed method name from setMissingExceptionMessage to reportMissingExceptionWithMessage to be more consistent with the existing API.
Converted tabs to spaces for newly added code to be in line with guidelines.
|
@marcphilipp, I believe I've made all the requested changes. Please let me know if you need anything else. Thanks! |
| /** | ||
| * 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.
The parameter is now called message.
|
@coreyjv Thanks! I've added two Javadoc-related comments inline. |
|
@marcphilipp, I apologize for the Javadoc oversight. It should be all set now. |
|
Would you please again check the name of the method? Above i asked you if we can rename it to something like notifyUncaughtExceptionMessage() or notifyNotThrownExceptionMessage() Instead of "missing" is supposed to use "NotThrown" or "Uncaught" matching Java API vocabulary. Instead of "report" we use in our API "fire" or "notify", in Java API Logger is "log" or "record". When you say notify, you do not have to say Message nor WithMessage, because implicitly can be notified only by a message/event. thx |
| fail("Expected test to throw " + expectation); | ||
| String failureMessage; | ||
|
|
||
| if ( isMissingExceptionMessageEmpty() ) { |
There was a problem hiding this comment.
No extra spaces inside if(), please
|
@Tibor17, the current name seems fine to me. |
|
@dsaff I'll try and get those changes done and committed this evening. |
Refactored message decision logic out of failDueToMissingException into its own method missingExceptionMessage.
|
@dsaff the requested changes are complete. |
|
Thanks! |
|
@coreyjv, would you mind also updating the wiki at https://github.com/KentBeck/junit/wiki/Exception-testing to mention this new feature? Thanks! |
|
@coreyjv @dsaff Merging this pull request broke the build: https://junit.ci.cloudbees.com/job/JUnit/9/console I think @coreyjv Do you have time to fix that? |
|
We should probably state in the wiki that this is unreleased functionality, right? |
|
I'll fix it this evening. I'm sorry about that. I tested the build locally Thanks! On Nov 14, 2012, at 16:14, Marc Philipp notifications@github.com wrote: @coreyjv https://github.com/coreyjv @dsaff I think String.isEmpty() has been added in JDK6. We are still trying to @coreyjv https://github.com/coreyjv Do you have time to fix that? — |
|
I'll add a note about unreleased functionality this evening as well. On Nov 14, 2012, at 16:17, Marc Philipp notifications@github.com wrote: We should probably state in the wiki that this is unreleased functionality, — |
|
@coreyjv The Eclipse project does have JDK5 set as execution environment and compiler compliance level. However, if you use a newer JDK, Eclipse will warn you, but you won't get any compile errors as long as you do not use any language features of JDK6 or JDK7. API changes such as |
|
@marcphilipp see pull request: https://github.com/KentBeck/junit/pull/548 |
|
@marcphilipp I've updated the wiki as well. |
Fixed a JDK5 backwards compatibility issue introduced in Pull #542
|
So I looked at the wiki, and decided that, although I want to keep the docs current, spreading around unreleased functionality might be more confusing than helpful. So I moved your comment to the 4.12 unrelease notes: https://github.com/KentBeck/junit/wiki/4.12-release-notes. Hopefully it won't be too hard for one of us to move it back when we release. |
Added additional public method,
reportMissingExceptionWithMessage, toExpectedExceptionrule to allow athe specification of a custom message that would be thrown in the event a
test did not throw the expected exception. If one is not provided the
class falls back to the previous behavior.