Added missing expectations for EnginMasterTests #11309
Conversation
| EXPECT_CALL(*pChannel, collectFeatures(_)) | ||
| .Times(1); | ||
| EXPECT_CALL(*pChannel, postProcess(160000)) | ||
| .Times(1); |
There was a problem hiding this comment.
Do we really expect these to be called or is that just an artifact of the internals?
If so, there are better ways of dealing with that.
https://github.com/google/googletest/blob/3d787f5a0d58cfc37a0563bb15647a0d8aa2c1bf/docs/gmock_cook_book.md#knowing-when-to-expect-useoncall
There was a problem hiding this comment.
It is just when the call is missing, it is likely a regression. I stumbled over it when digging into #11257
There was a problem hiding this comment.
I don't really understand why ONE_CALL is an alternative.
So use ON_CALL by default, and only use EXPECT_CALL when you actually intend to verify that the call is made
In this case I want the later, so I think the solution is OK.
There was a problem hiding this comment.
Mhmm, the thing is that the other calls actually make sense. I don't understand why we need to verify that collectFeatures and postProcess calls have been made or what they even do or why its important for the test that those two methods have been called. Perhaps you can shed some light on that?
There was a problem hiding this comment.
For my understanding the purpose of these test is to detect significant changes in the code that may introduce regressions. All output should make the same calls.
The tests are pointless in terms of "testing a contract" or such which is the original purpose of a Mock.
I am currently working on finding the bug reported in #11257 where any additional test condition is welcome. But actually the main issue was the disturbing warning. This is fixed.
|
@Swiftb0y So this can be merged? |
|
Yes, I just wanted to give somebody else the opportunity to comment before merging. |
|
Thank you. |
This fixes warnings reported in #11308