-
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
Extend ExpectedException's documentation and fix #687. #720
Conversation
@@ -199,19 +325,27 @@ private void optionallyHandleException(Throwable e, boolean handleException) | |||
throws Throwable { | |||
if (handleException) { | |||
handleException(e); | |||
} else { | |||
} else if (!isExpectedException(e)) { |
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.
Sorry, missed this, which is the essential line of the pull. Nice.
I wonder how you'd feel about deprecating the explicit handleAssertion and handleAssumption methods, in favor of a single dontWrap(Matcher). (where the default dont-wrap matcher would be the current default of all AssertionErrors and AssumptionExceptions). This has two advantages:
|
I tried the dontWrap method, but unfortunately its makes the API and documentation harder to understand. Therefore I would propose to do without this method. I don't think that there are many people who need this improvement for their JUnit extensions. Do you know an extension, which uses an additional Exception for framework communication? |
I think we should revert 128553f. Whenever you have a test that uses ExpectedException together with I recommend to add this to ExpectedException's documentation and revert the change. @dsaff How to you think about it? |
Works for me, @stefanbirkner. Sorry for the delay in communication. |
@dsaff It's ready. |
* public ExpectedException thrown= ExpectedException.none(); | ||
* <p> | ||
* You can combine any of the presented expect-methods. The test is | ||
* successful if all specifictions are met. |
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.
Check spelling
Looking good. Added some minor changes to the javadoc. |
private String missingExceptionMessage; | ||
|
||
private ExpectedException() { | ||
} | ||
|
||
public ExpectedException handleAssertionErrors() { |
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.
Actually, it's very unusual for us to remove abruptly a method that was already in a public release of JUnit. We should probably leave these in, deprecated, with no implementation and javadoc about how things have changed.
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.
I restored the methods. They now throw an exception in order to notify the user about the deprecation.
@dsaff I'd like to squash the new commits if everything is fine. |
@stefanbirkner, I think it makes more sense to leave them as deprecated no-op methods: people who called them were explicitly requesting the behavior that will now be default, right? |
That's right. But people, who use this methods, are testing code that's throwing AssertionErrors or AssumptionViolatedExceptions. Therefore they have to adjust their tests, because the default behaviour of ExpectedException changed, too. IMHO we have to choose between three alternatives:
I still prefer alternative 3, because it's a good trade-off between lesser code to maintain and upsetting the users. |
Sorry, I'm probably just dense tonight. Given this test:
It works in 4.11, and if handleAssertionErrors is a no-op, it works in 4.12, too, right? |
That's true. I was wrong with the first alternative. Nobody has to change his methods, if we use no-op methods. The correct first alternative is.
This means that we must decide, whether we would like to leave the methods forever or not. |
@stefanbirkner, we don't actually have to decide today whether we would like to leave the methods forever. However, it's generally considered kind to give users at least a revision of deprecation warnings before causing their tests to stop compiling. We should leave them in, deprecated, in this release. If no one complains, we can remove them in the next release. |
Done. Should i prepare the commits for merging? |
Shouldn't we have test cases for the special exception type |
I'm not sure whether we need additional tests, because AssumptionViolationException and AssertionError are now handled like any other exception. Changing the AssertionError behaviour in 4.11 was not an accident but an explicit (bad) decision of mine. |
@stefanbirkner I think we're talking about different changes in 4.11 :-): I'm referring to the bug described in #583 (that also you stumbled across according to your comment in the discussion there :-)).
|
@UrsMetz, did @stefanbirkner's latest test handle your concern? |
@dsaff: my concern was handled by @stefanbirkner's last commit. |
Describe every expect method with a single test.
Still fixes junit-team#121, but the original fix was superfluous. Added documentation about handling assumes and asserts in conjunction with the ExpectedException rule.
Improve readability and avoid duplicate code.
@dsaff I squashed the commits. Everything should be ready for the merge. |
Extend ExpectedException's documentation and fix #687.
Done! Thanks! Can you make a note at https://github.com/junit-team/junit/wiki/4.12-release-notes |
Done. |
No description provided.