Prevent usage of @Test(expected = ...) and change existing tests

[nastra]

We should avoid usage of @Test(expected = ...) because it is not
always clear from where exactly an exception is thrown. We should rather
promote using Assertions.assertThatThrownBy(...).isInstanceOf(...).hasMessage(...)
as that makes sure that we are in fact getting the right exception with
the appropriate error message.

[RussellSpitzer]

We actually have a helper for this in AssertHelpers

/**
   * A convenience method to avoid a large number of @Test(expected=...) tests
   * @param message A String message to describe this assertion
   * @param expected An Exception class that the Runnable should throw
   * @param containedInMessage A String that should be contained by the thrown
   *                           exception's message
   * @param runnable A Runnable that is expected to throw the runtime exception
   */
  public static void assertThrows(String message,
                                  Class<? extends Exception> expected,
                                  String containedInMessage,
                                  Runnable runnable) {
                                  ```

[nastra]

I know but direct Assertions usage gives you much more flexibility around exception verification

[RussellSpitzer]

I think we would probably want to change all the instances in the code base to that then, rather than have two ways to do it.

[nastra]

looks like there are about 800 usages of different variations of assertThrows in the codebase, so not sure we'd want to update all of those. Maybe we'll just mention in the checkstyle message Prefer using AssertHelper.assertThrows(...) or Assertions.assertThatThrownBy(...).isInstanceOf(...) instead. I believe both cases should be valid, where Assertions.assertThatThrownBy(...) just gives you more flexibility over things

[rdblue]

assertThrows was used before we were using Assertions. It's pretty old. I think it's fine to go either way, with a slight preference for using Assertions since it is easier to read. The main thing to watch out for is that the exception message is validated as well as the type.

[nastra]

I'm in favor of steering people towards Assertions usage due to greater flexibility and more fluent assertion checks. In terms of checking the exception message, I think this is a good practice in general, since you really want to make sure you're getting the correct exception with the message you're expecting

[dimas-b]

From my POV, Assertions statements are pretty intuitive to read and interpret and in most cases result in very few lines of code. +1 to promoting them.

[danielcweeks]

Seems like we have general consensus that this is ok to proceed with. Thanks @nastra!