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

sunjayBhatia · 2020-06-03T16:38:27Z

Commit Message: Windows: Fixing straightforward issues with //test/common/network/...
Additional Description:

Do not use SO_REUSEADDR on Windows as it does not behave the same as Linux and other BSD socket stacks
Disable pipe mode test on Windows, see Windows: Support setting permissions on Unix domain sockets #11354
socket_option_factory_test no longer hard codes socket option value as struct linger consists of ushorts not ints on Windows
allocate callback handles on heap instead of stack to work around MSVC
enable address_impl_speed_test_benchmark_test
additionally, tag new failing tests on Windows

Risk Level: Low
Testing: Fixes Windows invocations of tests, excludes some on Windows, ran locally on Windows w/ MSVC
Docs Changes: N/A
Release Notes: N/A

Do not use SO_REUSEADDR on Windows Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>

See: #11354 Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io> Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io> Signed-off-by: William A Rowe Jr <wrowe@pivotal.io> Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>

struct linger contains ushorts not ints on Windows, so instead of hard coding linger buffer value, construct one and generate string representation on the fly. Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io> Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io> Signed-off-by: William A Rowe Jr <wrowe@pivotal.io> Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>

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

returning a raw struct from createCallbackOnPort caused invalid memory access issues with msvc on Windows when the mocked methods were called, instead return a pointer Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: William A Rowe Jr <wrowe@vmware.com> Co-authored-by: William A Rowe Jr <wrowe@vmware.com>

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: William A Rowe Jr <wrowe@vmware.com> Co-authored-by: William A Rowe Jr <wrowe@vmware.com>

sunjayBhatia · 2020-06-03T16:38:36Z

cc @wrowe

Surface additional failing tests for visibility before we get Windows running in CI. Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: William A Rowe Jr <wrowe@vmware.com> Co-authored-by: William A Rowe Jr <wrowe@vmware.com>

…ng-test-common-network Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: William A Rowe Jr <wrowe@vmware.com> Co-authored-by: William A Rowe Jr <wrowe@vmware.com>

…-common-network Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: William A Rowe Jr <wrowe@vmware.com> Co-authored-by: William A Rowe Jr <wrowe@vmware.com>

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: William A Rowe Jr <wrowe@vmware.com> Co-authored-by: William A Rowe Jr <wrowe@vmware.com>

ggreenway

This looks fine to me, assuming it passes CI. Nice to remove some fails_on_windows tags.

ggreenway · 2020-06-04T16:45:37Z

test/mocks/server/mocks.h

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


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

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

That's really interesting. Thanks for the explanation!

ggreenway · 2020-06-04T16:46:45Z

MacOS CI failure is due to bazel version; not related to this PR.

nigriMSFT · 2020-06-04T18:47:11Z

cc @davinci26

mattklein123 · 2020-06-04T20:01:27Z

Please merge master to fix CI.

/wait

repokitteh-read-only · 2020-06-04T20:18:18Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/.

🐱

Caused by: #11423 was synchronize by sunjayBhatia.

see: more, trace.

ggreenway · 2020-06-04T20:21:57Z

Something went wrong with how you tried to merge master. You've added new, different commits with the same content as the commits on master. Please undo whatever you did and try again.

/wait

…-common-network Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: William A Rowe Jr <wrowe@vmware.com> Co-authored-by: William A Rowe Jr <wrowe@vmware.com>

sunjayBhatia · 2020-06-04T20:23:05Z

Something went wrong with how you tried to merge master. You've added new, different commits with the same content as the commits on master. Please undo whatever you did and try again.

yep, we're fixing it now

ggreenway · 2020-06-04T20:23:10Z

For reference, these are the commands I run to merge master, with upstream as this (the main) repository, starting with this PR branch checked out:

git fetch upstream master
git merge upstream/master

ggreenway

That looks better; thanks.

wrowe · 2020-06-04T20:25:35Z

For reference, these are the commands I run to merge master, with upstream as this (the main) repository, starting with this PR branch checked out:

Thanks, we had encountered a problem with the merge and buildifier spec, and in dropping a patch ended up with a rebase rather than merge of origin/master. Fixed, and (our apologies) force-pushed to resolve.

ggreenway · 2020-06-04T20:36:02Z

For reference, these are the commands I run to merge master, with upstream as this (the main) repository, starting with this PR branch checked out:

Thanks, we had encountered a problem with the merge and buildifier spec, and in dropping a patch ended up with a rebase rather than merge of origin/master. Fixed, and (our apologies) force-pushed to resolve.

