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

Jump to bottom

Windows: Fixing straightforward issues with //test/common/network/... #11423

Merged
merged 11 commits into from
Jun 5, 2020
Merged

Windows: Fixing straightforward issues with //test/common/network/... #11423

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
f63a331
Fix listen_socket_impl_test
sunjayBhatia May 28, 2020
cf7e890
Disable pipe mode test on Windows
wrowe May 28, 2020
e7c1052
Fix socket_option_factory_test for Windows
wrowe May 29, 2020
c419fc4
Ensure shift is done on a 64 bit type
sunjayBhatia May 29, 2020
8d66d54
Fix filter matcher test
sunjayBhatia Jun 2, 2020
018f5e8
Enable address_impl_speed_test_benchmark_test
sunjayBhatia Jun 3, 2020
3a828f0
Windows: Tag additional failing tests
sunjayBhatia Jun 4, 2020
38c28fe
Merge branch 'windows-tag-additional-failing-tests' into windows-fixi…
wrowe Jun 4, 2020
016423d
Merge remote-tracking branch 'origin/master' into windows-fixing-test…
wrowe Jun 4, 2020
5851cdf
Fix const override specifier in mock
sunjayBhatia Jun 4, 2020
37ce036
Merge remote-tracking branch 'origin/master' into windows-fixing-test…
wrowe Jun 4, 2020
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
5 changes: 4 additions & 1 deletion source/common/network/listen_socket_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,15 @@ void ListenSocketImpl::setupSocket(const Network::Socket::OptionsSharedPtr& opti
template <>
void NetworkListenSocket<
NetworkSocketTrait<Address::SocketType::Stream>>::setPrebindSocketOptions() {

// On Windows, SO_REUSEADDR does not restrict subsequent bind calls when there is a listener as on
// Linux and later BSD socket stacks
#ifndef WIN32
int on = 1;
auto& os_syscalls = Api::OsSysCallsSingleton::get();
Api::SysCallIntResult status =
os_syscalls.setsockopt(io_handle_->fd(), SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on));
RELEASE_ASSERT(status.rc_ != -1, "failed to set SO_REUSEADDR socket option");
#endif
}

