test: fix bad route entry test cases on Windows by nigriMSFT · Pull Request #11830 · envoyproxy/envoy

nigriMSFT · 2020-07-01T00:18:11Z

Signed-off-by: Nick Grifka nigri@microsoft.com

Commit Message: test: fix bad route entry test cases on Windows
Additional Description:
Risk Level: Low
Testing: Ran test cases locally on Windows
Docs Changes: NA
Release Notes: NA

Signed-off-by: Nick Grifka <nigri@microsoft.com>

nigriMSFT · 2020-07-01T00:20:55Z

@lizan it seems ef7d81d change regarding yaml->protobuf conversion changed the expected exception message slightly for BadRouteEntryConfigPrefixAndPath and a few other test cases. On Windows, these test cases still seem to be producing the old exception message. Is this at all expected? Is it important that the exception message check be as specific as it is today? BadRouteEntryConfigPrefixAndPathAndRegex is an example of a more relaxed check that passes on both platforms.

For completeness, here is the expected vs. actual exception messages on Windows:

BadHttpRouteConfigurationsTest.BadRouteEntryConfigPrefixAndPath
expected: "invalid value oneof field 'path_specifier' is already set. Cannot set '(prefix|path)' for type oneof"
actual:   "invalid value oneof field 'path_specifier' is already set. Cannot set 'prefix' for type oneof"
BadHttpRouteConfigurationsTest.BadRouteEntryConfigPrefixAndRegex
expected: "invalid value oneof field 'path_specifier' is already set. Cannot set '(prefix|regex)' for type oneof"
actual:   "invalid value oneof field 'path_specifier' is already set. Cannot set 'prefix' for type oneof"
BadHttpRouteConfigurationsTest.BadRouteEntryConfigPathAndRegex
expected: "invalid value oneof field 'path_specifier' is already set. Cannot set '(path|regex)' for type oneof"
actual:   "invalid value oneof field 'path_specifier' is already set. Cannot set 'path' for type oneof"
BadHttpRouteConfigurationsTest.BadRouteEntryConfigPrefixAndPathAndRegex
expected: "invalid value oneof field 'path_specifier' is already set."
actual:   "invalid value oneof field 'path_specifier' is already set." (matches)

nigriMSFT · 2020-07-01T00:22:02Z

@davinci26 @sunjayBhatia @wrowe

dio · 2020-07-06T16:52:11Z

@nigriMSFT I think you need to merge master.

dio · 2020-07-06T16:53:40Z

test/common/router/config_impl_test.cc

      TestConfigImpl(parseRouteConfigurationFromYaml(yaml), factory_context_, true), EnvoyException,
-      "invalid value oneof field 'path_specifier' is already set. Cannot set '(prefix|path)' for "
-      "type oneof");
+      "invalid value oneof field 'path_specifier' is already set.");


So part of the message is missing/different when running on windows?

Yes. For this example specifically, we get "invalid value oneof field 'path_specifier' is already set. Cannot set 'prefix' for type oneof"

Isn't the (prefix|path) part a regex capture group for the EXPECT_THROW_WITH_REGEX? I looks like this failure is a symptom of googletest not supporting more advanced regex

Running this test locally I see:

[ RUN ] BadHttpRouteConfigurationsTest.BadRouteEntryConfigPrefixAndPath external/com_google_googletest/googletest/src/gtest-port.cc(860): error: Failed Syntax error at index 71 in simple regular expression "invalid value oneof field 'path_specifier' is already set. Cannot set '(prefix|path)' for type oneof": '(' is unsupported. Stack trace: ...

@sunjayBhatia is right. I've updated the PR to use GTEST_USES_SIMPLE_RE as we have done in other parts of the codebase, so that this can be simplified if/when google test supports capture groups Windows.

Realistically the only solve is the adoption of RE2, why Google test doesn't support Google RE2 is a little puzzling :-)

We can in fact shift as a project to pcre if we don't believe this is going to happen any time soon.

dio · 2020-07-06T23:21:14Z

@nigriMSFT Sorry, I know this is annoying, but you need to fix DCO https://github.com/envoyproxy/envoy/blob/master/CONTRIBUTING.md#fixing-dco 🙏🏽.

Signed-off-by: Nick Grifka <nigri@microsoft.com>

dio · 2020-07-06T23:57:11Z

Looks good. Running tools/code_format/code_format.py test/common/router/config_impl_test.cc should fix the formatting issue.

Format check failed, try apply following patch to fix:

diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc
index 2eab7fc6e..a0796a9f6 100644
--- a/test/common/router/config_impl_test.cc
+++ b/test/common/router/config_impl_test.cc
@@ -5109,12 +5109,12 @@ virtual_hosts:
   EXPECT_THAT_THROWS_MESSAGE(
       TestConfigImpl(parseRouteConfigurationFromYaml(yaml), factory_context_, true), EnvoyException,
       ::testing::AnyOf(
-        ::testing::ContainsRegex(
-          "invalid value oneof field 'path_specifier' is already set. Cannot set 'prefix' for "
-          "type oneof"),
-        ::testing::ContainsRegex(
-          "invalid value oneof field 'path_specifier' is already set. Cannot set 'path' for "
-          "type oneof")));
+          ::testing::ContainsRegex(
+              "invalid value oneof field 'path_specifier' is already set. Cannot set 'prefix' for "
+              "type oneof"),
+          ::testing::ContainsRegex(
+              "invalid value oneof field 'path_specifier' is already set. Cannot set 'path' for "
+              "type oneof")));
 #endif
 }
 
