http: moving tests over to new accessors#11287
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
alyssawilk
requested review from
PiotrSikora,
dio,
lizan,
mattklein123 and
snowp
as code owners
| TEST(EnvoyQuicUtilsTest, HeadersConversion) { | ||
| spdy::SpdyHeaderBlock headers_block; | ||
| headers_block[":host"] = "www.google.com"; | ||
| headers_block[":authority"] = "www.google.com"; |
| EXPECT_CALL(callbacks_, encodeHeaders_(_, _)) | ||
| .WillOnce(Invoke([&](const Http::HeaderMap& headers, const bool) -> void { | ||
| EXPECT_EQ(headers.get(Http::Headers::get().SetCookie)->value().getStringView(), "foo=baz"); | ||
| EXPECT_EQ(std::string{headers.get(Http::Headers::get().SetCookie)->value().getStringView()}, |
There was a problem hiding this comment.
Good eye!
Sadly we can't change this one - setCookie is exempted from the O(1) headers list because it's not spec compliant to coalesce.
asraa
left a comment
asraa
left a comment
There was a problem hiding this comment.
Woah!! This takes the clean-up cake! There was just one potential missing conversion.
|
:-) It's entirely possible I spent too much time looking at Envoy test logs... |
asraa
left a comment
asraa
left a comment
There was a problem hiding this comment.
Sweet, this looks good to me now! (modulo the quic test change.)
| const Http::AsyncClient::RequestOptions&) { | ||
| http_callbacks_ = &callbacks; | ||
| EXPECT_EQ("POST", std::string(request->headers().Method()->value().getStringView())); | ||
| EXPECT_EQ("POST", std::string(request->headers().getMethodValue())); |
There was a problem hiding this comment.
I noticed on your other new accessor PR that these unnecessary std::strings could be moved to a beginner clean-up task. This is just test code, so it's nbd, but I'd add this as clean-up in the issue (maybe just reference this whole PR)
There was a problem hiding this comment.
@asraa I am not able to get where is getMethodValue() defined. Can you please help out.
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
@asraa Ma'am, Can you please tell me Why we have made such changes to code base, std::string(request->headers().ContentLength()->value().getStringView())) I mean these std::string functions, From where can someone know which std::string function to change. I am very sorry to ask this dumb question, but I am very new to Envoy and OpenSource. |
|
Hi @yashwant121, you can find the definition for envoy/include/envoy/http/header_map.h Line 370 in 5faff2a The reason the change was made was to add a default empty header when someone was accessing the value, rather than potentially causing a potential null dereference when calling the former. The change in the issue for std::string replacement was just to remove the std::string cast around |
3 participants
Risk Level: Low (test only)
Testing: tests pass
Docs Changes: n/a
Release Notes: n/a