test: convert router v1 JSON test configs to v2 YAML by derekargueta · Pull Request #6332 · envoyproxy/envoy

derekargueta · 2019-03-20T20:25:20Z

Description: This pull request converts all v1 JSON config test snippets to v2 YAML. There are multiple reasons for this:

One way to load configs (parseRouteConfigurationFromV2Yaml)
v1 API is deprecated. This makes testing v2 changes easier, so that one does not have to muck with the v1 to v2 translation code (i.e. [v1.8.0 (Oct 4, 2018) deprecation] Remove features marked deprecated in #3838 #4616)
Remove another dependency on v1 API/JSON code
Consistency of YAML/JSON in these tests.

notes

1 test,TestBadCorsConfig, was removed in the process. It doesn't appear to violate any V2 API constraints so when passed through V2 parsing the test fails to throw any exceptions.
2 tests, BadRouteEntryConfigMissingPathSpecifier and BadRouteEntryConfigNoRedirectNoClusters, were swapped to use EXPECT_DEATH instead of EXPECT_THROW_WITH_MESSAGE. The exceptions previously thrown were part of rds_json.cc when converting v1 to v2.

Besides the above mentions, no test assertions were modified, only the test data to conform to v2.

Risk Level: Low
Docs Changes: No functional changes
Release Notes: No functional changes
Pt. 1/2 of #4616

Signed-off-by: Derek Argueta dereka@pinterest.com

Signed-off-by: Derek Argueta <dereka@pinterest.com>

derekargueta · 2019-03-20T20:41:16Z

hmm the two tests I switched to EXPECT_DEATH fail in the release build since they're not release asserts.

[  FAILED  ] BadHttpRouteConfigurationsTest.BadRouteEntryConfigMissingPathSpecifier
[  FAILED  ] BadHttpRouteConfigurationsTest.BadRouteEntryConfigNoAction

These seem like good test cases to retain, but wasn't sure whether to

switch them to expect assertions
modify config_impl.cc to throw an exception in these cases (these are covered by Protobuf validation, but those aren't tested here. Should I be using a different config load function that will run protobuf validations?)
remove these tests, relying on protobuf validation (don't think this would be right, as we should still exercise these paths)

mattklein123 · 2019-03-21T03:21:20Z

@derekargueta you are a hero, that's all I have to say.

derekargueta · 2019-03-21T23:00:02Z

So my update here: still a bit stuck on those 2 tests, trying to get them to throw exceptions again. It seems that the protobuf validation get's executed when running MessageUtil::Validate so I added that to parseRouteConfigurationFromV2Yaml after MessageUtil::loadFromYaml. When I try running that on route configs I get

bazel-out/darwin-fastbuild/bin/source/common/protobuf/_virtual_includes/utility_lib/common/protobuf/utility.h:226:10: error: use of undeclared identifier 'Validate'
    if (!Validate(message, &err)) {

Validate appears to come from Protobuf? Can't find its declaration/definition anywhere in Envoy source. I added the .pb.h files and bazel dependences, as specified by the docstring for MessateUtil::validate but still no go.

So my question (I suppose for @snowp) is am I moving in the right direction? Do we want to validate protobuf constraints as part of config tests? If we don't run validation, then malformed configs pass to config_impl.cc perfectly fine, just with default values I suppose.

derekargueta · 2019-03-21T23:02:01Z

FWIW the HCM config tests (my next target) are in a similar state - v1 JSON test configs that will throw errors during the v1-v2 translation phase, not protobuf validation.

Signed-off-by: Derek Argueta <dereka@pinterest.com>

derekargueta · 2019-03-22T04:45:41Z

Figured out how to apply proto validation here, so we should be green-light for test parity without relying on the exceptions thrown during the v1->v2 translation.

derekargueta · 2019-03-22T05:10:43Z

/retest

repokitteh-read-only · 2019-03-22T05:10:46Z

🙀 failed invoking rebuild of ci/circleci: coverage: 500 Internal Server Error

🐱

Caused by: a #6332 (comment) was created by @derekargueta.

see: more, trace.

snowp

Thanks, this looks great. Few comments/questions

snowp · 2019-03-22T19:04:34Z

test/common/router/config_impl_test.cc

      TestEnvironment::writeStringToFileForTest("direct_response_body", "Example text 3");

-  // A superset of v1_json, with API v2 direct-response configuration added.
  static const std::string v2_yaml = R"EOF(


minor nit but maybe this shouldn't reference v2 anymore?

snowp · 2019-03-22T19:05:21Z

test/common/router/config_impl_test.cc

-
  TestConfigImpl v2_yaml_config(parseRouteConfigurationFromV2Yaml(v2_yaml), factory_context_, true);
-  testConfig(v2_yaml_config, true);
+  testConfig(v2_yaml_config);


Since this is only invoked once now, maybe inline the lambda?

snowp · 2019-03-22T19:08:23Z

test/common/router/config_impl_test.cc

        route:
          cluster: some-cluster
-          idle_timeout: 0s
+          idle_timeout: 1s


Why did this change? Validation?

Yes, protobuf validation. (I'm no protobuf expert, but appears (validate.rules).duration.gt = {} is equivalent to > 0?)

I'm actually going to change this back to zero, but use EXPECT_THROW instead as that's the new expected behavior in v2. (ExplicitIdleTimeout below already tests a good idle_timeout so this test would be redundant)

Signed-off-by: Derek Argueta <dereka@pinterest.com>

snowp

Thanks!

mattklein123

Thanks, so amazing. One question.

/wait-any

mattklein123 · 2019-03-25T23:04:56Z

test/common/router/config_impl_test.cc

-  EXPECT_THROW_WITH_MESSAGE(
-      TestConfigImpl(parseRouteConfigurationFromJson(json), factory_context_, true), EnvoyException,
-      "routes must have redirect or one of cluster/cluster_header/weighted_clusters")
+  EXPECT_THROW(TestConfigImpl(parseRouteConfigurationFromV2Yaml(yaml), factory_context_, true),


Did this change because it's now handled by validation? Is it possible to include the validation error message? Same above?

Ah yeah, forgot to come back to these after figuring out protobuf validation. Handled now. Using _WITH_REGEX with the critical message part since the protobuf errors are pretty verbose

Signed-off-by: Derek Argueta <dereka@pinterest.com>

mattklein123

Thank you!!! HERO!

derekargueta added 3 commits March 20, 2019 12:04

convert router v1 JSON test configs to v2 YAML

4853290

Signed-off-by: Derek Argueta <dereka@pinterest.com>

add back in previously failing tests

f8ddc55

Signed-off-by: Derek Argueta <dereka@pinterest.com>

formatting

bb396c6

Signed-off-by: Derek Argueta <dereka@pinterest.com>

derekargueta changed the title ~~[test] convert router v1 JSON test configs to v2 YAML~~ test: convert router v1 JSON test configs to v2 YAML Mar 20, 2019

jmarantz requested a review from snowp March 21, 2019 17:40

jmarantz assigned snowp Mar 21, 2019

derekargueta added 2 commits March 21, 2019 21:20

fix failing tests

98a2442

Signed-off-by: Derek Argueta <dereka@pinterest.com>

formatting

73896c8

Signed-off-by: Derek Argueta <dereka@pinterest.com>

snowp suggested changes Mar 22, 2019

View reviewed changes

code review

9a5c3a1

Signed-off-by: Derek Argueta <dereka@pinterest.com>

derekargueta mentioned this pull request Mar 22, 2019

Convert v1 JSON test configs to v2 YAML #6362

Closed

snowp previously approved these changes Mar 25, 2019

View reviewed changes

derekargueta mentioned this pull request Mar 25, 2019

[v1.8.0 (Oct 4, 2018) deprecation] Remove features marked deprecated in #3838 #4616

Closed

mattklein123 self-assigned this Mar 25, 2019

mattklein123 reviewed Mar 25, 2019

View reviewed changes

repokitteh-read-only bot added the waiting:any label Mar 25, 2019

explicitly check error messages for protobuf validations

1d9f594

Signed-off-by: Derek Argueta <dereka@pinterest.com>

derekargueta dismissed snowp’s stale review via 1d9f594 March 26, 2019 22:32

repokitteh-read-only bot removed the waiting:any label Mar 26, 2019

derekargueta added 2 commits March 26, 2019 16:52

Merge branch 'master' into dereka/4616

0e64cd4

simplify an error message check to consistently pass

b4fabb0

Signed-off-by: Derek Argueta <dereka@pinterest.com>

mattklein123 approved these changes Mar 27, 2019

View reviewed changes

mattklein123 merged commit 2a2a886 into envoyproxy:master Mar 27, 2019

derekargueta deleted the dereka/4616 branch March 27, 2019 19:23

Conversation

derekargueta commented Mar 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

derekargueta commented Mar 20, 2019

Uh oh!

mattklein123 commented Mar 21, 2019

Uh oh!

derekargueta commented Mar 21, 2019

Uh oh!

derekargueta commented Mar 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

derekargueta commented Mar 22, 2019

Uh oh!

derekargueta commented Mar 22, 2019

Uh oh!

repokitteh-read-only bot commented Mar 22, 2019

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

derekargueta commented Mar 20, 2019 •

edited

Loading

derekargueta commented Mar 21, 2019 •

edited

Loading