Junit5: Deprecate expectThrows in favor of assertThrows returning the exception

[cpovirk]

Is it possible to rename expectThrows?

As we introduce people to expectThrows, we keep hearing:

• (almost always) This is great.
• (occasionally) This isn't what "expect" usually means.

I (and the people I'm hearing from) seem to be used to to "expect" as meaning: "If this fails, record the failure in a list. When the test fails, show all the failures." For example: https://github.com/google/googletest/blob/master/googletest/docs/Primer.md#assertions

Additionally, the "assert" vs. "expect" naming doesn't seem suggest "one of these returns the exception, and the other doesn't" to us.

I don't know if this is still open to discussion, but if it is, my first attempt would be...

    SomeException caught =
        exceptionThrownBy(SomeException.class, () -> { /* code */ });

...since it returns the exception thrown by the code. I'm happy to fish around for suggestions from others if it would be worthwhile, but I figured I should check before putting significant time into this. Thanks.

[smoyer64]

The @Rule available in JUnit 4.12 is called ExpectedException. I guess I never really thought about the method name "expectThrows" since it's so close.

[sbrannen]

TBH, I've personally never been a fan of the expectThrows method in the Assertions class.

Aside from the two fail methods in Assertions, expectThrows is the only method not named assert*.

Proposal

If it were up to me, I would simply delete the existing assertThrows method and rename expectThrows to assertThrows.

Rationale:

1. Assertions should _assert_ something.
2. We don't need two methods that basically do the same thing. (note that assertThrows delegates to expectThrows anyway).
3. If users want to do something with the caught exception, they can; otherwise, they can simply ignore the return value.
4. The confusion that users are confronted with due to the presence of assertThrows and expectThrows is unwarranted.

@junit-team/junit-lambda, what do the rest of you think about my proposal?

[mlevison]

Sam - Not a team member, but as a user and someone who educates others this
an nice tidy up. One fewer exceptions to the rule. Less cognitive load.

Cheers
Mark just off a red eye

On Oct 1, 2016 9:28 AM, "Sam Brannen" [email protected] wrote:

TBH, I've personally never been a fan of the expectThrows method in the
Assertions class.

Aside from the two fail methods in Assertions, expectThrows is the only
method not named assert*.
Proposal

If it were up to me, I would simply delete the existing assertThrows
method and rename expectThrows to assertThrows.

Rationale:

1. Assertions should _assert_ something.
2. We don't need two methods that basically do the same thing. (note
   that assertThrows delegates to expectThrows anyway).
3. If users want to do something with the caught exception, they can;
   otherwise, they can simply ignore the return value.
4. The confusion that users are confronted with due to the presence of
   assertThrows and expectThrows is unwarranted.

@junit-team/junit-lambda
https://github.com/orgs/junit-team/teams/junit-lambda, what do the rest
of you think about my proposal?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/junit-team/junit5/issues/531#issuecomment-250912498,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAImZlfj6cOswSUHjOfzI6veppwhljQjks5qvl_rgaJpZM4KLiHM
.

[bechte]

I have also been stumbled upon those methods being confused. Totally agree. 👍

[marcphilipp]

I only remember there being some argument about some static analysis tools (I don't remember which) that would give you warnings when calling a method that returns an exception which you don't consume. IIRC that was the reason a void variant was introduced and, since overloading was not an option, a different name was used for the method that is now called expectThrows probably to sound similiar to ExpectedException. At the time that seemed like a reasonable compromise to me. However, I can see the benefit in @sbrannen's proposal so I wouldn't veto if we decide to follow it.

[sbrannen]

Thanks for chiming in guys.

@marcphilipp, yep, I believe you are correct that the second method was added in order to avoid warnings, but I also don't recall which IDE/tool emits warnings for an used/unassigned return value.

@mmerdes, do you also agree with my proposal?

[jbduncan]

@sbrannen, I know that IntelliJ and Google's error-prone compiler emit warnings if the return values of methods annotated with @CheckReturnValue are not assigned to a variable.

Might one of both of these be the offender(s) that prompted the existence of expectThrows?

[sormuras]

IntelliJ reports the following warning: Result of 'expectThrows()' not thrown -- I just remove the check mark here.

[sbrannen]

@sormuras, thanks for sharing the screenshot.

I'm guessing there's a way to disable that in IntelliJ as well -- yes?

I'm also guessing that IntelliJ IDEA could be improved to _not_ report a warning for such methods in assertion utilities. 😉

@akozlova, what are your thoughts?

[sbrannen]

@jbduncan, it looks like Goggle's error-prone will address that here: https://github.com/google/error-prone/issues/452

[sormuras]

I'm guessing there's a way to disable that in IntelliJ as well -- yes?

Yes. See second shot linked above.

[sbrannen]

For anyone interested, the _source_ of expectThrows can be seen here:

• https://github.com/junit-team/junit4/pull/1153
• https://github.com/junit-team/junit4/pull/1154

[cpovirk]

I know that IntelliJ and Google's error-prone compiler emit warnings if the return values of methods annotated with @CheckReturnValue are not assigned to a variable.

Google's Error Prone cares only about methods with @CheckReturnValue, not with other methods that return Throwable (in contrast, it appears, to IntelliJ IDEA).

It looks like Goggle's error-prone will address that here: google/error-prone#452

That bug report was to address a slightly different problem:

IndexOutOfBoundsException thrown = expectThrows(
    IndexOutOfBoundsException.class,
    () -> {
      list.get(99); // problem reported here
    });

The idea is that a method like List.get would be annotated @CheckReturnValue, so Error Prone would produce an error for its unused return value. The bug report requested that we disable @CheckReturnValue checking for the last statement inside the ThrowingRunnable method/lambda. (And Error Prone has made the change, though I doubt it's in a release yet.)

Error Prone already doesn't mind if you ignore the return value of expectThrows. So this is fine:

expectThrows(
    UnsupportedOperationException.class,
    () -> {
      list.clear();
    });

[sbrannen]

Hi @cpovirk,

Thanks for the clarification!

[sbrannen]

@cpovirk, please note that ThrowingRunnable is from JUnit 4; whereas, JUnit Jupiter (i.e., the programming model for JUnit 5) has an Executable functional interface which serves the same purpose (just with a more generic name). So if you're explicitly supporting ThrowingRunnable, you'll want to add explicit support for Executable as well. 😉

[cpovirk]

@sbrannen Thanks, I didn't realize that. It looks like the person who made the fix did, and he's keying off the method: org.junit.Assert/org.junit.jupiter.api.Assertions assertThrows/expectThrows. So I think we're in good shape.

https://github.com/google/error-prone/commit/f9ea34b52ace79c5f3e1d39fd0f1ade2eba9b06a#diff-cf2cabfe009902419c7dde78f91abe3aR243

[mmerdes]

@sbrannen
yes, sounds reasonable to me

[sbrannen]

FYI: expectThrows is now deprecated and will be removed in 5.0 M4.

[kcooney]

@sbrannen I just found this thread. Shouldn't we make the same change in JUnit 4.13? We haven't started the release process for 4.13.

Did we decide that IntelliJ users should simply disable the warning, or did the warning only happen if the method was annotated with @CheckReturnValue?

[sbrannen]

Shouldn't we make the same change in JUnit 4.13? We haven't started the release process for 4.13.

Yes, I would recommend making the same change in JUnit 4.13.

Did we decide that IntelliJ users should simply disable the warning, or did the warning only happen if the method was annotated with @CheckReturnValue?

Google error-prone already has it covered (see link by @cpovirk), and IntelliJ users can simply disable that warning if it bothers them (see attached screenshot above in this thread).

[sormuras]

The screenshot is here:

[kcooney]

FYI: there is some discussion at https://github.com/junit-team/junit4/pull/1396 about whether assertThrows() is a good name considering the method returns something. Having the name be different than the other methods might reduce confusion (Edit: i.e. having the name not start with "assert" could give a hint that it's a different kind of method)

[sbrannen]

@kcooney, we have already hashed this out and implemented what we decided on.

Basically, this is one of those areas of API design for which you'll only please half of the audience. Some people love that there is only a single assertion method for exceptions. Others (as in the JUnit 4 thread) dislike it severely.

As for the name, "expect" is definitively no better than "assert" or "verify" or "check". They all mean the same thing "assert an expectation"; none of them implies that a value is returned. The only way to convey without a doubt that an assertion method returns something is to state that explicitly in the method name -- for example, assertThatCodeBlockThrowsExceptionAndReturnExceptionThrown(...). Even if we shorten that to something like assertExceptionThrownAndReturnIt(...), that is still exceedingly verbose in contrast to the rest of the assertion methods.

That's why we opted for assertThrows(...). It's clear and concise. The fact that it breaks from JUnit's tradition by returning a value is well documented, and people will catch on immediately, especially since every example will show it in use.

And... as always -- and as explicitly stated in the JUnit 5 User Guide -- nobody is forced to use JUnit's assertions: people are free to use AssertJ, Hamcrest, Truth, etc.

[mlevison]

@sbrannen - there's truth? cool why didn't someone tell me. Are beauty,
strange and charmed all part of the same package? If so I will forever use
this library.

Sorry for late night humour
Mark

On Tue, Dec 13, 2016 at 9:45 PM, Sam Brannen [e-mail]
wrote:

@kcooney https://github.com/kcooney, we have already hashed this out
and implemented what we decided on.

Basically, this is one of those areas of API design for which you'll only
please half of the audience. Some people love that there is only a single
assertion method for exceptions. Others (as in the JUnit 4 thread) dislike
it severely.

As for the name, "expect" is definitively no better than "assert" or
"verify" or "check". They all mean the same thing "assert an expectation";
none of them implies that a value is returned. The only way to convey
without a doubt that an assertion method returns something is to state that
explicitly in the method name -- for example,
assertThatCodeBlockThrowsExceptionAndReturnExceptionThrown(...). Even if
we shorten that to something like assertExceptionThrownAndReturnIt(...),
that is still exceedingly verbose in contrast to the rest of the assertion
methods.

That's why we opted for assertThrows(...). It's clear and concise. The
fact that it breaks from JUnit's tradition by returning a value is well
documented, and people will catch on immediately, especially since every
example will show it in use.

And... as always -- and as explicitly stated in the JUnit 5 User Guide --
nobody is forced to use JUnit's assertions: people are free to use AssertJ,
Hamcrest, Truth, etc.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/junit-team/junit5/issues/531#issuecomment-266927302,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAImZpSuwmX_Kg6Nctkom74_CNxk451_ks5rH1gvgaJpZM4KLiHM
.

--

[image: headshot-square-300x300]
http://www.flickr.com/photos/36331075@N00/9674877046/ Mark Levison | 1
[phone] | Twitter https://twitter.com/mlevison | LinkedIn
http://ca.linkedin.com/in/marklevison | Facebook
https://www.facebook.com/agilepainrelief
Certified ScrumMaster Training: Vancouver
http://agilepainrelief.com/courses/vancouver | Edmonton
http://agilepainrelief.com/courses/edmonton | Ottawa
http://agilepainrelief.com/courses/ottawa | Montreal
http://agilepainrelief.com/courses/montreal | Toronto
http://agilepainrelief.com/courses/toronto
Certified Product Owner & Private Training also available ~ Our Training
Schedule http://agilepainrelief.com/courses/certified-scrum-agile-training
Agile Pain Relief Consulting http://agilepainrelief.com/ | Notes from a
Tool User http://agilepainrelief.com/notesfromatooluser
Proud Sponsor of Agile Tour Gatineau Ottawa http://goagiletour.ca/ and Agile
Coach Camp Canada http://agilecoachcampcanada.wordpress.com/

[marcphilipp]

That's why we opted for assertThrows(...). It's clear and concise. The fact that it breaks from JUnit's tradition by returning a value is well documented, and people will catch on immediately, especially since every example will show it in use.

I totally agree with @sbrannen on this one.