Fix Windows compilation of test sources #10822

wrowe · 2020-04-17T03:00:55Z

Description:

This patch is extracted from #10542 and must be preceded by that merge in order for all enabled tests to compile successfully on Windows.

This patch should still be correct in the interim, and should effectively be a no-op on Linux.

Remove erroneous test from PR http2: support custom SETTINGS parameters #9964
- Test macro missing the second argument (for test name),
  causing compilation on Windows to fail. It appears to
  have no purpose, copy-paste error?
more involved compilation failures in test sources are
excluded with skip_on_windows tag
- involving tests to do with features that don't exist for
  windows (signals, etc.)
- sh_test rules do not currently work on Windows, waiting on a Bazel
  fix
Individually exclude tests in non-compiling extensions
with skip_on_windows
subset of failing tests tagged with fails_on_windows
- tests that time out
- tests that are known to fail
- non-exhaustive list, additional failing tests will be
  fixed/tagged in subsequent efforts
various fixes for Windows compilation
- fix os_fd_t type in mocks
- ensure consistent integer types
- proliferate use of OsSysCalls in tests
- exclude signals tests
- add _set_invalid_parameter_handler to test main to ensure runtime
  does not abort when functions are called with invalid parameters
- print warning about tcpdump not currently being enabled on Windows
- matchers_test.cc: do not return address of literal
- test/extensions/filters/network/direct_response/BUILD: skip test with
  constructor+lambda that does not compile on Windows
- codec_impl_test.cc: MSVC pre-processing does not like the multline
  string passed to EXPECT_EQ macro, pull into intermediate variable

Co-authored-by: Sunjay Bhatia sbhatia@pivotal.io
Co-authored-by: William A Rowe Jr wrowe@pivotal.io
Signed-off-by: Sunjay Bhatia sbhatia@pivotal.io
Signed-off-by: William A Rowe Jr wrowe@pivotal.io

