Add matchers for testing exception properties#2904
Add matchers for testing exception properties#2904vslashg merged 10 commits intogoogle:masterfrom AmatanHead:throw-matchers
Conversation
|
Rather than |
|
@vlovich this is exactly what's happening: |
vlovich
left a comment
There was a problem hiding this comment.
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?
Only if you have
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. |
|
@CJ-Johnson can we please have someone from the project team to review this PR? |
|
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. |
|
Are you able to re-run the CI? |
|
@CJ-Johnson I'm not sure if there is a special command to restart CI so I've rebased the branch. |
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
|
Rebased onto the current master. |
|
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) |
|
@CJ-Johnson, how's internal review going for this PR? |
|
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. |
|
The change has been submitted internally. It will become available the next time we do an upstream push! |
|
Dear Googlers and @AmatanHead, do you think it's a good design to force users to bind function to its arguments? Because internally // 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(foo(1, 2, 3), std::runtime_error); |
|
@kuzkry As for my reasoning for this implementation, it was the following:
I'm not saying it is an ideal solution, but it is the best one I could come up with given the requirements. |
|
Firstly, it seems the new 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: From other results, it seems to me the lambda in the I tried to call the lambda within the |
|
@adambadura, did you include the gMock header ( |
|
@doak, after more than half of a year I cannot be sure now. However, normally I do include |
|
Btw, it also works to include just |
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