Add matchers for testing exception properties

[AmatanHead]

This PR adds matchers that accept a callable and verify that when invoked, it throws an exception with the given type and properties.

Fixes #952

[vlovich]

Rather than ThrowsMessage/ThrowsMessageHasSubstr, does it makes sense to have Throws take a generic matcher to evaluate the thrown exception? There could be a convenience matcher to retrieve .what() from an exception & then the composition facilities of gmock could be leveraged more powerfully (e.g. to introspect user-specific exceptions).

[AmatanHead]

@vlovich this is exactly what's happening: ThrowsMessage<E>(M) is just a shortcut for Throws<E>(Property("what", &std::exception::what, M). I've introduced these shortcuts because checking contents of the message seems to be the most frequent scenario. However, you're free to use Throws with an arbitrary matcher. See the last example in this snippet.

[vlovich]

Yeah, I did notice that. It's a really well written piece of code & I hope you get approval to land it. I do think that ThrowsMessageHasSubstr is a bit superfluous as the syntactic sugar benefit isn't that significant vs putting HasSubstr() around the argument. I'm not a maintainer on googletest though so that's just my 2c for whatever that's worth.

On the other hand, for some reason I have approval permission? That doesn't seem right. Is googletest just open for any arbitrary other party to approve a diff? Or did I somehow get ninja permissions here because I at one point worked at Google?

[AmatanHead]

I do think that ThrowsMessageHasSubstr is a bit superfluous as the syntactic sugar benefit isn't that significant

Only if you have using namespace testing in your code =) Also, ThrowsMessage("message") will try and match message for equality, which is something you might not expect. Presence of ThrowsMessageHasSubstr makes this moment a bit clearer.

On the other hand, for some reason I have approval permission?

I think GitHub allows anyone to leave a review, no matter if it's approval, need work or neutral. It doesn't allow merging PR though.

[AmatanHead]

@CJ-Johnson can we please have someone from the project team to review this PR?

[AmatanHead]

The build failed while trying to connect to a local Bazel server. I suppose this is a transient error that has nothing to do with the changes in this PR.

[CJ-Johnson]

Are you able to re-run the CI?

[AmatanHead]

@CJ-Johnson I'm not sure if there is a special command to restart CI so I've rebased the branch.

[AmatanHead]

Rebased onto the current master.

[CJ-Johnson]

Thank you for the PR, we have started internal review. Please don’t push any more changes into this PR as they might be overwritten. (325012153)

[AmatanHead]

@CJ-Johnson, how's internal review going for this PR?

[CJ-Johnson]

Ah yikes, 12 days already! I'm so sorry for the delay on that. It ended up not being "ready" yet so we've had to work on it a bit further (same API, modified implementation). I apologize for the delay since this came up in the middle of several ongoing tasks. I will do my best to get this done by the end of the week! Thanks for the reminder.

[CJ-Johnson]

The change has been submitted internally. It will become available the next time we do an upstream push!

[kuzkry]

Dear Googlers and @AmatanHead, do you think it's a good design to force users to bind function to its arguments? Because internally ExceptionMatcherImpl calls (void)(std::forward<T>(x)());, so x is expected to be callable with 0 arguments, therefore:

//  I can't do this (doesn't compile)
EXPECT_THAT(foo(1, 2, 3), ::testing::ThrowsMessage<std::runtime_error>("thrown from foo"));

// Instead I've gotta do that
EXPECT_THAT([]{ foo(1, 2, 3); }, ::testing::ThrowsMessage<std::runtime_error>("thrown from foo"));

What I mean here is that IMHO it doesn't compose nicely with EXPECT_THROW interface which doesn't require argument binding tricks and works like that:

EXPECT_THROW(foo(1, 2, 3), std::runtime_error);

[AmatanHead]

@kuzkry EXPECT_THAT is not related to EXPECT_THROW in any way. In fact, EXPECT_THAT was originally a part of gmock, while EXPECT_THROW is native to gtest. It helps to think of it this way. EXPECT_THAT(a, b) means that we expect the value of a to satisfy condition b. Therefore EXPECT_THAT(a, Throws<Error>()) means that the value of a throws an error when called. Then EXPECT_THAT(foo(1, 2, 3), Throws<Error>()) would then mean that the value of foo(1, 2, 3) throws an error when called.

As for my reasoning for this implementation, it was the following:

• adding a new macro was discussed against in the linked issue;
• EXPECT_THAT is the only mechanism that'll allow us to implement this feature;
• we can't change the semantics of EXPECT_THAT;
• thus this is the only way we can implement this.

I'm not saying it is an ideal solution, but it is the best one I could come up with given the requirements.

[adambadura]

Firstly, it seems the new Throws* matchers are not documented.

Secondly, do they work? I'm not able to make them work... I have the following code:

EXPECT_THAT(
    [&](){
        foo();
    },
    ::testing::ThrowsMessage<MyException>(
        ::testing::HasSubstr("message")
    )
);

however, while executing the test it fails:

Value of: [&](){ foo(); }
Expected: throws an exception which is a MyException which contains .what() that has substring "message"
  Actual: 8-byte object <F0-3C 27-01 00-00 00-00>, does not throw any exception

From other results, it seems to me the lambda in the EXPECT_THAT was not invoked. Instead, it was considered a value to be matched that didn't match.

I tried to call the lambda within the EXPECT_THAT but this resulted in a compilation error since the result of the lambda is void while EXPECT_THAT (unsurprisingly) expects some value.

[doak]

@adambadura, did you include the gMock header (gmock/gmock.h)? I stumbled over the same, it was not obvious to me.

[adambadura]

@doak, after more than half of a year I cannot be sure now. However, normally I do include <gmock/gmock.h> when using EXPECT_THAT or any matchers. So, I don't expect a missing include was the reason here.

[stonigrl]

.

[doak]

Btw, it also works to include just gmock/gmock-matchers.h:

#include <gtest/gtest.h>
#include <gmock/gmock-matchers.h>

TEST(assertOnExpectedExceptionMessage, throwExceptionWithMessage_catchesExceptionWithCorrectMessage) {
    ASSERT_THAT(
            [&](){ throw std::runtime_error("bla"); },
            testing::ThrowsMessage<std::runtime_error>("bla"));
}