template <>
Expand Down
4 changes: 0 additions & 4 deletions test/common/network/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ envoy_cc_test_library(
envoy_cc_test(
name = "address_impl_test",
srcs = ["address_impl_test.cc"],
tags = ["fails_on_windows"],
deps = [
"//source/common/network:address_lib",
"//source/common/network:utility_lib",
Expand All @@ -59,7 +58,6 @@ envoy_cc_benchmark_binary(
envoy_benchmark_test(
name = "address_impl_speed_test_benchmark_test",
benchmark_binary = "address_impl_speed_test",
tags = ["fails_on_windows"],
)

envoy_cc_test(
Expand Down Expand Up @@ -167,7 +165,6 @@ envoy_cc_test(
envoy_cc_test(
name = "listen_socket_impl_test",
srcs = ["listen_socket_impl_test.cc"],
tags = ["fails_on_windows"],
deps = [
"//source/common/network:address_lib",
"//source/common/network:listen_socket_lib",
Expand Down Expand Up @@ -266,7 +263,6 @@ envoy_cc_test(
name = "socket_option_factory_test",
srcs = ["socket_option_factory_test.cc"],
external_deps = ["abseil_str_format"],
tags = ["fails_on_windows"],
deps = [
"//source/common/network:address_lib",
"//source/common/network:socket_option_factory_lib",
Expand Down
2 changes: 2 additions & 0 deletions test/common/network/address_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,7 @@ TEST(PipeInstanceTest, Basic) {
EXPECT_EQ(nullptr, address.ip());
}

#ifndef WIN32
TEST(PipeInstanceTest, BasicPermission) {
std::string path = TestEnvironment::unixDomainSocketPath("foo.sock");

Expand All @@ -342,6 +343,7 @@ TEST(PipeInstanceTest, BasicPermission) {
<< path << std::oct << "\t" << (stat_buf.st_mode & 07777) << std::dec << "\t"
<< (stat_buf.st_mode) << strerror(result.errno_);
}
#endif

TEST(PipeInstanceTest, PermissionFail) {
NiceMock<Api::MockOsSysCalls> os_sys_calls;
Expand Down
46 changes: 23 additions & 23 deletions test/common/network/filter_matcher_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,13 @@ struct CallbackHandle {
} // namespace
class ListenerFilterMatcherTest : public testing::Test {
public:
CallbackHandle createCallbackOnPort(int port) {
CallbackHandle handle;
handle.address_ = std::make_shared<Network::Address::Ipv4Instance>("127.0.0.1", port);
handle.socket_ = std::make_unique<MockConnectionSocket>();
handle.callback_ = std::make_unique<MockListenerFilterCallbacks>();
EXPECT_CALL(*handle.socket_, localAddress()).WillRepeatedly(ReturnRef(handle.address_));
EXPECT_CALL(*handle.callback_, socket()).WillRepeatedly(ReturnRef(*handle.socket_));
std::unique_ptr<CallbackHandle> createCallbackOnPort(int port) {
auto handle = std::make_unique<CallbackHandle>();
handle->address_ = std::make_shared<Network::Address::Ipv4Instance>("127.0.0.1", port);
handle->socket_ = std::make_unique<MockConnectionSocket>();
handle->callback_ = std::make_unique<MockListenerFilterCallbacks>();
EXPECT_CALL(*(handle->socket_), localAddress()).WillRepeatedly(ReturnRef(handle->address_));
EXPECT_CALL(*(handle->callback_), socket()).WillRepeatedly(ReturnRef(*(handle->socket_)));
return handle;
}
envoy::config::listener::v3::ListenerFilterChainMatchPredicate createPortPredicate(int port_start,
Expand All @@ -44,9 +44,9 @@ TEST_F(ListenerFilterMatcherTest, DstPortMatcher) {
auto handle79 = createCallbackOnPort(79);
auto handle80 = createCallbackOnPort(80);
auto handle81 = createCallbackOnPort(81);
EXPECT_FALSE(matcher->matches(*handle79.callback_));
EXPECT_TRUE(matcher->matches(*handle80.callback_));
EXPECT_FALSE(matcher->matches(*handle81.callback_));
EXPECT_FALSE(matcher->matches(*(handle79->callback_)));
EXPECT_TRUE(matcher->matches(*(handle80->callback_)));
EXPECT_FALSE(matcher->matches(*(handle81->callback_)));
}

TEST_F(ListenerFilterMatcherTest, AnyMatdcher) {
Expand All @@ -56,9 +56,9 @@ TEST_F(ListenerFilterMatcherTest, AnyMatdcher) {
auto handle79 = createCallbackOnPort(79);
auto handle80 = createCallbackOnPort(80);
auto handle81 = createCallbackOnPort(81);
EXPECT_TRUE(matcher->matches(*handle79.callback_));
EXPECT_TRUE(matcher->matches(*handle80.callback_));
EXPECT_TRUE(matcher->matches(*handle81.callback_));
EXPECT_TRUE(matcher->matches(*(handle79->callback_)));
EXPECT_TRUE(matcher->matches(*(handle80->callback_)));
EXPECT_TRUE(matcher->matches(*(handle81->callback_)));
}

TEST_F(ListenerFilterMatcherTest, NotMatcher) {
Expand All @@ -69,9 +69,9 @@ TEST_F(ListenerFilterMatcherTest, NotMatcher) {
auto handle79 = createCallbackOnPort(79);
auto handle80 = createCallbackOnPort(80);
auto handle81 = createCallbackOnPort(81);
EXPECT_TRUE(matcher->matches(*handle79.callback_));
EXPECT_FALSE(matcher->matches(*handle80.callback_));
EXPECT_TRUE(matcher->matches(*handle81.callback_));
EXPECT_TRUE(matcher->matches(*(handle79->callback_)));
EXPECT_FALSE(matcher->matches(*(handle80->callback_)));
EXPECT_TRUE(matcher->matches(*(handle81->callback_)));
}

TEST_F(ListenerFilterMatcherTest, OrMatcher) {
Expand All @@ -87,9 +87,9 @@ TEST_F(ListenerFilterMatcherTest, OrMatcher) {
auto handle443 = createCallbackOnPort(443);
auto handle3306 = createCallbackOnPort(3306);

EXPECT_FALSE(matcher->matches(*handle3306.callback_));
EXPECT_TRUE(matcher->matches(*handle80.callback_));
EXPECT_TRUE(matcher->matches(*handle443.callback_));
EXPECT_FALSE(matcher->matches(*(handle3306->callback_)));
EXPECT_TRUE(matcher->matches(*(handle80->callback_)));
EXPECT_TRUE(matcher->matches(*(handle443->callback_)));
}

TEST_F(ListenerFilterMatcherTest, AndMatcher) {
Expand All @@ -105,9 +105,9 @@ TEST_F(ListenerFilterMatcherTest, AndMatcher) {
auto handle443 = createCallbackOnPort(443);
auto handle3306 = createCallbackOnPort(3306);

EXPECT_FALSE(matcher->matches(*handle3306.callback_));
EXPECT_FALSE(matcher->matches(*handle80.callback_));
EXPECT_TRUE(matcher->matches(*handle443.callback_));
EXPECT_FALSE(matcher->matches(*(handle3306->callback_)));
EXPECT_FALSE(matcher->matches(*(handle80->callback_)));
EXPECT_TRUE(matcher->matches(*(handle443->callback_)));
}
} // namespace Network
} // namespace Envoy
} // namespace Envoy
16 changes: 9 additions & 7 deletions test/common/network/socket_option_factory_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,13 +130,20 @@ TEST_F(SocketOptionFactoryTest, TestBuildLiteralOptions) {
Protobuf::RepeatedPtrField<envoy::config::core::v3::SocketOption> socket_options_proto;
Envoy::Protobuf::TextFormat::Parser parser;
envoy::config::core::v3::SocketOption socket_option_proto;
struct linger expected_linger;
expected_linger.l_onoff = 1;
expected_linger.l_linger = 3456;
absl::string_view linger_bstr{reinterpret_cast<const char*>(&expected_linger),
sizeof(struct linger)};
std::string linger_bstr_formatted = testing::PrintToString(linger_bstr);
static const char linger_option_format[] = R"proto(
state: STATE_PREBIND
level: %d
name: %d
buf_value: "\x01\x00\x00\x00\x80\x0d\x00\x00"
buf_value: %s
)proto";
auto linger_option = absl::StrFormat(linger_option_format, SOL_SOCKET, SO_LINGER);
auto linger_option =
absl::StrFormat(linger_option_format, SOL_SOCKET, SO_LINGER, linger_bstr_formatted);
ASSERT_TRUE(parser.ParseFromString(linger_option, &socket_option_proto));
*socket_options_proto.Add() = socket_option_proto;
static const char keepalive_option_format[] = R"proto(
Expand All @@ -156,11 +163,6 @@ TEST_F(SocketOptionFactoryTest, TestBuildLiteralOptions) {
EXPECT_TRUE(option_details.has_value());
EXPECT_EQ(SOL_SOCKET, option_details->name_.level());
EXPECT_EQ(SO_LINGER, option_details->name_.option());
struct linger expected_linger;
expected_linger.l_onoff = 1;
expected_linger.l_linger = 3456;
absl::string_view linger_bstr{reinterpret_cast<const char*>(&expected_linger),
sizeof(struct linger)};
EXPECT_EQ(linger_bstr, option_details->value_);

option_details = socket_options->at(1)->getOptionDetails(
Expand Down
1 change: 1 addition & 0 deletions test/extensions/filters/http/compressor/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ envoy_extension_cc_test(
"compressor_filter_integration_test.cc",
],
extension_name = "envoy.filters.http.compressor",
tags = ["fails_on_windows"],
deps = [
"//source/extensions/compression/gzip/compressor:config",
"//source/extensions/compression/gzip/decompressor:config",
Expand Down
1 change: 1 addition & 0 deletions test/extensions/filters/http/decompressor/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ envoy_extension_cc_test(
"decompressor_filter_integration_test.cc",
],
extension_name = "envoy.filters.http.decompressor",
tags = ["fails_on_windows"],
deps = [
"//source/extensions/compression/gzip/compressor:config",
"//source/extensions/compression/gzip/decompressor:config",
Expand Down
1 change: 1 addition & 0 deletions test/extensions/filters/http/grpc_web/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ envoy_extension_cc_test(
name = "grpc_web_integration_test",
srcs = ["grpc_web_filter_integration_test.cc"],
extension_name = "envoy.filters.http.grpc_web",
tags = ["fails_on_windows"],
deps = [
"//source/common/buffer:buffer_lib",
"//source/common/http:header_map_lib",
Expand Down
1 change: 1 addition & 0 deletions test/extensions/filters/udp/dns_filter/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ envoy_extension_cc_test(
name = "dns_filter_integration_test",
srcs = ["dns_filter_integration_test.cc"],
extension_name = "envoy.filters.udp_listener.dns_filter",
tags = ["fails_on_windows"],
deps = [
":dns_filter_test_lib",
"//source/extensions/filters/udp/dns_filter:config",
Expand Down
1 change: 1 addition & 0 deletions test/integration/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1194,6 +1194,7 @@ envoy_cc_test(
srcs = [
"local_reply_integration_test.cc",
],
tags = ["fails_on_windows"],
deps = [
":http_integration_lib",
":http_protocol_integration_lib",
Expand Down
2 changes: 1 addition & 1 deletion test/mocks/server/mocks.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -691,7 +691,7 @@ class MockBootstrapExtensionFactory : public BootstrapExtensionFactory {
MOCK_METHOD(BootstrapExtensionPtr, createBootstrapExtension,
(const Protobuf::Message&, Configuration::ServerFactoryContext&), (override));
MOCK_METHOD(ProtobufTypes::MessagePtr, createEmptyConfigProto, (), (override));
MOCK_METHOD(std::string, name, (), (const override));
MOCK_METHOD(std::string, name, (), (const, override));
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain what's going on here? (I don't object to the change; I don't understand what is going on here, and why this broke windows).

Copy link
Member Author

Choose a reason for hiding this comment

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

MSVC doesn't accept the two specifiers without a comma, it errors with .\test/mocks/server/mocks.h(694): error C2338: const override cannot be recognized as a valid specification modifier., from the google test cheatsheet, it seems a comma separated list is the correct way to specify multiple keywords: https://github.com/google/googletest/blob/master/googlemock/docs/cheat_sheet.md#mocking-a-normal-class-mockclass

Copy link
Contributor

Choose a reason for hiding this comment

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

That's really interesting. Thanks for the explanation!

};

} // namespace Configuration
Expand Down