-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Deprecate expectThrows in favor of assertThrows returning the exception #531
Comments
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. |
TBH, I've personally never been a fan of the Aside from the two ProposalIf it were up to me, I would simply delete the existing Rationale:
@junit-team/junit-lambda, what do the rest of you think about my proposal? |
Sam - Not a team member, but as a user and someone who educates others this Cheers On Oct 1, 2016 9:28 AM, "Sam Brannen" notifications@github.com wrote:
|
I have also been stumbled upon those methods being confused. Totally agree. 👍 |
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 |
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? |
@sbrannen, I know that IntelliJ and Google's error-prone compiler emit warnings if the return values of methods annotated with Might one of both of these be the offender(s) that prompted the existence of |
IntelliJ reports the following warning: Result of 'expectThrows()' not thrown -- I just remove the check mark here. |
@jbduncan, it looks like Goggle's error-prone will address that here: google/error-prone#452 |
Yes. See second shot linked above. |
For anyone interested, the source of |
Google's Error Prone cares only about methods with
That bug report was to address a slightly different problem:
The idea is that a method like Error Prone already doesn't mind if you ignore the return value of
|
Hi @cpovirk, Thanks for the clarification! |
@cpovirk, please note that |
@sbrannen Thanks, I didn't realize that. It looks like the person who made the fix did, and he's keying off the method: google/error-prone@f9ea34b#diff-cf2cabfe009902419c7dde78f91abe3aR243 |
@sbrannen |
FYI: |
@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 |
Yes, I would recommend making the same change in JUnit 4.13.
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). |
The screenshot is here: |
FYI: there is some discussion at junit-team/junit4#1396 about whether |
@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, That's why we opted for 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. |
@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 ***@***.***> 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
<#531 (comment)>,
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
(877) 248-8277 | 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/>
|
I totally agree with @sbrannen on this one. |
More information: junit-team/junit5#531 PiperOrigin-RevId: 186900384
More information: junit-team/junit5#531 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=187034408
More information: junit-team/junit5#531 PiperOrigin-RevId: 186900384
Is it possible to rename
expectThrows
?As we introduce people to
expectThrows
, we keep hearing: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...
...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.
The text was updated successfully, but these errors were encountered: