-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Refactor e2e bookinfo tests #4185
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
Refactor e2e bookinfo tests #4185
Conversation
vadimeisenbergibm
commented
Mar 11, 2018
- extract a common constant
- let the routing rules reside in different directories (required for tests for add istio.io tutorial yamls #3940)
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Assign the PR to them by writing The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
/test istio-unit-tests |
Codecov Report
@@ Coverage Diff @@
## master #4185 +/- ##
========================================
+ Coverage 71% 73% +3%
========================================
Files 318 313 -5
Lines 29478 28465 -1013
========================================
+ Hits 20649 20755 +106
+ Misses 7584 6428 -1156
- Partials 1245 1282 +37
Continue to review full report at Codecov.
|
Need to rethink this. |
/test istio-pilot-e2e |
/test istio-unit-tests |
/retest |
/test istio-unit-tests |
/test istio-unit-tests |
@ldemailly could you please review and approve? The |
/test e2e-rbac-auth |
bookinfoDbYaml = "samples/bookinfo/kube/bookinfo-db.yaml" | ||
bookinfoMysqlYaml = "samples/bookinfo/kube/bookinfo-mysql.yaml" | ||
bookinfoDetailsExternalServiceYaml = "samples/bookinfo/kube/bookinfo-details-v2.yaml" | ||
bookinfoSampleDir = "samples/bookinfo" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this is still a constant, which it is, I'm a bit unsure what the change does
(I can agree it'd would have been cleaner to have the common prefix separated vs copypasted, but the end result is still the same... so ... what does this part of the change do ?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a minor refactoring. The change prevents typing errors in the future files to be added:
bookinfoNewYaml = "samles/bookinfo/kube/bookinfo.yaml"
vs.
bookinfoNewYaml = bookinfoSamleDir + "/kube/bookinfo.yaml"
Both code snippets have a typo, p is missing in sample.
In the first case, it will not be caught by the compiler and will fail in the test.
In the second case, it will be caught by the compiler, which is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
except if you had a typo in
bookinfoSampleDir = "samles/bookinfo"
it won't either... but hopefully all the tests would
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but bookinfoSampleDir
you define only once. In the first case, you risk to make a typo in any new yaml you add.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you use the constant in the code to join the dir with the files, it's also only once; either 100% correct or 100% wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, this is what I did after your proposal - 4615a99
testMysqlRule = "route-rule-ratings-mysql.yaml" | ||
detailsExternalServiceRouteRule = "route-rule-details-v2.yaml" | ||
detailsExternalServiceEgressRule = "egress-rule-google-apis.yaml" | ||
allRule = bookinfoSampleDir + "/kube/route-rule-all-v1.yaml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is the real change, for rules
can you put the concatenation once in the code (ie change rulesDir or equiv) rather than add more hardcoded prefixes in all those contants, that seems going in the reverse direction (more hardcoded prefixes rather than less) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, this is the real change, to allow bookinfo yamls to reside in different directories.
can you put the concatenation once in the code
Good point, will do.
/test istio-pilot-e2e |
3 similar comments
/test istio-pilot-e2e |
/test istio-pilot-e2e |
/test istio-pilot-e2e |
/test istio-unit-tests |
/test istio-pilot-e2e |
@ldemailly Could you please approve this PR? |
/lgtm |
based on istio#4185
This is far from finished, but it reduces memory usage by ~10%. Pulling the following changes from github.com/istio/proxy: 2656f34 Update Envoy SHA to latest with MetricImpl optimizations (release-1.0). (istio#1939) Pulling the following changes from github.com/envoyproxy/envoy: c1cc68dda stats: refactoring MetricImpl without strings (istio#4190) 36809d80a fuzz: coverage profile generation instructions. (istio#4193) ba40cc933 upstream: rebuild cluster when health check config is changed (istio#4075) 05c0d52d3 build: use clang-6.0. (istio#4168) 01f403ec4 thrift_proxy: introduce header transport (istio#4082) 564d256fb tcp: allow connection pool callers to store protocol state (istio#4131) 3e1d643b9 configs: match latest API changes (istio#4185) bc6a10c2f Fix wrong mock function name. (istio#4187) e994c1c0b Bump yaml-cpp so it builds with Visual Studio 15.8 (istio#4182) 3d1325e89 Converting envoy configs to V2 (istio#2957) 8d0680feb Add timestamp to HealthCheckEvent definition (istio#4119) 497efb95b server: handle non-EnvoyExceptions safely if thrown in constructor. (istio#4173) 6d6fafdb3 config: strengthen validation for gRPC config sources. (istio#4171) 132302caf fuzz: reduce log level when running under fuzz engine. (istio#4161) 7c04ac255 test: fix V6EmptyOptions in coverage with IPv6 enable (istio#4155) 1b2219bd7 ci: remove deprecated bazel --batch option (istio#4166) 2db6a4ce1 ci: update clang to version 6.0 in the Ubuntu build image. (istio#4157) 71152b710 ratelimit: Add ratelimit custom response headers (istio#4015) 306287418 ssl: make Ssl::Connection const everywhere (istio#4179) 706e26238 Handle validation of non expiring tokens in jwt_authn filter (istio#4007) f06e9588a fuzz: tag trivial fuzzers with no_fuzz. (istio#4156) 27fb1d353 thrift_proxy: add service name matching to router implementation (istio#4130) 8c189a552 Make over provisioning factor configurable (istio#4003) 6c08fb43c Making test less flaky (istio#4149) 592775b7b fuzz: bare bones HCM fuzzer. (istio#4118) For istio#7912. Signed-off-by: Piotr Sikora <piotrsikora@google.com>
This is far from finished, but it reduces memory usage by ~10%. Pulling the following changes from github.com/istio/proxy: c9d2230 Update Envoy SHA to latest with MetricImpl optimizations. (istio#1938) a32a587 Add a check cache test for string map sub keys (istio#1931) Pulling the following changes from github.com/envoyproxy/envoy: c1cc68dda stats: refactoring MetricImpl without strings (istio#4190) 36809d80a fuzz: coverage profile generation instructions. (istio#4193) ba40cc933 upstream: rebuild cluster when health check config is changed (istio#4075) 05c0d52d3 build: use clang-6.0. (istio#4168) 01f403ec4 thrift_proxy: introduce header transport (istio#4082) 564d256fb tcp: allow connection pool callers to store protocol state (istio#4131) 3e1d643b9 configs: match latest API changes (istio#4185) bc6a10c2f Fix wrong mock function name. (istio#4187) e994c1c0b Bump yaml-cpp so it builds with Visual Studio 15.8 (istio#4182) 3d1325e89 Converting envoy configs to V2 (istio#2957) 8d0680feb Add timestamp to HealthCheckEvent definition (istio#4119) 497efb95b server: handle non-EnvoyExceptions safely if thrown in constructor. (istio#4173) 6d6fafdb3 config: strengthen validation for gRPC config sources. (istio#4171) 132302caf fuzz: reduce log level when running under fuzz engine. (istio#4161) 7c04ac255 test: fix V6EmptyOptions in coverage with IPv6 enable (istio#4155) 1b2219bd7 ci: remove deprecated bazel --batch option (istio#4166) 2db6a4ce1 ci: update clang to version 6.0 in the Ubuntu build image. (istio#4157) 71152b710 ratelimit: Add ratelimit custom response headers (istio#4015) 306287418 ssl: make Ssl::Connection const everywhere (istio#4179) 706e26238 Handle validation of non expiring tokens in jwt_authn filter (istio#4007) f06e9588a fuzz: tag trivial fuzzers with no_fuzz. (istio#4156) 27fb1d353 thrift_proxy: add service name matching to router implementation (istio#4130) 8c189a552 Make over provisioning factor configurable (istio#4003) 6c08fb43c Making test less flaky (istio#4149) 592775b7b fuzz: bare bones HCM fuzzer. (istio#4118) For istio#7912. Signed-off-by: Piotr Sikora <piotrsikora@google.com>
…). (#8161) This is far from finished, but it reduces memory usage by ~10%. Pulling the following changes from github.com/istio/proxy: 2656f34 Update Envoy SHA to latest with MetricImpl optimizations (release-1.0). (#1939) Pulling the following changes from github.com/envoyproxy/envoy: c1cc68dda stats: refactoring MetricImpl without strings (#4190) 36809d80a fuzz: coverage profile generation instructions. (#4193) ba40cc933 upstream: rebuild cluster when health check config is changed (#4075) 05c0d52d3 build: use clang-6.0. (#4168) 01f403ec4 thrift_proxy: introduce header transport (#4082) 564d256fb tcp: allow connection pool callers to store protocol state (#4131) 3e1d643b9 configs: match latest API changes (#4185) bc6a10c2f Fix wrong mock function name. (#4187) e994c1c0b Bump yaml-cpp so it builds with Visual Studio 15.8 (#4182) 3d1325e89 Converting envoy configs to V2 (#2957) 8d0680feb Add timestamp to HealthCheckEvent definition (#4119) 497efb95b server: handle non-EnvoyExceptions safely if thrown in constructor. (#4173) 6d6fafdb3 config: strengthen validation for gRPC config sources. (#4171) 132302caf fuzz: reduce log level when running under fuzz engine. (#4161) 7c04ac255 test: fix V6EmptyOptions in coverage with IPv6 enable (#4155) 1b2219bd7 ci: remove deprecated bazel --batch option (#4166) 2db6a4ce1 ci: update clang to version 6.0 in the Ubuntu build image. (#4157) 71152b710 ratelimit: Add ratelimit custom response headers (#4015) 306287418 ssl: make Ssl::Connection const everywhere (#4179) 706e26238 Handle validation of non expiring tokens in jwt_authn filter (#4007) f06e9588a fuzz: tag trivial fuzzers with no_fuzz. (#4156) 27fb1d353 thrift_proxy: add service name matching to router implementation (#4130) 8c189a552 Make over provisioning factor configurable (#4003) 6c08fb43c Making test less flaky (#4149) 592775b7b fuzz: bare bones HCM fuzzer. (#4118) For #7912. Signed-off-by: Piotr Sikora <piotrsikora@google.com>
This is far from finished, but it reduces memory usage by ~10%. Pulling the following changes from github.com/istio/proxy: c9d2230 Update Envoy SHA to latest with MetricImpl optimizations. (#1938) a32a587 Add a check cache test for string map sub keys (#1931) Pulling the following changes from github.com/envoyproxy/envoy: c1cc68dda stats: refactoring MetricImpl without strings (#4190) 36809d80a fuzz: coverage profile generation instructions. (#4193) ba40cc933 upstream: rebuild cluster when health check config is changed (#4075) 05c0d52d3 build: use clang-6.0. (#4168) 01f403ec4 thrift_proxy: introduce header transport (#4082) 564d256fb tcp: allow connection pool callers to store protocol state (#4131) 3e1d643b9 configs: match latest API changes (#4185) bc6a10c2f Fix wrong mock function name. (#4187) e994c1c0b Bump yaml-cpp so it builds with Visual Studio 15.8 (#4182) 3d1325e89 Converting envoy configs to V2 (#2957) 8d0680feb Add timestamp to HealthCheckEvent definition (#4119) 497efb95b server: handle non-EnvoyExceptions safely if thrown in constructor. (#4173) 6d6fafdb3 config: strengthen validation for gRPC config sources. (#4171) 132302caf fuzz: reduce log level when running under fuzz engine. (#4161) 7c04ac255 test: fix V6EmptyOptions in coverage with IPv6 enable (#4155) 1b2219bd7 ci: remove deprecated bazel --batch option (#4166) 2db6a4ce1 ci: update clang to version 6.0 in the Ubuntu build image. (#4157) 71152b710 ratelimit: Add ratelimit custom response headers (#4015) 306287418 ssl: make Ssl::Connection const everywhere (#4179) 706e26238 Handle validation of non expiring tokens in jwt_authn filter (#4007) f06e9588a fuzz: tag trivial fuzzers with no_fuzz. (#4156) 27fb1d353 thrift_proxy: add service name matching to router implementation (#4130) 8c189a552 Make over provisioning factor configurable (#4003) 6c08fb43c Making test less flaky (#4149) 592775b7b fuzz: bare bones HCM fuzzer. (#4118) For #7912. Signed-off-by: Piotr Sikora <piotrsikora@google.com>