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

Fluent expectations for ExpectedException #1245

Merged
merged 1 commit into from
Mar 13, 2016

Conversation

pablisco
Copy link
Contributor

This small PR will allow for the ExpectedException rule to be used in a more fluent manner.

thrown.expect(IllegalArgumentException.class).expectMessage("Not a valid argument");

May need updating on https://github.com/pablisco/hamcrest-junit too

@kcooney
Copy link
Member

kcooney commented Feb 19, 2016

I think we decided to not make any more changes to ExpectedException and instead encourage people to use assertThrows and expectThrows.

See #1199

@pablisco
Copy link
Contributor Author

Few things. Firstly, using assertThrows is not much different from just adding expected on the @Test annotation. Plus for people that can't use Java 8 (cough Android cough) it's not convenient to have to use the lambda.
Secondly, the methods mentioned doesn't allow to check for annotation message or cause, which can be helpful to test different exceptions that are only differentiated by their message or the cause of the exception.

@kcooney
Copy link
Member

kcooney commented Feb 19, 2016

assertThrows is very different than adding expected: you can specify exactly what code you expect to throw.

And if you want to check the cause, you can use expectThrows which returns the exception.

In contrast, ExpectedException is very easy to use incorrectly, which is why we are slowly discouraging it's use.

I conceed your point about Android (I have been raising the same concerns about JUnit Lambda's use of Java 8 APIs).

I'll leave it up to @marcphilipp to decide

@pablisco
Copy link
Contributor Author

@kcooney I'm interested in knowing, what's the incorrect usage? It seems quite straight forward.

An interesting alternative for lambda can be found in AssertJ:
http://joel-costigliola.github.io/assertj/assertj-core-features-highlight.html#exception-assertion

Also, more powerful in terms on what to test on the exception.

@kcooney
Copy link
Member

kcooney commented Feb 19, 2016

@pablisco The ExpectedException API has too much "magic". I have seen many people use ExpectedEcception and write code after the method call that is expected to throw. It also can't be used in a helper method (because the helper method will never return).

@kcooney
Copy link
Member

kcooney commented Feb 19, 2016

We considered an approch similar to AssertJ, but 1) creating a good DSL is challenging, 2) not everyone likes DSLs and 3) people that do like DSLs most likely already use an assertion DSL (like Fest or Truth).

@marcphilipp
Copy link
Member

Even though the use of ExpectedException is discouraged, I'm inclined to merge this pull request for two reasons:

  1. it is a nice little enhancement
  2. it does not break backwards compatibility

marcphilipp added a commit that referenced this pull request Mar 13, 2016
Fluent expectations for ExpectedException
@marcphilipp marcphilipp merged commit 06274d1 into junit-team:master Mar 13, 2016
@PhilippWendler
Copy link

Is maintaining bytecode compatibility a goal for JUnit? This will break test classes compiled against ExpectedException from 4.12 to work with 4.13 or newer (source).

@kcooney
Copy link
Member

kcooney commented Aug 17, 2016

@PhilippWendler I don't think we have given that much thought. We do try to maintain source code compatibility, but we have unintentionally broken source code compatibility in some of our prior releases.

Binary compatibility is usually only an issue with third-party libraries. I don't know how likely it is for third-party libraries to use this class.

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.

None yet

4 participants