-
Notifications
You must be signed in to change notification settings - Fork 12.9k
[ci] Use assertj/fluent exceptions for cleaner unit testing #977
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
Conversation
3c8ee06
to
f5043e1
Compare
Not ready yet! Need to restore jdk6 support on test side and figure out two classes to switch over. |
Dropped fluent exception and instead using catch exception. Catch exception doesn't handle constructor exceptions so just using try/catch for those. Further refinement with getting rid of junit rule entirely. |
Use earlier assertj for java 6 support. |
Added some close of resources that I saw as well while addressing this. |
</dependency> | ||
<dependency> | ||
<groupId>eu.codearte.catch-exception</groupId> | ||
<artifactId>catch-exception</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hazendaz Is need catch-exception
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the junit rules needed switched to something. So this was required in a number of cases. In fact, I ended up just clearing all junit rules entirely out. One major benefit to using this method is that catch exception proxies the exception so code coverage on unit test looks better. That library isn't necessary once on java 8 though. For now just needed something to fill the gap. Although direct usage of try/catch with reading the exceptions would have also worked.
try { | ||
new ParameterExpression("id:"); | ||
} catch (BuilderException e) { | ||
e.getMessage().contains("Parsing error in {id:} in position 3"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hazendaz ,
Thanks for the work!
I think there needs to be a call to fail()
in the try clause, otherwise the test passes even when there is no exception thrown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! I'll go back through and add those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, and it also needs assertTrue
or something to verify the result of contains()
method XD
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it! I'll try to take care of those tomorrow night.
[ci] Use assertj/fluent exceptions for cleaner unit testing
Let's face it, hamcrest is all but EOL. With junit 5, we will be using assertj so lets use it now. And for more of a temporary thing lets use fluent exception until we are on java 8.