Skip to content

[Windows] Removes abstract mock classes Stream Encoder/Decoder#11480

Merged
mattklein123 merged 1 commit into
envoyproxy:masterfrom
davinci26:fix-mockStreamEncoderDecoder
Jun 10, 2020
Merged

[Windows] Removes abstract mock classes Stream Encoder/Decoder#11480
mattklein123 merged 1 commit into
envoyproxy:masterfrom
davinci26:fix-mockStreamEncoderDecoder

Conversation

@davinci26
Copy link
Copy Markdown
Member

@davinci26 davinci26 commented Jun 5, 2020

Signed-off-by: Sotiris Nanopoulos sonanopo@microsoft.com

Removes abstract mock classes Stream Encoder/Decoder

Additional Description:

As part of our effort to port Envoy to Windows we run into an issue with GTests and GMock in particular that is causing issues when we test on Windows. More specifically chromium documentation:

NiceMock and StrictMock only work for mock methods defined using the MOCK_METHOD macro directly in the MockFoo class. If a mock method is defined in a base class of MockFoo, the “nice” or “strict” modifier may not affect it, depending on the compiler.

The way I interpret it is that when you inherit from a Mock class you can not guarantee that the parent Mock class functions are treated as interesting calls. This leads to a GMock warning and consequently a test error. Example base mock classes are MockStreamEncoder and MockStreamDecoder

Tests that are now passing:

  • //test/common/http:conn_manager_impl_test
  • //test/common/http/http1:codec_impl_test
  • //test/common/http/http1:conn_pool_test
  • //test/common/http/http2:conn_pool_test
  • //test/common/http:async_client_impl_test
  • //test/common/router:router_test

Risk Level: Low (Test only)
Testing: Tested manually on Windows and Linux
Docs Changes: N/A
Release Notes: N/A

@davinci26
Copy link
Copy Markdown
Member Author

@junr03
Copy link
Copy Markdown
Member

junr03 commented Jun 5, 2020

@davinci26 could you fix DCO and formatting?

/wait

@davinci26
Copy link
Copy Markdown
Member Author

@junr03 I will do that.

Lemme wait until tomorrow morning to see if anyone has any other comments to batch them together and fix everything in a single iteration.

@davinci26 davinci26 force-pushed the fix-mockStreamEncoderDecoder branch from 7da45c0 to 725fe00 Compare June 6, 2020 01:26
@davinci26 davinci26 force-pushed the fix-mockStreamEncoderDecoder branch from 725fe00 to 50b13ae Compare June 8, 2020 06:32
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
@davinci26 davinci26 force-pushed the fix-mockStreamEncoderDecoder branch from 50b13ae to 76b90b1 Compare June 8, 2020 06:42
@davinci26
Copy link
Copy Markdown
Member Author

Sorry for the spam folks, configuring DCO turned out to be trickier than I expected

@davinci26
Copy link
Copy Markdown
Member Author

@junr03 ready for a review when you are 😀

@mattklein123 mattklein123 merged commit e3f616e into envoyproxy:master Jun 10, 2020
yashwant121 pushed a commit to yashwant121/envoy that referenced this pull request Jun 24, 2020
…y#11480)

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>
songhu pushed a commit to songhu/envoy that referenced this pull request Jun 25, 2020
…y#11480)

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
yashwant121 pushed a commit to yashwant121/envoy that referenced this pull request Jul 24, 2020
…y#11480)

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>
lizan pushed a commit that referenced this pull request Aug 21, 2020
)

Similar to #11480. Removes mock class inheritance in MockListenerFactoryContext which prohibits NiceMocks from behaving as expected on Windows\MSVC.

Additional Description:

Both tests have been passing locally in 1k runs.

Risk Level: Low (Test only)
Testing: N/A
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
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

Successfully merging this pull request may close these issues.

3 participants