Implement matchers for thrown exceptions in `Runnable`

[ggalmazor]

Fixes #329

This PR is inspired by @peterdemaeyer's #330, which fell through the cracks two years ago. I want to rekindle this effort to have matchers for exceptions thrown in Runnable.

The goal of this matcher is to provide an easy way to verify that some code will throw some expected exception by wrapping it inside a Runnable that's executed safely by the matcher and tests the thrown exception with the provided matchers, as in:

assertThat(() -> SomeCode.thatThrows(), throwsException(withMessage("Boom!")));

This is my first contribution to this repo, so I expect my changes won't align with current conventions. I've tried to imitate patterns I've seen in other TypeSafeMatcher subclasses, but I can't be sure if the result is 100% acceptable. Please point me in the right direction, and I'll apply any changes needed to merge this PR.

In this PR:

• New throwsException type-safe matcher for Runnable
• New throwsExceptionEqualTo type-safe matcher for Throwable
• New throwsExceptionWithMessage type-safe matcher for Throwable

[tumbarumba]

Thanks for picking this up @ggalmazor, I really appreciate you helping out like this (and also to you @peterdemaeyer, apologies for not looking at your previous PR earlier).

At a high level, I really like what this code is doing. However, I think we could make some usabilty improvements. My biggest gripe is that the code as stands requires an actual exception to match against. I prefer the JUnit approach of matching against the exception types (see https://junit.org/junit5/docs/current/user-guide/#writing-tests-exceptions-expected)

Some example code that I think would work well in the context:

import org.junit.Test;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.exception.ThrowsException.throwsException;

public class ThrowsExceptionMatcherTest {
    @Test
    public void testThrows() {
        assertThat(this::codeThatThrows, throwsException());
        assertThat(this::codeThatThrows, throwsException(RuntimeException.class));
        assertThat(this::codeThatThrows, throwsExceptionWithMessage("boom"));
        assertThat(this::codeThatThrows, throwsExceptionWithMessage(containsString("oom")));
        assertThat(this::codeThatThrows, throwsExceptionWithMessage(RuntimeException.class, "boom"));
        assertThat(this::codeThatThrows, throwsExceptionWithMessage(RuntimeException.class, containsString("oom")));
    }

    public void codeThatThrows() {
        throw new RuntimeException("boom");
    }
}

Some other odds and ends to think about:

• Add an alias to the static builder in org.hamcrest.Matchers
• Update changes
• Change the tests to use JUnit Jupiter
  • See #424
  • this is a change recently added - just need to merge in

[ggalmazor]

Ah, thanks so much for your guidance @tumbarumba. I'll work on your suggestions asap 👍

[ggalmazor]

@tumbarumba I finally got around to adding your suggestions to the PR.

I've added QoL matcher factories to support different variations, providing types instead of instances, as you suggested. I agree with you that this was missing, and it adds much-needed versatility 👍

However, I have kept the matchers that target actual exceptions because I do think they bring value to enable Hamcrest users to build matchers that target exception internal state beyond their message, e.g., checking secondary causes or matching some public property. In my experience, this flexibility can be pivotal when dealing with third-party libraries that "get in your way" regarding error management.

This is an "extended" version of the examples you used to describe how you'd like to see the matcher work (I hope you like it 😄):

@Test
public void examples() {
  RuntimeException boom = new RuntimeException("boom");
  Runnable codeThatThrows = () -> {
    throw boom;
  };

  assertThat(codeThatThrows, throwsException());
  assertThat(codeThatThrows, throwsException(boom));
  assertThat(codeThatThrows, throwsException(RuntimeException.class));
  assertThat(codeThatThrows, throwsException("boom"));
  assertThat(codeThatThrows, throwsException(withMessage("boom")));
  assertThat(codeThatThrows, throwsException(withMessage(containsString("oom"))));
  assertThat(codeThatThrows, throwsException(RuntimeException.class, "boom"));
  assertThat(codeThatThrows, throwsException(RuntimeException.class, withMessage("boom")));
  assertThat(codeThatThrows, throwsException(RuntimeException.class, withMessage(containsString("oom"))));
}

I kept the throwsException as the main entry point to match against Runnables. It has overrides to support all the variations of exception, exception class, message, and exception matcher I could think of.

I couldn't add variations for the message matcher due to type erasure conflicts with the exception matcher variants. The exception matcher would be more versatile between the two, which was another argument for me to keep it around.

Besides that, I've also upgraded the tests to junit5 and added the aliases you requested.

[tumbarumba]

Thanks @ggalmazor. I should get time to have a proper look this weekend.

[tumbarumba]

Hi @ggalmazor, I spent some time last week reviewing your code, and testing it out in various scenarios. Upon reflection, I think there are a number of changes I'd still like to see.

Here are some of the things I was thinking about...

The current implementation is spread across 3 classes (ThrowsException, ThrowsExceptionEqualTo, and ThrowsExceptionWithMessage). This struck me as slightly more complicated than needed. I looked into how you referenced these classes in org.hamcrest.Matchers, and noticed that only the static functions from ThrowsException were referenced. This got me wondering if we could simplify the structure.

ThrowsExceptionWithMessage is a nice wrapper around a string matcher used in the context of a Throwable. I think the static builder function withMessage is a little bit problematic, in that it only makes sense in the context of that class. I couldn't do a static import of the method name, and be able to infer it's purpose. I can see how you're referencing the function within ThrowsException, but that would be harder for users to reference directly outside of that context.

ThrowsExceptionEqualTo looks like it could be simplified with a combination of IsInstanceOf and IsEqual matchers. That is sort of what you're doing in the ThrowsException class, but in this instance there was a different implementation.

In the end, I tried to see if I could merge the implementations. Starting with your code, I did some refactoring and ended up with this merged ThrowsException. You can find this on the tumbarumba/throws-exception branch. Are you able to get your PR to something like that?

[ggalmazor]

Thanks for taking the time to look into this, @tumbarumba. Your branch made it super easy for me to introduce the changes. I just had to tweak a few things to adapt the tests 👍

I also like that you migrated the class to extend from the self-diagnosing matcher superclass, which feels more robust and modular.

A few notes:

• I still think that matching exception instances would be a valuable addition. However, I think we have lost that option by changing the constructor in ThrowsException to accept a class and a string matcher.

  I want to pursue this idea further, and I'll open another PR if I can produce something I feel good about. I would rather avoid blocking this PR just because of that.

• I've removed the factory method that allows providing an instance to create the matcher since the value is low compared to using the other factories and could misdirect users into thinking the matcher will match instances instead of exception classes.

  For exception instances, I would rather have a separate set of matchers that are aligned with my previous point above and provide comprehensive instance matching to cover some of the examples I provided in my previous comment on this PR.

Let me know what you think :)

[tumbarumba]

Looks good, @ggalmazor. Nice work!

Can you please make one more minor change: you've reformatted all the javadoc in org.hamcrest.Matchers. I agree that the changes are an improvement, but it's unrelated to the addition of a new matcher. Can you revert the reformatting, and just go with the additional methods?

[ggalmazor]

Can you please make one more minor change: you've reformatted all the javadoc in org.hamcrest.Matchers. I agree that the changes are an improvement, but it's unrelated to the addition of a new matcher. Can you revert the reformatting, and just go with the additional methods?

Yes, apologies! That was an unintended change