No problem; git is easy to mess up.

wrowe · 2020-06-04T21:00:38Z

@ggreenway use azp rekick to try mac over again, looks like an intermittent I/O error.

ggreenway · 2020-06-04T21:04:00Z

/azp retest macOS

azure-pipelines · 2020-06-04T21:04:05Z

Command 'retest' is not supported by Azure Pipelines. Supported commands help: Get descriptions, examples and documentation about supported commands Example: help "command_name" list: List all pipelines for this repository using a comment. Example: "list" run: Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run. Example: "run" or "run pipeline_name, pipeline_name, pipeline_name" where: Report back the Azure DevOps orgs that are related to this repository and org Example: "where" See additional documentation.

ggreenway · 2020-06-04T21:05:43Z

/azp rekick macOS

azure-pipelines · 2020-06-04T21:05:47Z

Command 'rekick' is not supported by Azure Pipelines. Supported commands help: Get descriptions, examples and documentation about supported commands Example: help "command_name" list: List all pipelines for this repository using a comment. Example: "list" run: Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run. Example: "run" or "run pipeline_name, pipeline_name, pipeline_name" where: Report back the Azure DevOps orgs that are related to this repository and org Example: "where" See additional documentation.

ggreenway · 2020-06-04T21:06:24Z

/azp run macOS

azure-pipelines · 2020-06-04T21:06:29Z

No pipelines are associated with this pull request.

ggreenway · 2020-06-04T21:09:10Z

/azp list

azure-pipelines · 2020-06-04T21:09:15Z

CI/CD Pipelines for this repository: envoy-macos envoy-presubmit envoy-windows

ggreenway · 2020-06-04T21:09:31Z

/azp run envoy-macos

azure-pipelines · 2020-06-04T21:09:37Z

Azure Pipelines failed to run 1 pipeline(s).

ggreenway · 2020-06-04T21:30:46Z

/azp run

azure-pipelines · 2020-06-04T21:30:59Z

Azure Pipelines successfully started running 1 pipeline(s), but failed to run 2 pipeline(s).

wrowe · 2020-06-04T23:28:28Z

And... //test/integration:hotrestart_test timing out for Linux-x64 asan.

Will need to be re-kicked, curious first whether tsa/compile_time_options both pass, first.

mattklein123 · 2020-06-04T23:39:02Z

I've seen the hot restart tests timing out a lot recently, @jmarantz potentially due to your recent change? I can try to take a look tomorrow.

jmarantz · 2020-06-05T00:04:11Z

I just noticed hotrestart tests timing out but we haven't merged that change where I messing with it. That's #11301 .

jmarantz · 2020-06-05T00:55:17Z

When testing hotrestart_test locally on an unrelated PR, it passed in 210 seconds, but I could see the machines in CI might be a bit slower than my desktop. I'm going to write a small PR to add a size-tag to this test to see if that resolves the issue.

jmarantz · 2020-06-05T01:19:42Z

#11459

…envoyproxy#11423) Do not use SO_REUSEADDR on Windows as it does not behave the same as Linux and other BSD socket stacks Disable pipe mode test on Windows, see envoyproxy#11354 socket_option_factory_test no longer hard codes socket option value as struct linger consists of ushorts not ints on Windows allocate callback handles on heap instead of stack to work around MSVC enable address_impl_speed_test_benchmark_test additionally, tag new failing tests on Windows Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: William A Rowe Jr <wrowe@vmware.com> Co-authored-by: William A Rowe Jr <wrowe@vmware.com> Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>

sunjayBhatia and others added 6 commits June 3, 2020 12:08

Fix listen_socket_impl_test

f63a331

Do not use SO_REUSEADDR on Windows Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>

Disable pipe mode test on Windows

cf7e890

See: #11354 Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io> Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io> Signed-off-by: William A Rowe Jr <wrowe@pivotal.io> Co-authored-by: William A Rowe Jr <wrowe@pivotal.io>

Ensure shift is done on a 64 bit type

c419fc4

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

Enable address_impl_speed_test_benchmark_test

018f5e8

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: William A Rowe Jr <wrowe@vmware.com> Co-authored-by: William A Rowe Jr <wrowe@vmware.com>

mattklein123 assigned ggreenway Jun 3, 2020

sunjayBhatia and others added 2 commits June 4, 2020 10:10

sunjayBhatia requested review from dio, lizan and mattklein123 as code owners June 4, 2020 14:13

wrowe and others added 2 commits June 4, 2020 11:23

Fix const override specifier in mock