Risk Level: Low
Testing: Local on msvc-cl, linux-gcc
Docs Changes: n/a
Release Notes: n/a
[Optional Fixes #Issue]
[Optional Deprecated:]

- Remove erroneous test from PR #9964 - Test macro missing the second argument (for test name), causing compilation on Windows to fail. It appears to have no purpose, copy-paste error? - more involved compilation failures in test sources are excluded with skip_on_windows tag - involving tests to do with features that don't exist for windows (signals, etc.) - sh_test rules do not currently work on Windows, waiting on a Bazel fix - Individually exclude tests in non-compiling extensions with skip_on_windows - subset of failing tests tagged with fails_on_windows - tests that time out - tests that are known to fail - non-exhaustive list, additional failing tests will be fixed/tagged in subsequent efforts - various fixes for Windows compilation - fix os_fd_t type in mocks - ensure consistent integer types - proliferate use of OsSysCalls in tests - exclude signals tests - add _set_invalid_parameter_handler to test main to ensure runtime does not abort when functions are called with invalid parameters - print warning about tcpdump not currently being enabled on Windows - matchers_test.cc: do not return address of literal - test/extensions/filters/network/direct_response/BUILD: skip test with constructor+lambda that does not compile on Windows - codec_impl_test.cc: MSVC pre-processing does not like the multline string passed to EXPECT_EQ macro, pull into intermediate variable Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io> Co-authored-by: William A Rowe Jr <wrowe@pivotal.io> Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io> Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>

This did not belong to the test/... changes for Windows and breaks the unix build, because 10542 introduced WINDOWS_SKIP_TARGETS. This logic applies to the build schema for Windows, not specific windows test fixes and exclusions. Signed-off-by: William A Rowe Jr <wrowe@pivotal.io> Co-authored-by: William A Rowe Jr <wrowe@pivotal.io> Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>

wrowe · 2020-04-17T06:26:38Z

/retest

repokitteh-read-only · 2020-04-17T06:26:42Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #10822 (comment) was created by @wrowe.

see: more, trace.

Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>

wrowe · 2020-04-17T17:44:10Z

Ready for review and merge. As we noted in PR 10542;

Before removing test/* changes from this PR, all tests passing, except for coverage (known to be unstable.)

Any coverage and macos regressions indicate new problems on master or issues with the CI build environments themselves, not this specific patch.

wrowe · 2020-04-18T01:10:41Z

/retest

repokitteh-read-only · 2020-04-18T01:10:45Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #10822 (comment) was created by @wrowe.

see: more, trace.

Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>

…lation Pick up possible fixes to coverage Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>

…lation Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>

yanavlasov · 2020-04-20T14:35:50Z

test/common/buffer/owned_impl_test.cc

+  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);


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

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.)

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?

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.

(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.)

mattklein123

Thanks, LGTM modulo some small comments. Thank you!

/wait

mattklein123 · 2020-04-20T22:07:55Z

test/extensions/filters/network/mysql_proxy/mysql_integration_test.cc

@@ -1,5 +1,8 @@
+#ifndef WIN32
 #include <pthread.h>


Is this actually needed on linux? I kind of doubt it. Can it just be removed?

Confirmed on gcc.

mattklein123 · 2020-04-20T22:09:27Z

test/main.cc

+
+  _set_invalid_parameter_handler(noop_invalid_parameter_handler);
+
+  WORD wVersionRequested;


nit: snake case var names here and below.

mattklein123 · 2020-04-20T22:10:05Z

test/common/buffer/owned_impl_test.cc

+  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);


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?

…lation Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>

Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>

Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io> Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>

…lation Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>

as suggested by mattklein123. Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>

Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>

wrowe · 2020-04-21T16:59:40Z

/retest

... likely won't retest for github 500's overload on the envoy-presubmit pipeline

repokitteh-read-only · 2020-04-21T16:59:43Z

🤷‍♀️ nothing to rebuild.

🐱

Caused by: a #10822 (comment) was created by @wrowe.

see: more, trace.

Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>

mattklein123

Thanks!

Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io> Co-authored-by: William A Rowe Jr <wrowe@pivotal.io> Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io> Signed-off-by: William A Rowe Jr <wrowe@pivotal.io> Signed-off-by: pengg <pengg@google.com>

wrowe requested review from alyssawilk, htuch, lizan, mattklein123, PiotrSikora and zuercher as code owners April 17, 2020 03:00

Kick CI

095d0c0

Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>

wrowe added 2 commits April 17, 2020 21:21

Force macos rebuild

396672a

Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>

Merge remote-tracking branch 'origin/master' into windows-tests-compi…

82c59d3

…lation Pick up possible fixes to coverage Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>

mattklein123 self-assigned this Apr 18, 2020

Merge remote-tracking branch 'origin/master' into windows-tests-compi…

cf507db

…lation Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>

yanavlasov reviewed Apr 20, 2020

View reviewed changes

yanavlasov self-assigned this Apr 20, 2020

mattklein123 requested changes Apr 20, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 20, 2020

wrowe and others added 4 commits April 21, 2020 00:39

Merge remote-tracking branch 'origin/master' into windows-tests-compi…

b1de7f3

…lation Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>

Snake case, suggested by mattklein123

9dd9824

Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>

Drop unnecessary header as observed by mattklein123

b016a5e

Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io> Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>

Merge remote-tracking branch 'origin/master' into windows-tests-compi…

9890540

…lation Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>

repokitteh-read-only bot removed the waiting label Apr 21, 2020

wrowe mentioned this pull request Apr 21, 2020

Replace pipe() with os_sys_calls_impl::socketpair(AF_UNIX,...) in buffer tests #10871

Open

wrowe requested a review from mattklein123 April 21, 2020 07:26

snakeCase variables (in origin and new copy)

daf42be

as suggested by mattklein123. Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>

Fix snake case

ad703ab

Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>

Rekick for github 500 overloads during last build

2125077

Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>

mattklein123 approved these changes Apr 21, 2020

View reviewed changes

mattklein123 merged commit 4c29cd5 into envoyproxy:master Apr 21, 2020

sunjayBhatia deleted the windows-tests-compilation branch April 21, 2020 20:17

rgs1 mentioned this pull request Apr 22, 2020

Crash regression from #10362 #10901

Closed

sunjayBhatia mentioned this pull request Apr 24, 2020

NT support #129

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Windows compilation of test sources #10822

Fix Windows compilation of test sources #10822

wrowe commented Apr 17, 2020

wrowe commented Apr 17, 2020

repokitteh-read-only bot commented Apr 17, 2020

wrowe commented Apr 17, 2020

wrowe commented Apr 18, 2020

repokitteh-read-only bot commented Apr 18, 2020

yanavlasov Apr 20, 2020

wrowe Apr 20, 2020

mattklein123 Apr 20, 2020

wrowe Apr 21, 2020 •

edited

Loading

wrowe Apr 21, 2020

mattklein123 Apr 21, 2020

mattklein123 left a comment

mattklein123 Apr 20, 2020

wrowe Apr 20, 2020

mattklein123 Apr 20, 2020

mattklein123 Apr 20, 2020

wrowe commented Apr 21, 2020

repokitteh-read-only bot commented Apr 21, 2020

mattklein123 left a comment


		_set_invalid_parameter_handler(noop_invalid_parameter_handler);

		WORD wVersionRequested;

Fix Windows compilation of test sources #10822

Fix Windows compilation of test sources #10822

Conversation

wrowe commented Apr 17, 2020

wrowe commented Apr 17, 2020

repokitteh-read-only bot commented Apr 17, 2020

wrowe commented Apr 17, 2020

wrowe commented Apr 18, 2020

repokitteh-read-only bot commented Apr 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wrowe Apr 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wrowe commented Apr 21, 2020

repokitteh-read-only bot commented Apr 21, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

wrowe Apr 21, 2020 •

edited

Loading