@@ -5157,12 +5157,12 @@ virtual_hosts:
   EXPECT_THAT_THROWS_MESSAGE(
       TestConfigImpl(parseRouteConfigurationFromYaml(yaml), factory_context_, true), EnvoyException,
       ::testing::AnyOf(
-        ::testing::ContainsRegex(
-          "invalid value oneof field 'path_specifier' is already set. Cannot set 'prefix' for "
-          "type oneof"),
-        ::testing::ContainsRegex(
-          "invalid value oneof field 'path_specifier' is already set. Cannot set 'regex' for "
-          "type oneof")));
+          ::testing::ContainsRegex(
+              "invalid value oneof field 'path_specifier' is already set. Cannot set 'prefix' for "
+              "type oneof"),
+          ::testing::ContainsRegex(
+              "invalid value oneof field 'path_specifier' is already set. Cannot set 'regex' for "
+              "type oneof")));
 #endif
 }
 
@@ -5205,12 +5205,12 @@ virtual_hosts:
   EXPECT_THAT_THROWS_MESSAGE(
       TestConfigImpl(parseRouteConfigurationFromYaml(yaml), factory_context_, true), EnvoyException,
       ::testing::AnyOf(
-        ::testing::ContainsRegex(
-          "invalid value oneof field 'path_specifier' is already set. Cannot set 'path' for "
-          "type oneof"),
-        ::testing::ContainsRegex(
-          "invalid value oneof field 'path_specifier' is already set. Cannot set 'regex' for "
-          "type oneof")));
+          ::testing::ContainsRegex(
+              "invalid value oneof field 'path_specifier' is already set. Cannot set 'path' for "
+              "type oneof"),
+          ::testing::ContainsRegex(
+              "invalid value oneof field 'path_specifier' is already set. Cannot set 'regex' for "
+              "type oneof")));
 #endif
 }

Signed-off-by: Nick Grifka <nigri@microsoft.com>

dio · 2020-07-10T00:08:49Z

/azp run envoy-presubmit

azure-pipelines · 2020-07-10T00:08:57Z

Azure Pipelines successfully started running 1 pipeline(s).

dio · 2020-07-13T14:52:55Z

/azp run envoy-presubmit

azure-pipelines · 2020-07-13T14:53:04Z

Azure Pipelines successfully started running 1 pipeline(s).

dio · 2020-07-14T22:24:42Z

@nigriMSFT sorry. Seems like you need to sync with head to pick some CI fixes.

Signed-off-by: Nick Grifka <nigri@microsoft.com>

nigriMSFT · 2020-07-18T00:02:16Z

@dio Linux-x64 clang_tidy CI job seemed to have hung on "cCmakeMakeRule external/envoy/bazel/foreign_cc/event/include;" but none of the other jobs did. I'm not sure what else I can do here but retry? Is this a known issue?

dio · 2020-07-18T00:08:41Z

/azp run envoy-presubmit

azure-pipelines · 2020-07-18T00:08:50Z

Azure Pipelines successfully started running 1 pipeline(s).

dio

Thanks!

Signed-off-by: Nick Grifka <nigri@microsoft.com> Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

Signed-off-by: Nick Grifka <nigri@microsoft.com> Signed-off-by: scheler <santosh.cheler@appdynamics.com>

test: fix bad route entry test cases on Windows

45055bd

Signed-off-by: Nick Grifka <nigri@microsoft.com>

ggreenway assigned dio Jul 6, 2020

dio reviewed Jul 6, 2020

View reviewed changes

Merge branch 'master' into badrouteconfig-test

5e103c1

Use compound match statement when GTEST_USES_SIMPLE_RE is defined

17cdb59

Signed-off-by: Nick Grifka <nigri@microsoft.com>

fix formatting

afb8c30

Signed-off-by: Nick Grifka <nigri@microsoft.com>

Merge branch 'master' into badrouteconfig-test

f79f165

Signed-off-by: Nick Grifka <nigri@microsoft.com>

dio approved these changes Jul 18, 2020

View reviewed changes

mattklein123 merged commit e7bdb1f into envoyproxy:master Jul 20, 2020

KBaichoo pushed a commit to KBaichoo/envoy that referenced this pull request Jul 30, 2020

test: fix bad route entry test cases on Windows (envoyproxy#11830)

de1a46c

Signed-off-by: Nick Grifka <nigri@microsoft.com> Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

scheler pushed a commit to scheler/envoy that referenced this pull request Aug 4, 2020

test: fix bad route entry test cases on Windows (envoyproxy#11830)

bd3818f

Signed-off-by: Nick Grifka <nigri@microsoft.com> Signed-off-by: scheler <santosh.cheler@appdynamics.com>

Conversation

nigriMSFT commented Jul 1, 2020

Uh oh!

nigriMSFT commented Jul 1, 2020

Uh oh!

nigriMSFT commented Jul 1, 2020

Uh oh!

dio commented Jul 6, 2020

Uh oh!

dio Jul 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nigriMSFT Jul 6, 2020

Choose a reason for hiding this comment

Uh oh!

sunjayBhatia Jul 6, 2020

Choose a reason for hiding this comment

Uh oh!

nigriMSFT Jul 6, 2020

Choose a reason for hiding this comment

Uh oh!

wrowe Jul 7, 2020

Choose a reason for hiding this comment

Uh oh!

dio commented Jul 6, 2020

Uh oh!

dio commented Jul 6, 2020

Uh oh!

dio commented Jul 10, 2020

Uh oh!

azure-pipelines bot commented Jul 10, 2020

Uh oh!

dio commented Jul 13, 2020

Uh oh!

azure-pipelines bot commented Jul 13, 2020

Uh oh!

dio commented Jul 14, 2020

Uh oh!

nigriMSFT commented Jul 18, 2020

Uh oh!

dio commented Jul 18, 2020

Uh oh!

azure-pipelines bot commented Jul 18, 2020

Uh oh!

dio left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dio Jul 6, 2020 •

edited

Loading