5851cdf

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: William A Rowe Jr <wrowe@vmware.com> Co-authored-by: William A Rowe Jr <wrowe@vmware.com>

ggreenway previously approved these changes Jun 4, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 4, 2020

sunjayBhatia dismissed ggreenway’s stale review via fd7b2dd June 4, 2020 20:18

sunjayBhatia requested a review from alyssawilk as a code owner June 4, 2020 20:18

repokitteh-read-only bot added api and removed waiting labels Jun 4, 2020

repokitteh-read-only bot added the waiting label Jun 4, 2020

repokitteh-read-only bot removed the waiting label Jun 4, 2020

ggreenway approved these changes Jun 4, 2020

View reviewed changes

wrowe mentioned this pull request Jun 5, 2020

Windows: unmark tests that are not failing on Windows #11460

Closed

htuch removed the api label Jun 5, 2020

mattklein123 merged commit f0464a4 into envoyproxy:master Jun 5, 2020

wrowe deleted the windows-fixing-test-common-network branch June 7, 2020 13:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sunjayBhatia commented Jun 3, 2020 •

edited

Loading

sunjayBhatia commented Jun 3, 2020

ggreenway left a comment

ggreenway Jun 4, 2020

sunjayBhatia Jun 4, 2020

ggreenway Jun 4, 2020

ggreenway commented Jun 4, 2020

nigriMSFT commented Jun 4, 2020

mattklein123 commented Jun 4, 2020

repokitteh-read-only bot commented Jun 4, 2020

ggreenway commented Jun 4, 2020

sunjayBhatia commented Jun 4, 2020

ggreenway commented Jun 4, 2020

ggreenway left a comment

wrowe commented Jun 4, 2020

ggreenway commented Jun 4, 2020

wrowe commented Jun 4, 2020

ggreenway commented Jun 4, 2020

azure-pipelines bot commented Jun 4, 2020

ggreenway commented Jun 4, 2020

azure-pipelines bot commented Jun 4, 2020

ggreenway commented Jun 4, 2020

azure-pipelines bot commented Jun 4, 2020

ggreenway commented Jun 4, 2020

azure-pipelines bot commented Jun 4, 2020

ggreenway commented Jun 4, 2020

azure-pipelines bot commented Jun 4, 2020

ggreenway commented Jun 4, 2020

azure-pipelines bot commented Jun 4, 2020

wrowe commented Jun 4, 2020

mattklein123 commented Jun 4, 2020

jmarantz commented Jun 5, 2020

jmarantz commented Jun 5, 2020 •

edited

Loading

jmarantz commented Jun 5, 2020

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

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

Conversation

sunjayBhatia commented Jun 3, 2020 • edited Loading

sunjayBhatia commented Jun 3, 2020

ggreenway left a comment

Choose a reason for hiding this comment

ggreenway Jun 4, 2020

Choose a reason for hiding this comment

sunjayBhatia Jun 4, 2020

Choose a reason for hiding this comment

ggreenway Jun 4, 2020

Choose a reason for hiding this comment

ggreenway commented Jun 4, 2020

nigriMSFT commented Jun 4, 2020

mattklein123 commented Jun 4, 2020

repokitteh-read-only bot commented Jun 4, 2020

ggreenway commented Jun 4, 2020

sunjayBhatia commented Jun 4, 2020

ggreenway commented Jun 4, 2020

ggreenway left a comment

Choose a reason for hiding this comment

wrowe commented Jun 4, 2020

ggreenway commented Jun 4, 2020

wrowe commented Jun 4, 2020

ggreenway commented Jun 4, 2020

azure-pipelines bot commented Jun 4, 2020

ggreenway commented Jun 4, 2020

azure-pipelines bot commented Jun 4, 2020

ggreenway commented Jun 4, 2020

azure-pipelines bot commented Jun 4, 2020

ggreenway commented Jun 4, 2020

azure-pipelines bot commented Jun 4, 2020

ggreenway commented Jun 4, 2020

azure-pipelines bot commented Jun 4, 2020

ggreenway commented Jun 4, 2020

azure-pipelines bot commented Jun 4, 2020

wrowe commented Jun 4, 2020

mattklein123 commented Jun 4, 2020

jmarantz commented Jun 5, 2020

jmarantz commented Jun 5, 2020 • edited Loading

jmarantz commented Jun 5, 2020

sunjayBhatia commented Jun 3, 2020 •

edited

Loading

jmarantz commented Jun 5, 2020 •

edited

Loading