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

rename expectThrows() to assertThrows() #1504

Merged
merged 1 commit into from
Feb 17, 2018

Conversation

panchenko
Copy link
Contributor

as suggested again in #1496, Hopefully it helps with having a release sooner.

The important part is making a release, it's not a big deal if there is 1 or 2 methods.

* @return the exception thrown by {@code runnable}
* @since 4.13
*/
public static <T extends Throwable> T expectThrows(Class<T> expectedThrowable, ThrowingRunnable runnable) {
public static <T extends Throwable> T assertThrows(Class<T> expectedThrowable,
ThrowingRunnable runnable, String message) {
Copy link
Member

@kcooney kcooney Feb 15, 2018

Choose a reason for hiding this comment

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

In JUnit 4.x the message comes first in the assertion methods, so perhaps we should do that to be consistent.

Besides consistency, the lambda have often be multiple lines, so I personally find it easier to read if the lambda is the last argument

@kcooney
Copy link
Member

kcooney commented Feb 15, 2018

@rschmitt Now that JUnit5 is out and stable, we wanted to (belatedly) prepare to release 4.13. Since JUnit5 combined assertThrows() and expectThrows() into a single method named assertThrows() (and we haven't heard concerns) it seems reasonable to merge the two methods in 4.13.. Looping you in since you did so much work to get this functionally added to 4.x.

@rschmitt
Copy link
Contributor

@kcooney I think that merging them is fine; it's better to be consistent with JUnit 5.

}

private static String buildPrefix(String message) {
return message != null && message.length() != 0 ? message + " ==> " : "";
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please replace ==> with something unobstrusive like a colon :. Thanks.

@stefanbirkner
Copy link
Contributor

@panchenko Do you have time to squash the commits?

@panchenko
Copy link
Contributor Author

I can, also can be done while merging.

@stefanbirkner
Copy link
Contributor

stefanbirkner commented Feb 16, 2018

I would like to squash and then rebase. Unfortunately this is not supported by GitHub's web UI.

@stefanbirkner stefanbirkner merged commit 627b85a into junit-team:master Feb 17, 2018
@stefanbirkner stefanbirkner added this to the 4.13 milestone Feb 17, 2018
@stefanbirkner
Copy link
Contributor

Merged. Thanks, @panchenko.

@stefanbirkner
Copy link
Contributor

I changed the release notes accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants