Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Is interleaving EXPECT_CALL()s and calls to the mock function *really* undefined behavior? #2828

Closed
ElectricRCAircraftGuy opened this issue Apr 27, 2020 · 6 comments

Comments

@ElectricRCAircraftGuy
Copy link

ElectricRCAircraftGuy commented Apr 27, 2020

There seems to be a lot of confusion on this topic and the documentation really needs clarity. Please update the documentation to either validate this statement, or to correct it if it's wrong. There are several statements here, so I'll label the official Google one as "Wording 1 (official)" and the alternate possible wordings which others assert and which contradict this to be "Wording 2" and "Wording 3". Which of these 3 wordings is actually correct? I have personally seen tests which appear to run just fine which definitely go against the official "Wording 1", but I currently believe they are simply "undefined behavior which happens to work". What's the truth? Can you update the documentation please to provide examples and make this not a debated topic anymore?

Google documentation directly states:

Wording 1 (official)

"Important note: gMock requires expectations to be set before the mock functions are called, otherwise the behavior is undefined. In particular, you mustn't interleave EXPECT_CALL()s and calls to the mock functions" (https://github.com/google/googletest/blob/master/googlemock/docs/for_dummies.md#using-mocks-in-tests)

The part I especially want you to focus on is this part:

In particular, you mustn't interleave EXPECT_CALL()s and calls to the mock functions

Yet, I have personally seen the following appear to work just fine. Here, testMethod() calls the mocked myMockObj.myMethod(). Notice that here we are definitely interweaving calls to EXPECT_CALL() and the mocked method, which is exactly what the Google documentation above says is undefined behavior. This works just fine though! Is it undefined behavior that happens to work fine? Or, is it really not undefined behavior?

TEST(FooTest, testCaseName)
{
    MyMock myMockObj;
    ...
    EXPECT_CALL(myMockObj, myMethod(_)).WillOnce(Return(true));
    testMethod();
    ASSERT_THAT(...);

    EXPECT_CALL(myMockObj, myMethod(_)).WillOnce(Return(false));
    testMethod();
    ASSERT_THAT(...);
}

The code snippet above comes from @Marko Popovic's Stack Overflow answer here. In his answer, he asserts that doing the above code snippet is fine, and NOT undefined behavior, and thus implies that the Google documentation is wrong. His assertion seems to be the following (these are my own words, summarizing what he demonstrates):

Wording 2

Important note: gMock requires expectations to be set before the mock functions are called, otherwise the behavior is undefined. Furthermore, once you do an EXPECT_CALL() for a given mocked method, and then call that method, if you do another EXPECT_CALL() afterwards to then change the expectations set on future calls to that method, you must call the method again, or else the 2nd EXPECT_CALL() is now undefined behavior.

A colleague of mine has another theory, and I summarize his understanding this way:

Wording 3

Important note: gMock requires expectations to be set before the mock functions are called, otherwise the behavior is undefined. Therefore, interleaving EXPECT_CALL()s and calls to the mocked functions is just fine so long as you call at least one EXPECT_CALL() prior to any call to the mocked method being tested by this EXPECT_CALL(), and follow all other rules of EXPECT_CALL()s.

I have personally written a great deal about the rules of multiple EXPECT_CALL()s here, but have assumed the Google documentation to be correct (which maybe it isn't), when it says:

In particular, you mustn't interleave EXPECT_CALL()s and calls to the mock functions (https://github.com/google/googletest/blob/master/googlemock/docs/for_dummies.md#using-mocks-in-tests)

So, is my documentation correct? In particular this part?


[QUOTE OF MY OWN WRITING START]

Question 3: Can I call EXPECT_CALL to set some expectations on a mock method, call the mock method, then call EXPECT_CALL on the method again to change the expectations, then call the mock method again?

This question wasn't even explicitly asked by the OP, but the only reason I found this page is because I was searching for this answer for many hours and couldn't find it. My Google search was "[gmock multiple expect_call][10]." Therefore, others asking this question will also fall on this page and need a conclusive answer.

A: NO, you can NOT do this! Although it may seem to work in testing, according to Google, it produces undefined behavior. See general rule #2 above!

"Important note: gMock requires expectations to be set before the mock functions are called, otherwise the behavior is undefined. In particular, you mustn't interleave EXPECT_CALL()s and calls to the mock functions" (https://github.com/google/googletest/blob/master/googlemock/docs/for_dummies.md#using-mocks-in-tests)

Therefore, this is NOT ALLOWED!

[QUOTE OF MY OWN WRITING END]

I've added my own answer to Stack Overflow here too: https://stackoverflow.com/questions/40089204/interleaving-expect-calls-and-calls-to-the-mock-functions/61467271#61467271. I pose questions in my "answer" which led me to open this issue.

@ElectricRCAircraftGuy ElectricRCAircraftGuy changed the title Is interleaving EXPECT_CALL()s and calls to the mock function really undefined behavior? Is interleaving EXPECT_CALL()s and calls to the mock function *really* undefined behavior? Apr 27, 2020
@sbenzaquen
Copy link
Collaborator

sbenzaquen commented Apr 27, 2020 via email

@asoffer
Copy link
Contributor

asoffer commented Apr 28, 2020

As per Sam's comment, yes this is UB and something we do not intend to define.

@asoffer asoffer closed this as completed Apr 28, 2020
@ElectricRCAircraftGuy
Copy link
Author

ElectricRCAircraftGuy commented Apr 28, 2020

Thank you, Google contributors & collaborators, for responding, can you confirm or deny the following too please?

Perhaps this is NOT undefined behavior though!? I added Mock::VerifyAndClearExpectations(&myMockObj).

TEST(FooTest, testCaseName)
{
    MyMock myMockObj;
    ...
    EXPECT_CALL(myMockObj, myMethod(_)).WillOnce(Return(true));
    testMethod();
    ASSERT_THAT(...);

    Mock::VerifyAndClearExpectations(&myMockObj); // <== NOTICE THIS ADDED LINE!
    EXPECT_CALL(myMockObj, myMethod(_)).WillOnce(Return(false));
    testMethod();
    ASSERT_THAT(...);
}

@mliszcz
Copy link

mliszcz commented Oct 28, 2020

Hi GoogleTest maintainers. Would it be possible to get your feedback on the above question from @ElectricRCAircraftGuy?

One more use-case would be bringing system under test into desired state in the test fixture constructor. This cannot be worked around by simply reordering the statements. For now I used placement new and recreated the mocks but I don't like this solution.

struct MyTest : public Test
{
    MyServiceMock myService;
    MySystem mySystem;
    MyTest() :
        myService(),
        mySystem(&myService)
    {
        mySystem.goToStateX(); // this performs mock function calls on myService
        // Would calling VeryfyAndClearExpectations(&myService) here make it well-defined?
    }
};

TEST_F(MyTest, DoYInStateX)
{
    EXPECT_CALL(myService, ...);
    mySystem.doY();
}

pull bot pushed a commit to ZSX-JOJO/chromium that referenced this issue Jul 28, 2021
It's not recommended to mix assertions and mock function calls. See
google/googletest#2828.

This also prepares the test to be more readable to test better work
item accounting on macOS.

Bug: 1230156
Change-Id: I7080ae85cf8e4c579b406d030687a8b3e462a54b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3056530
Reviewed-by: Gabriel Charette <[email protected]>
Commit-Queue: Olivier Li <[email protected]>
Cr-Commit-Position: refs/heads/master@{#906032}
zeng450026937 pushed a commit to zeng450026937/base that referenced this issue Jul 30, 2021
It's not recommended to mix assertions and mock function calls. See
google/googletest#2828.

This also prepares the test to be more readable to test better work
item accounting on macOS.

Bug: 1230156
Change-Id: I7080ae85cf8e4c579b406d030687a8b3e462a54b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3056530
Reviewed-by: Gabriel Charette <[email protected]>
Commit-Queue: Olivier Li <[email protected]>
Cr-Commit-Position: refs/heads/master@{#906032}
NOKEYCHECK=True
GitOrigin-RevId: bef398b8e182b147b3d3cc7bf24f7e433c774740
@ElectricRCAircraftGuy
Copy link
Author

ElectricRCAircraftGuy commented May 31, 2022

@mliszcz and others, just to follow up, my code example here is also undefined gmock behavior!

See the updates I just added to my answer here: Interleaving EXPECT_CALL()s and calls to the mock functions is undefined behavior, and see this quote here:

From: https://google.github.io/googletest/gmock_cheat_sheet.html#verifying-and-resetting-a-mock (emphasis added):

Do not set new expectations after verifying and clearing a mock after its use. Setting expectations after code that exercises the mock has undefined behavior. See Using Mocks in Tests for more information.

(Thanks to @nnovich-OK for pointing the above quote out in the comments).

And also, from here: https://google.github.io/googletest/gmock_for_dummies.html#using-mocks-in-tests (with original emphasis from the original quote):

Important note: gMock requires expectations to be set before the mock functions are called, otherwise the behavior is undefined. Do not alternate between calls to EXPECT_CALL() and calls to the mock functions, and do not set any expectations on a mock after passing the mock to an API.

This means EXPECT_CALL() should be read as expecting that a call will occur in the future, not that a call has occurred. Why does gMock work like that? Well, specifying the expectation beforehand allows gMock to report a violation as soon as it rises, when the context (stack trace, etc) is still available. This makes debugging much easier.

@CsabaBeres
Copy link

CsabaBeres commented Aug 2, 2022

See the updates I just added to my answer here: Interleaving EXPECT_CALL()s and calls to the mock functions is undefined behavior

Is it still undefined behaviour if you use VerifyAndClear (instead of VerifyAndClearExpectations) ? I think it essentially re-creates the mock in place.
I tried to get answer about this myself, but I and my thread is ignored (justifiably).
UPDATE: I am afraid that Google states that VerifAndClear does not do the trick either:
https://google.github.io/googletest/gmock_cheat_sheet.html#verifying-and-resetting-a-mock

I am afraid that @mliszcz is right in this #2828 (comment)
It turned out to be very simple to implement a generic function which re-creates the mock in place, so we use that in a pilot to refresh the mocks and avoid the UB. The problem is that it is impossible to prove that this solution works (because you cannot write test to provoke UB, so you cannot prove that you solved/avoided it).
One solution could be a clear, defined way from Google to refresh the mocks.
In absence of that the framework will be much less useful, I fear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants