Delete assertThrows()

[kcooney]

Delete expectThrows(). Change assertThrows() to return the caught exception.

JUnit 5 decided to make the same change in the Assertions class, so this
change makes the JUnit4 API consistent.

[stefanbirkner]

The Javadoc of ThrowingRunnable has a reference to expectThrows. Could you change that, please.

[kcooney]

Fixed comment that referenced expectThrows, rebased and squashed commits

[kcooney]

Thanks, Marc. I am going to wait for one or two more approvals.

[kcooney]

I also requested feedback on the original pull request.

[rschmitt]

Is there a single precedent anywhere in JUnit for an assert function that does not return void? Was a motivation for this change even spelled out somewhere? In terms of naming conventions, this strikes me as the equivalent of creating a function called toString that writes something to standard out and returns void instead of String.

[kcooney]

@rschmitt see junit-team/junit5#531 for the discussion about the associated JUnit 5 APIs.

[kcooney]

@rschmitt did you have a chance to read that thread?

Personally I don't have a strong opinion about how these methods should be named (the name expectThrows() never bothered me). I just want the APIs for making assertions about exceptions to be consistent between JUnit 4 and JUnit 5.

[rschmitt]

The only new point I encountered in the other thread is that "expect" terminology has different connotations in some other frameworks. The only specific example of that was Google Test, which is actually a C++ testing framework. Since we're drawing analogies to non-Java languages, I'll point out the expect method from Rust's Option type, which corresponds to expectThrows semantics perfectly (return the expected thing if it's there, panic if it's not):

https://doc.rust-lang.org/std/option/enum.Option.html#method.expect

There is another argument I saw which I would like to directly refute, this one from @sbrannen:

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

To this I respond: so what? The key point here is that expectThrows is the only method in Assertions that returns anything, and yet nobody objects to that. (I don't recall any proposals to remove expectThrows on the grounds of uselessness.) Renaming expectThrows to assertThrows despite this essential difference is fake simplicity.

[kcooney]

Regardless of the name in JUnit5, I'm coming around to the decision that expectThrows is a better name for JUnit4 (see the discussion on the JUnit5 thread)

[marcphilipp]

Regardless of the name in JUnit5, I'm coming around to the decision that expectThrows is a better name for JUnit4 (see the discussion on the JUnit5 thread)

I strongly disagree. I have done quite a few talks in the last couple of months and often got the feedback that expectThrows should be called assertThrows.

In terms of naming conventions, this strikes me as the equivalent of creating a function called toString that writes something to standard out and returns void instead of String.

I see that differently. The name assertThrows is perfectly fine because it does assert that an exception is thrown. Therefore, it does match the other methods in Assert, it just returns the actual exception as additional info.

[kcooney]

The method executes a lambda, catches the thrown exception, asserts that the exception's type matches the passes-in class, then returns the caught exception; the assertion is only one small part So it is quite different than all the other methods.

[panchenko]

There is no harm in having both like in TestNG, for ~1.5 years already.

[kcooney]

@panchenko what specifically in TestNG are you referring to?

[rschmitt]

https://github.com/cbeust/testng/blob/af791db28ae6b7add6b1a03da5cac690a4d20752/src/main/java/org/testng/Assert.java#L943-L998

…

On Wed, Dec 14, 2016 at 12:32 PM, Kevin Cooney ***@***.***> wrote: @panchenko <https://github.com/panchenko> what specifically in TestNG are you referring to? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1396 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADjW-a6vokX8FNcMovuWzYrTB0C9WMwIks5rIFJMgaJpZM4LAdYF> .

[kcooney]

So @panchenko you are proposing keeping both expectThrows() and assertThrows() as-is?

I wasn't aware that the same methods were added to TestNG. Perhaps we should reach out to the TestNG team to see if the methods they had led to any confusion, and if not, keep both methods (at least in org.junit.Assert; JUnit 5 may do something different).

[panchenko]

@kcooney exactly, I would suggest keeping both.

scalatest also has 2 methods - assertThrows and intercept.

[jbduncan]

I disagree that expectThrows should be included.

Looking at TestNG's source code (and remembering the source code from older versions of JUnit 5), it looks to me that expectThrows is merely a wrapper around assertThrows that returns void instead of T extends Throwable, so it makes no sense to me to have expectThrows from both a performance and readability standpoint, since the return result of assertThrows can always be ignored.

[kcooney]

@jbduncan actually assertThrows is a wrapper around expectThrows.

I don't follow the performance concerns. These methods are passed lamdas which throw exceptions. Exception handling is much more expensive than a function call.

JUnit4 doesn't have any assert methods than return something that should be used. I am not sure we should start now, so I think we should have both, just like TestNG.

[jbduncan]

@kcooney Oh yes, silly me! Got them a bit mixed in my mind.