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.

Sign up for GitHub

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

Added missing expectations for EnginMasterTests #11309

Merged
merged 1 commit into from
Mar 6, 2023
Merged

Added missing expectations for EnginMasterTests #11309

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions src/test/enginemastertest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,10 @@ TEST_F(EngineMasterTest, SingleChannelOutputWorks) {
EXPECT_CALL(*pChannel, isPflEnabled())
.Times(1)
.WillOnce(Return(false));
EXPECT_CALL(*pChannel, collectFeatures(_))
.Times(1);
EXPECT_CALL(*pChannel, postProcess(160000))
.Times(1);
Comment on lines +76 to +79
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is just when the call is missing, it is likely a regression. I stumbled over it when digging into #11257

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@Swiftb0y Swiftb0y Feb 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// Instruct the mock to just return when process() gets called.
EXPECT_CALL(*pChannel, process(_, MAX_BUFFER_LEN))
Expand Down Expand Up @@ -110,6 +114,10 @@ TEST_F(EngineMasterTest, SingleChannelPFLOutputWorks) {
EXPECT_CALL(*pChannel, isPflEnabled())
.Times(1)
.WillOnce(Return(true));
EXPECT_CALL(*pChannel, collectFeatures(_))
.Times(1);
EXPECT_CALL(*pChannel, postProcess(160000))
.Times(1);

// Instruct the mock to just return when process() gets called.
EXPECT_CALL(*pChannel, process(_, _))
Expand Down Expand Up @@ -153,6 +161,10 @@ TEST_F(EngineMasterTest, TwoChannelOutputWorks) {
EXPECT_CALL(*pChannel1, isPflEnabled())
.Times(1)
.WillOnce(Return(false));
EXPECT_CALL(*pChannel1, collectFeatures(_))
.Times(1);
EXPECT_CALL(*pChannel1, postProcess(160000))
.Times(1);

// Instruct channel 2 to claim it is active, master and not PFL.
EXPECT_CALL(*pChannel2, isActive())
Expand All @@ -164,6 +176,10 @@ TEST_F(EngineMasterTest, TwoChannelOutputWorks) {
EXPECT_CALL(*pChannel2, isPflEnabled())
.Times(1)
.WillOnce(Return(false));
EXPECT_CALL(*pChannel2, collectFeatures(_))
.Times(1);
EXPECT_CALL(*pChannel2, postProcess(160000))
.Times(1);

// Instruct the mock to just return when process() gets called.
EXPECT_CALL(*pChannel1, process(_, MAX_BUFFER_LEN))
Expand Down Expand Up @@ -210,6 +226,10 @@ TEST_F(EngineMasterTest, TwoChannelPFLOutputWorks) {
EXPECT_CALL(*pChannel1, isPflEnabled())
.Times(1)
.WillOnce(Return(true));
EXPECT_CALL(*pChannel1, collectFeatures(_))
.Times(1);
EXPECT_CALL(*pChannel1, postProcess(160000))
.Times(1);

// Instruct channel 2 to claim it is active, master and PFL.
EXPECT_CALL(*pChannel2, isActive())
Expand All @@ -221,6 +241,10 @@ TEST_F(EngineMasterTest, TwoChannelPFLOutputWorks) {
EXPECT_CALL(*pChannel2, isPflEnabled())
.Times(1)
.WillOnce(Return(true));
EXPECT_CALL(*pChannel2, collectFeatures(_))
.Times(1);
EXPECT_CALL(*pChannel2, postProcess(160000))
.Times(1);

// Instruct the mock to just return when process() gets called.
EXPECT_CALL(*pChannel1, process(_, MAX_BUFFER_LEN))
Expand Down Expand Up @@ -272,6 +296,10 @@ TEST_F(EngineMasterTest, ThreeChannelOutputWorks) {
EXPECT_CALL(*pChannel1, isPflEnabled())
.Times(1)
.WillOnce(Return(false));
EXPECT_CALL(*pChannel1, collectFeatures(_))
.Times(1);
EXPECT_CALL(*pChannel1, postProcess(160000))
.Times(1);

// Instruct channel 2 to claim it is active, master and not PFL.
EXPECT_CALL(*pChannel2, isActive())
Expand All @@ -283,6 +311,10 @@ TEST_F(EngineMasterTest, ThreeChannelOutputWorks) {
EXPECT_CALL(*pChannel2, isPflEnabled())
.Times(1)
.WillOnce(Return(false));
EXPECT_CALL(*pChannel2, collectFeatures(_))
.Times(1);
EXPECT_CALL(*pChannel2, postProcess(160000))
.Times(1);

// Instruct channel 3 to claim it is active, master and not PFL.
EXPECT_CALL(*pChannel3, isActive())
Expand All @@ -294,6 +326,10 @@ TEST_F(EngineMasterTest, ThreeChannelOutputWorks) {
EXPECT_CALL(*pChannel3, isPflEnabled())
.Times(1)
.WillOnce(Return(false));
EXPECT_CALL(*pChannel3, collectFeatures(_))
.Times(1);
EXPECT_CALL(*pChannel3, postProcess(160000))
.Times(1);

// Instruct the mock to just return when process() gets called.
EXPECT_CALL(*pChannel1, process(_, MAX_BUFFER_LEN))
Expand Down Expand Up @@ -348,6 +384,10 @@ TEST_F(EngineMasterTest, ThreeChannelPFLOutputWorks) {
EXPECT_CALL(*pChannel1, isPflEnabled())
.Times(1)
.WillOnce(Return(true));
EXPECT_CALL(*pChannel1, collectFeatures(_))
.Times(1);
EXPECT_CALL(*pChannel1, postProcess(160000))
.Times(1);

// Instruct channel 2 to claim it is active, master and not PFL.
EXPECT_CALL(*pChannel2, isActive())
Expand All @@ -359,6 +399,10 @@ TEST_F(EngineMasterTest, ThreeChannelPFLOutputWorks) {
EXPECT_CALL(*pChannel2, isPflEnabled())
.Times(1)
.WillOnce(Return(true));
EXPECT_CALL(*pChannel2, collectFeatures(_))
.Times(1);
EXPECT_CALL(*pChannel2, postProcess(160000))
.Times(1);

// Instruct channel 3 to claim it is active, master and not PFL.
EXPECT_CALL(*pChannel3, isActive())
Expand All @@ -370,6 +414,10 @@ TEST_F(EngineMasterTest, ThreeChannelPFLOutputWorks) {
EXPECT_CALL(*pChannel3, isPflEnabled())
.Times(1)
.WillOnce(Return(true));
EXPECT_CALL(*pChannel3, collectFeatures(_))
.Times(1);
EXPECT_CALL(*pChannel3, postProcess(160000))
.Times(1);

// Instruct the mock to just return when process() gets called.
EXPECT_CALL(*pChannel1, process(_, MAX_BUFFER_LEN))
Expand Down