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

Fix Windows compilation of test sources #10822

Merged
merged 13 commits into from
Apr 21, 2020
Merged
34 changes: 22 additions & 12 deletions test/common/buffer/owned_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -649,19 +649,24 @@ TEST_F(OwnedImplTest, ReserveZeroCommit) {
slices[i].len_ = 0;
}
buf.commit(slices, allocated_slices);
int pipe_fds[2] = {0, 0};
ASSERT_EQ(::pipe(pipe_fds), 0);
os_fd_t pipe_fds[2] = {0, 0};
auto& os_sys_calls = Api::OsSysCallsSingleton::get();
#ifdef WIN32
ASSERT_EQ(os_sys_calls.socketpair(AF_INET, SOCK_STREAM, 0, pipe_fds).rc_, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use socketpair for linux tests as well and avoid ifdefing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a question of whether we are trying to test tcp or unix domain sockets in the linux implementation. Even though we have resolved that we can have AF_UNIX sockets on windows, a nameless pipe still isn't available. We can stub that in later, but this is the shortest-path to a working test compilation.

So if there are a couple +1's to switching this to os_sys_calls.socketpair, we are fine with that, or we can leave it as-is. A very final step of this integration will be revisiting all of the ifdef/defined() WIN32 to evaluate whether or not they should remain (some of them predate our involvement.)

Copy link
Member

Choose a reason for hiding this comment

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

I think as all of these tests are buffer tests and not network tests, having them all use socket pair seems fine to me so let's just do that?

Copy link
Contributor Author

@wrowe wrowe Apr 21, 2020

Choose a reason for hiding this comment

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

Tracking this suggestion in issue #10871, which you can assign back to myself and @sunjayBhatia.

We would appreciate accepting this PR as-is, and we will revisit this after resolving the rest of the Windows CI //test/... RBE changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(I had forgotten that socketpair on Linux fails with AF_INET, so the Windows logic can't be used until we support AF_UNIX on Windows.)

Copy link
Member

Choose a reason for hiding this comment

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

Sure SGTM

#else
ASSERT_EQ(pipe(pipe_fds), 0);
#endif
Network::IoSocketHandleImpl io_handle(pipe_fds[0]);
ASSERT_EQ(::fcntl(pipe_fds[0], F_SETFL, O_NONBLOCK), 0);
ASSERT_EQ(::fcntl(pipe_fds[1], F_SETFL, O_NONBLOCK), 0);
ASSERT_EQ(os_sys_calls.setsocketblocking(pipe_fds[0], false).rc_, 0);
ASSERT_EQ(os_sys_calls.setsocketblocking(pipe_fds[1], false).rc_, 0);
const uint32_t max_length = 1953;
std::string data(max_length, 'e');
const ssize_t rc = ::write(pipe_fds[1], data.data(), max_length);
const ssize_t rc = os_sys_calls.write(pipe_fds[1], data.data(), max_length).rc_;
ASSERT_GT(rc, 0);
const uint32_t previous_length = buf.length();
Api::IoCallUint64Result result = buf.read(io_handle, max_length);
ASSERT_EQ(result.rc_, static_cast<uint64_t>(rc));
ASSERT_EQ(::close(pipe_fds[1]), 0);
ASSERT_EQ(os_sys_calls.close(pipe_fds[1]).rc_, 0);
ASSERT_EQ(previous_length, buf.search(data.data(), rc, previous_length));
EXPECT_EQ("bbbbb", buf.toString().substr(0, 5));
expectSlices({{5, 0, 4056}, {1953, 2103, 4056}}, buf);
Expand All @@ -672,19 +677,24 @@ TEST_F(OwnedImplTest, ReadReserveAndCommit) {
Buffer::OwnedImpl buf;
buf.add("bbbbb");

int pipe_fds[2] = {0, 0};
ASSERT_EQ(::pipe(pipe_fds), 0);
os_fd_t pipe_fds[2] = {0, 0};
auto& os_sys_calls = Api::OsSysCallsSingleton::get();
#ifdef WIN32
ASSERT_EQ(os_sys_calls.socketpair(AF_INET, SOCK_STREAM, 0, pipe_fds).rc_, 0);
#else
ASSERT_EQ(pipe(pipe_fds), 0);
#endif
Network::IoSocketHandleImpl io_handle(pipe_fds[0]);
ASSERT_EQ(::fcntl(pipe_fds[0], F_SETFL, O_NONBLOCK), 0);
ASSERT_EQ(::fcntl(pipe_fds[1], F_SETFL, O_NONBLOCK), 0);
ASSERT_EQ(os_sys_calls.setsocketblocking(pipe_fds[0], false).rc_, 0);
ASSERT_EQ(os_sys_calls.setsocketblocking(pipe_fds[1], false).rc_, 0);

const uint32_t read_length = 32768;
std::string data = "e";
const ssize_t rc = ::write(pipe_fds[1], data.data(), data.size());
const ssize_t rc = os_sys_calls.write(pipe_fds[1], data.data(), data.size()).rc_;
ASSERT_GT(rc, 0);
Api::IoCallUint64Result result = buf.read(io_handle, read_length);
ASSERT_EQ(result.rc_, static_cast<uint64_t>(rc));
ASSERT_EQ(::close(pipe_fds[1]), 0);
ASSERT_EQ(os_sys_calls.close(pipe_fds[1]).rc_, 0);
EXPECT_EQ("bbbbbe", buf.toString());
expectSlices({{6, 4050, 4056}}, buf);
}
Expand Down
6 changes: 6 additions & 0 deletions test/common/buffer/watermark_buffer_test.cc
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include <array>

#include "common/api/os_sys_calls_impl.h"
#include "common/buffer/buffer_impl.h"
#include "common/buffer/watermark_buffer.h"
#include "common/network/io_socket_handle_impl.h"
Expand Down Expand Up @@ -199,7 +200,12 @@ TEST_F(WatermarkBufferTest, MoveOneByte) {

TEST_F(WatermarkBufferTest, WatermarkFdFunctions) {
os_fd_t pipe_fds[2] = {0, 0};
#ifdef WIN32
auto& os_sys_calls = Api::OsSysCallsSingleton::get();
ASSERT_EQ(0, os_sys_calls.socketpair(AF_INET, SOCK_STREAM, 0, pipe_fds).rc_);
#else
ASSERT_EQ(0, pipe(pipe_fds));
#endif

buffer_.add(TEN_BYTES, 10);
buffer_.add(TEN_BYTES, 10);
Expand Down
2 changes: 2 additions & 0 deletions test/common/grpc/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,8 @@ envoy_cc_test_library(
envoy_cc_test(
name = "grpc_client_integration_test",
srcs = ["grpc_client_integration_test.cc"],
# Fails on windows; Connection is terminated early testing BasicStream/IPv4_EnvoyGrpc
tags = ["fails_on_windows"],
deps = [
":grpc_client_integration_test_harness_lib",
"//source/common/grpc:async_client_lib",
Expand Down
2 changes: 2 additions & 0 deletions test/common/http/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ envoy_cc_test(
envoy_cc_test(
name = "codec_client_test",
srcs = ["codec_client_test.cc"],
# IpVersions/CodecNetworkTest.SendData/IPv4: Test times out on Windows.
tags = ["fails_on_windows"],
deps = [
":common_lib",
"//source/common/buffer:buffer_lib",
Expand Down
3 changes: 0 additions & 3 deletions test/common/http/http2/codec_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -987,9 +987,6 @@ TEST_P(Http2CodecImplTest, WatermarkUnderEndStream) {
response_encoder_->encodeHeaders(response_headers, true);
}

class Http2CodecImplSettingsBasicTest : public Http2CodecImplTest {};
TEST_P(Http2CodecImplSettingsBasicTest, ) {}

class Http2CodecImplStreamLimitTest : public Http2CodecImplTest {};

// Regression test for issue #3076.
Expand Down
4 changes: 4 additions & 0 deletions test/common/network/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@ envoy_cc_test(
envoy_cc_test(
name = "connection_impl_test",
srcs = ["connection_impl_test.cc"],
# Times out on Windows
tags = ["fails_on_windows"],
deps = [
"//source/common/buffer:buffer_lib",
"//source/common/common:empty_string",
Expand Down Expand Up @@ -177,6 +179,8 @@ envoy_cc_test(
envoy_cc_test(
name = "listener_impl_test",
srcs = ["listener_impl_test.cc"],
# Times out on Windows
tags = ["fails_on_windows"],
deps = [
"//source/common/event:dispatcher_lib",
"//source/common/network:address_lib",
Expand Down
12 changes: 8 additions & 4 deletions test/common/network/dns_impl_test.cc
Original file line number Diff line number Diff line change
@@ -1,13 +1,10 @@
#include <arpa/inet.h>
#include <arpa/nameser.h>
#include <arpa/nameser_compat.h>

#include <list>
#include <memory>
#include <string>
#include <unordered_map>
#include <vector>

#include "envoy/common/platform.h"
#include "envoy/config/core/v3/address.pb.h"
#include "envoy/event/dispatcher.h"
#include "envoy/network/address.h"
Expand All @@ -34,6 +31,13 @@
#include "ares_dns.h"
#include "gtest/gtest.h"

#if !defined(WIN32)
#include <arpa/nameser.h>
#include <arpa/nameser_compat.h>
#else
#include "nameser.h"
#endif

using testing::_;
using testing::Contains;
using testing::InSequence;
Expand Down
2 changes: 2 additions & 0 deletions test/common/router/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,8 @@ envoy_cc_fuzz_test(
name = "route_fuzz_test",
srcs = ["route_fuzz_test.cc"],
corpus = ":route_corpus",
# The corpus_from_config_impl currently does not work, the tests it runs do not pass
tags = ["skip_on_windows"],
deps = [
":route_fuzz_proto_cc_proto",
"//source/common/router:config_lib",
Expand Down
5 changes: 5 additions & 0 deletions test/common/runtime/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ envoy_cc_test_library(
envoy_cc_test(
name = "runtime_protos_test",
srcs = ["runtime_protos_test.cc"],
# Pass for the time being, test times out on windows
tags = ["fails_on_windows"],
deps = [
"//source/common/runtime:runtime_lib",
"//test/mocks/runtime:runtime_mocks",
Expand All @@ -42,6 +44,9 @@ envoy_cc_test(
name = "runtime_impl_test",
srcs = ["runtime_impl_test.cc"],
data = glob(["test_data/**"]) + ["filesystem_setup.sh"],
# Inexplicable failure promoting arguments to mock, see
# https://envoyproxy.slack.com/archives/CNAK09BSB/p1571946165007300
tags = ["fails_on_windows"],
deps = [
"//source/common/config:runtime_utility_lib",
"//source/common/runtime:runtime_lib",
Expand Down
6 changes: 5 additions & 1 deletion test/common/signal/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,11 @@ envoy_package()
envoy_cc_test(
name = "signals_test",
srcs = ["signals_test.cc"],
tags = ["backtrace"],
# Posix signal tests are irrelevant to Windows
tags = [
"backtrace",
"skip_on_windows",
],
deps = [
"//source/common/signal:sigaction_lib",
"//test/test_common:utility_lib",
Expand Down
18 changes: 16 additions & 2 deletions test/exe/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -17,23 +17,35 @@ envoy_sh_test(
"//bazel:raw_build_id.ldscript",
"//source/exe:envoy-static",
],
# The sh_test helper from Bazel does not work as expected, see: https://github.com/bazelbuild/bazel/issues/10959
tags = ["fails_on_windows"],
)

envoy_sh_test(
name = "envoy_static_test",
srcs = ["envoy_static_test.sh"],
coverage = False,
data = ["//source/exe:envoy-static"],
# For windows, we expect to use a .ps1 script that leverages dumpbin.exe, see:
# https://github.com/envoyproxy/envoy/pull/8280#pullrequestreview-290187328
# The sh_test helper from Bazel does not work as expected, see: https://github.com/bazelbuild/bazel/issues/10959
# Sanitizers doesn't like statically linked lib(std)c++ and libgcc, skip this test in that context.
tags = ["no_san"],
tags = [
"fails_on_windows",
"no_san",
],
)

envoy_sh_test(
name = "pie_test",
srcs = ["pie_test.sh"],
coverage = False,
data = ["//source/exe:envoy-static"],
tags = ["nofips"],
# Since VS2015 or even earlier, link.exe defaults to PIE generation
tags = [
"nofips",
"skip_on_windows",
],
)

envoy_sh_test(
Expand All @@ -45,6 +57,8 @@ envoy_sh_test(
"//bazel:raw_build_id.ldscript",
"//source/exe:envoy-static",
],
# The sh_test helper from Bazel does not work as expected, see: https://github.com/bazelbuild/bazel/issues/10959
tags = ["fails_on_windows"],
)

envoy_cc_test(
Expand Down
17 changes: 13 additions & 4 deletions test/exe/main_common_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,12 @@ class MainCommonTest : public testing::TestWithParam<Network::Address::IpVersion
// Pick a prime number to give more of the 32-bits of entropy to the PID, and the
// remainder to the random number.
const uint32_t four_digit_prime = 7919;
#ifdef WIN32
return ::GetCurrentProcessId() * four_digit_prime +
random_generator_.random() % four_digit_prime;
#else
return getpid() * four_digit_prime + random_generator_.random() % four_digit_prime;
#endif
}

const char* const* argv() { return &argv_[0]; }
Expand Down Expand Up @@ -121,12 +126,13 @@ TEST_P(MainCommonDeathTest, OutOfMemoryHandler) {
"MainCommonTest::OutOfMemoryHandler not supported by this compiler configuration");
#else
MainCommon main_common(argc(), argv());
#if !defined(WIN32)
// Resolving symbols for a backtrace takes longer than the timeout in coverage builds,
// so disable handling that signal.
signal(SIGABRT, SIG_DFL);
#endif
EXPECT_DEATH_LOG_TO_STDERR(
[]() {
// Resolving symbols for a backtrace takes longer than the timeout in coverage builds,
// so disable handling that signal.
signal(SIGABRT, SIG_DFL);

// Allocating a fixed-size large array that results in OOM on gcc
// results in a compile-time error on clang of "array size too big",
// so dynamically find a size that is too large.
Expand Down Expand Up @@ -245,6 +251,8 @@ TEST_P(AdminRequestTest, AdminRequestGetStatsAndQuit) {
EXPECT_TRUE(waitForEnvoyToExit());
}

// no signals on Windows -- could probably make this work with GenerateConsoleCtrlEvent
#ifndef WIN32
// This test is identical to the above one, except that instead of using an admin /quitquitquit,
// we send ourselves a SIGTERM, which should have the same effect.
TEST_P(AdminRequestTest, AdminRequestGetStatsAndKill) {
Expand Down Expand Up @@ -302,6 +310,7 @@ TEST_P(AdminRequestTest, AdminRequestContentionEnabled) {
kill(getpid(), SIGTERM);
EXPECT_TRUE(waitForEnvoyToExit());
}
#endif

TEST_P(AdminRequestTest, AdminRequestBeforeRun) {
// Induce the situation where the Envoy thread is active, and main_common_ is constructed,
Expand Down
2 changes: 2 additions & 0 deletions test/extensions/common/wasm/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ envoy_cc_test(
data = [
"//test/extensions/common/wasm/test_data:modules",
],
# wasm (wee v8 etc) will not compile on Windows
tags = ["skip_on_windows"],
deps = [
"//source/extensions/common/wasm:wasm_vm_lib",
"//test/test_common:environment_lib",
Expand Down
9 changes: 6 additions & 3 deletions test/extensions/filters/common/rbac/matchers_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -248,9 +248,10 @@ TEST(AuthenticatedMatcher, uriSanPeerCertificate) {

const std::vector<std::string> uri_sans{"foo", "baz"};
const std::vector<std::string> dns_sans;
const std::string subject = "subject";
EXPECT_CALL(*ssl, uriSanPeerCertificate()).WillRepeatedly(Return(uri_sans));
EXPECT_CALL(*ssl, dnsSansPeerCertificate()).WillRepeatedly(Return(dns_sans));
EXPECT_CALL(*ssl, subjectPeerCertificate()).WillRepeatedly(ReturnRef("subject"));
EXPECT_CALL(*ssl, subjectPeerCertificate()).WillRepeatedly(ReturnRef(subject));

EXPECT_CALL(Const(conn), ssl()).WillRepeatedly(Return(ssl));

Expand All @@ -272,14 +273,15 @@ TEST(AuthenticatedMatcher, dnsSanPeerCertificate) {

const std::vector<std::string> uri_sans{"uri_foo"};
const std::vector<std::string> dns_sans{"foo", "baz"};
const std::string subject = "subject";

EXPECT_CALL(*ssl, uriSanPeerCertificate()).WillRepeatedly(Return(uri_sans));
EXPECT_CALL(Const(conn), ssl()).WillRepeatedly(Return(ssl));

EXPECT_CALL(*ssl, dnsSansPeerCertificate()).WillRepeatedly(Return(dns_sans));
EXPECT_CALL(Const(conn), ssl()).WillRepeatedly(Return(ssl));

EXPECT_CALL(*ssl, subjectPeerCertificate()).WillRepeatedly(ReturnRef("subject"));
EXPECT_CALL(*ssl, subjectPeerCertificate()).WillRepeatedly(ReturnRef(subject));

// We should get check if any DNS SAN matches as URI SAN is not available.
envoy::config::rbac::v3::Principal::Authenticated auth;
Expand Down Expand Up @@ -374,9 +376,10 @@ TEST(PolicyMatcher, PolicyMatcher) {

const std::vector<std::string> uri_sans{"bar", "baz"};
const std::vector<std::string> dns_sans;
const std::string subject = "subject";
EXPECT_CALL(*ssl, uriSanPeerCertificate()).Times(4).WillRepeatedly(Return(uri_sans));
EXPECT_CALL(*ssl, dnsSansPeerCertificate()).WillRepeatedly(Return(dns_sans));
EXPECT_CALL(*ssl, subjectPeerCertificate()).WillRepeatedly(ReturnRef("subject"));
EXPECT_CALL(*ssl, subjectPeerCertificate()).WillRepeatedly(ReturnRef(subject));

EXPECT_CALL(Const(conn), ssl()).Times(2).WillRepeatedly(Return(ssl));
EXPECT_CALL(Const(info), downstreamLocalAddress()).Times(2).WillRepeatedly(ReturnRef(addr));
Expand Down
Loading