connect: regression testing bugfix #34112
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
| status_200_ = parser().getStatus() != Http::Http1::ParserStatus::Error && | ||
| parser().statusCode() == Http::Code::OK; |
There was a problem hiding this comment.
Why not put this operation into the status200() function rather than precalculating and storing? (If there is a reason, a comment explaining that reason would be good.)
There was a problem hiding this comment.
(I see the reason in #34096 now, so explanatory comment is the request!)
| // Strip the CONNECT upgrade. | ||
| std::string prefix_data; | ||
| const std::string hostname(default_request_headers_.getHostValue()); | ||
| ASSERT_TRUE(fake_upstream_connection_->waitForInexactRawData("\r\n\r\n", &prefix_data)); | ||
| EXPECT_EQ(absl::StrCat("CONNECT ", hostname, ":443 HTTP/1.1\r\n\r\n"), prefix_data); | ||
|
|
||
| std::string content_length = include_content_length ? "Content-Length: 0\r\n" : ""; |
There was a problem hiding this comment.
Super-nitty since it's just a test, but absl::string_view is good for this.
| // Regression tests https://github.com/envoyproxy/envoy/issues/34096 | ||
| TEST_P(Http11ConnectHttpIntegrationTest, DfpWithProxyAddressAndContentLength) { |
There was a problem hiding this comment.
Should the test without content length also still exist?
There was a problem hiding this comment.
Didn't get a response on this comment. Seems like both paths should be getting tested.
There was a problem hiding this comment.
yeah sorry we do that by default - all th eother tests test this with stripConnectUpgradeAndRespond()
|
I was thinking maybe the fix should be in Envoy's BalsaParser implementation? It kinda scares me that after the parser finishes executing an input, the headers_ object is cleared. Maybe the headers should just stay? |
|
yeah said on ucn chat, I'm hoping we can land a real fix in google3 ASAP at which point I'll put this on hold, and use it to regression test after import lands. |
|
/wait on decision making |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
alyssawilk
changed the title
|
balsa fix landed, so this is now a test-only change |
|
/retest |
| // Regression tests https://github.com/envoyproxy/envoy/issues/34096 | ||
| TEST_P(Http11ConnectHttpIntegrationTest, DfpWithProxyAddressAndContentLength) { |
There was a problem hiding this comment.
Didn't get a response on this comment. Seems like both paths should be getting tested.
Commit Message: n/a
Additional Description: n/a
Risk Level: n/a (test only)
Testing: yep
tests #34096