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

Adds fail() methods that pass Throwables - closes #656. #661

Merged

Conversation

smoyer64
Copy link
Contributor

@smoyer64 smoyer64 commented Feb 17, 2017

Overview

This PR adds two additional signatures for the Assertions.fail() methods:

  • Assertions.fail(String message, Throwable cause)
  • Assertions.fail(Throwable cause)

I hereby agree to the terms of the JUnit Contributor License Agreement.


Definition of Done

Sorry, something went wrong.

@smoyer64 smoyer64 changed the title Added fail() methods that pass Throwables - closes #656. Adds fail() methods that pass Throwables - closes #656. Feb 17, 2017
@@ -36,6 +36,14 @@ static void fail(String message) {
fail(() -> message);
}

static void fail(String message, Throwable cause) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is quite a new use of fail to me, so I think the method would be better named failWithCause or something along those lines to help prevent confusion.

throw new AssertionFailedError(message, cause);
}

static void fail(Throwable cause) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

* <em>Fails</em> a test with the given failure {@code message} as well
* as the underlying {@code cause}.
*/
public static void fail(String message, Throwable cause) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

/**
* <em>Fails</em> a test with the given underlying {@code cause}.
*/
public static void fail(Throwable cause) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

@smoyer64
Copy link
Contributor Author

@jbduncan I'm open to whatever names the group feels comfortable with ... there was quite a bit of discussion on issue #655 before I created #656. My preference was to make a conditional assertion called something like assertNotThrowing() but ultimately agreed that creating these fail() provided the convenience I felt was needed. If you look at AssertionFailedError, there are plenty of constructors in the underlying class.

So ... what does the team want?

@@ -80,6 +80,7 @@ on GitHub.
* `Assertions.assertThrows()` now uses canonical names for exception types when
generating assertion failure messages.
* `TestInstancePostProcessors` registered on test methods are now invoked.
* There are two new signatures `Assertions.fail` - `Assertions.fail(String message, Throwable cause)` and `Assertions.fail(Throwable cause)`.
Copy link
Member

Choose a reason for hiding this comment

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

Please manually wrap lines with more than 100 characters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've amended the commit which updated the release notes.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! 👍

@marcphilipp
Copy link
Member

The core team is going to meet this Wednesday. We'll discuss it then and let you know.

@smoyer64 smoyer64 force-pushed the issues/656-fail-assertions-with-throwable branch from 0bbf74e to 42757a9 Compare February 18, 2017 17:27
@marcphilipp marcphilipp merged commit 23367cb into junit-team:master Feb 22, 2017
@smoyer64 smoyer64 deleted the issues/656-fail-assertions-with-throwable branch February 22, 2017 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants