test: moving websocket tests to using HTTP codec.#4143
test: moving websocket tests to using HTTP codec.#4143alyssawilk merged 5 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
dnoe
left a comment
dnoe
left a comment
There was a problem hiding this comment.
With my memory refreshed I see that "old style" refers to the styles of websocket support in Envoy described at https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/websocket. Maybe just refer to that at the top of the method so the context is obvious?
test/integration/fake_upstream.cc
Outdated
| return AssertionFailure() << body().toString() << " not equal to " << data; | ||
| } | ||
| } | ||
| return success; |
There was a problem hiding this comment.
tiny nit: Call this "succeeded" instead of success to make it abundantly clear "return success" doesn't actually mean what it sounds like.
| } | ||
|
|
||
| if (!old_style) { | ||
| // Check for and remove headers added by default for HTTP requests. |
There was a problem hiding this comment.
Can you hoist these comments so they're above the method and/or add a bit more about what the "style" is in old and new? old_style doesn't give that much context on its own.
There was a problem hiding this comment.
I've moved this to take the member variable and done a one-time comment there. Let me know if you'd still like comments in both the validate functions.
| bool old_style) { | ||
| Http::TestHeaderMapImpl proxied_response_headers(original_proxied_response_headers); | ||
| if (!old_style) { | ||
| // Check for and remove headers added by default for HTTP responses. |
There was a problem hiding this comment.
Same here, a comment above that concisely explains what the old_style behavior change is would be helpful.
| if (original_response_headers.ContentLength() == nullptr) { | ||
| ASSERT_STREQ(proxied_response_headers.TransferEncoding()->value().c_str(), "chunked"); | ||
| proxied_response_headers.removeTransferEncoding(); | ||
| } |
There was a problem hiding this comment.
Looks like the same check on ContentLength is repeated for request and response. Worth factoring it out?
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
* origin/master: fix flaky RBAC integration test. (envoyproxy#4147) header_map: copy constructor for HeaderMapImpl. (envoyproxy#4129) test: moving websocket tests to using HTTP codec. (envoyproxy#4143) upstream: init host hc value based on hc value from other priorities (envoyproxy#3959) Signed-off-by: Snow Pettersen <snowp@squareup.com>
2 participants
Description: Moving the websocket test over to using HTTP rather than TCP helpers. This makes it much easier to do the full protocol mesh when we have H2 websockets.
Risk Level: Low (test only)
Testing: test passes.
Docs Changes: n/a
Release Notes: n/a