API: Deprecate AssertHelpers#6977
Conversation
Fokko
left a comment
Fokko
left a comment
There was a problem hiding this comment.
I like it. Should we also remove the usage of AssertHelpers? Could be a separate PR.
That would be ideal in the long run, but the diff will be quite big, because it's being used at lots of places. At the very least I think we can start and not use it in new tests. |
jackye1995
left a comment
jackye1995
left a comment
There was a problem hiding this comment.
We can create an issue to remove the usages of it, it would be a good beginner task.
| * </pre> | ||
| * | ||
| * @deprecated Use {@link Assertions#assertThatThrownBy(ThrowableAssert.ThrowingCallable)} directly | ||
| * as it provides a more fluent way of asserting on exceptions. |
There was a problem hiding this comment.
My only issue with this is making sure that hasMessage is called. How do we enforce that?
There was a problem hiding this comment.
that's a valid point. I was thinking maybe we could introduce a checkstyle rule (a multiline regex) that would make sure checks against the error message are done. I can open a separate PR for that.
There was a problem hiding this comment.
Do we need deprecations on the methods as well?
There was a problem hiding this comment.
all calling sites that use any method from AssertHelpers will see that the class itself is deprecated, so I don't think we need to deprecate each method individually
There was a problem hiding this comment.
I did some small tests, there is a small difference in IDE: if only the static method is imported the user won't see deprecation warning in code unless he/she looks at the import block, so it might still be beneficial to mark methods as deprecated as well.
There was a problem hiding this comment.
good point @jackye1995. I've added deprecations to all methods inside that class
I believe we should point people for newly written code to use `Assertions#assertThatThrownBy(..)` as it provides a more fluent way of asserting exceptions
|
@nastra, @jackye1995 I merged this. It will be a good move, but we have to make sure we are catching the message validation in reviews! |
Point people to use `Assertions#assertThatThrownBy(..)` as it provides a more fluent way of asserting exceptions
|
follow-up to complete removal of AssertHelpers -- #10500 |
5 participants
I believe we should point people for newly written code to use
Assertions#assertThatThrownBy(..)as it provides a more fluent way of asserting exceptions