Fix MOCK_METHOD to handle noexcept correctly #2498
Conversation
There was a problem hiding this comment.
The fix seems OK, and certainly improves behavior.
I suggested some improvements to tests.
Another small issue is behavior in case noexcept is used multiple times (which is invalid usage). I think it will lead to cryptic error messages, and better validation would be nice. But it isn't critical.
| }; | ||
|
|
||
| TEST(MockMethodMockFunctionTest, NoexceptSpecifierPreserved) { | ||
| EXPECT_TRUE(noexcept(std::declval<MockMethodNoexceptSpecifier>().func1())); |
There was a problem hiding this comment.
Is there a reason to use declval here? I'd simply define a MockMethodNoexceptSpecifier object.
In case the compiler warns that it's unused, it can be suppressed by cast to void.
There was a problem hiding this comment.
I had that previously, but the compiler would always say the result of the expression is noexcept(false).
https://travis-ci.org/google/googletest/jobs/594800623#L2313
My guess would be it has to do with the constructor not being marked noexcept.
But declval really is the "correct" solution as we just want to test the noexcept specifier for the function (and not the default constructor as well).
Another solution would be to use std::is_nothrow_invocable, but I don't think there's a technical reason to prefer one over another.
There was a problem hiding this comment.
My intention was to define one MockMethodNoexceptSpecifier object in the test, and use it in all expectations. Then it wouldn't matter if the constructor is noexcept.
There was a problem hiding this comment.
That's definitely another solution. Personally, I wouldn't like having to declare an instance of an object (which is a runtime construct) just so syntactically (since noexcept expressions are evaluated at compile-time) I can reason about the methods.
But I also can't argue it would be less code.
If this comes up in the "internal review" I'd be alright changing it.
There was a problem hiding this comment.
Instead of EXPECT_TRUE/FALSE, these could be tested at compile time with static_assert.
There was a problem hiding this comment.
Yeah, it's all about tradeoffs if you want to use static_asserts in your test code to test some logic.
If you move it into a compile-time check, you know it'll always be true. The tests wouldn't compile otherwise. On the other hand, normally one test failing doesn't bar other tests from running and reporting their successes/failures. Since the noexcept spec wouldn't be absolutely fatal if it was wrong, I opted to implement them as runtime checks so they can report successes/failures just like other tests.
| EXPECT_EQ(noexcept(std::declval<MockMethodNoexceptSpecifier>().func5()), noexcept(1+1)); | ||
| EXPECT_EQ(noexcept(std::declval<MockMethodNoexceptSpecifier>().func6()), noexcept(1+1)); | ||
| EXPECT_EQ(noexcept(std::declval<MockMethodNoexceptSpecifier>().func7()), noexcept(hasTwoParams(1,2))); | ||
| } |
There was a problem hiding this comment.
I see no point in comparing with noexcept(1+1) - it simply evaluates to true. I suggest:
void Throws();
void DoesntThrow() noexcept;
struct MockMethodNoexceptSpecifier {
MOCK_METHOD(void, func4, (), (noexcept(Throws())));
MOCK_METHOD(void, func5, (), (noexcept(DoesntThrow())));
};
TEST(...) {
EXPECT_FALSE(noexcept(mock.func4()));
EXPECT_TRUE(noexcept(mock.func5()));
}
Seemingly it currently (before my change) leads to no error messages: https://godbolt.org/z/0TtYoG |
|
@thejcannon I am going to import this into internal system so we can have an internal review. |
PiperOrigin-RevId: 274097989
|
@thejcannon Looks like this broke Windows builds. |
|
@gennadiycivil I'll take a look at it when I get to work today. Might be related to #2490 |
PiperOrigin-RevId: 274155281
|
In the meantime we reverted, ba513d2 |
|
#2514 has been created to fix the issue. |
Godbolt link to the preprocessed output of using the macro:
https://godbolt.org/z/rptA9J
(Scroll down to the bottom of the preprocessed output to see it parrotting the
noexceptspecifier verbatim)Fixes #2472
Let me know if y'all can think of any other test cases to throw at it.