Conversation
Taking one for the team here. Signed-off-by: Matt Klein <mklein@lyft.com>
test/extensions/transport_sockets/tls/integration/ssl_integration_test.cc
Show resolved
Hide resolved
| EXPECT_EQ(trace.socket_buffered_trace().events(0).read().data().as_string(), "POST"); | ||
| EXPECT_EQ(trace.socket_buffered_trace().events(1).write().data().as_string(), "HTTP/"); | ||
| EXPECT_EQ(trace.socket_buffered_trace().events(0).write().data().as_string(), "POST"); | ||
| EXPECT_EQ(trace.socket_buffered_trace().events(1).read().data().as_string(), "HTTP/"); |
There was a problem hiding this comment.
It seems like you modified a previous test to do something different. TBH I'm not fully understanding the semantic difference. But I do wonder whether it might have made sense to keep the old function and add a new one. Was the old functionality of the RequestWithJsonBodyAsString test not needed?
There was a problem hiding this comment.
The difference is the tap filter is installed on the upstream connection vs. the downstream. This test was primarily testing a different output type, but now we cover two different things. I can bring back the old test but I'm not sure it adds much extra value.
Signed-off-by: Matt Klein <mklein@lyft.com>
|
@jmarantz updated. LMK if this reads better. |
jmarantz
left a comment
There was a problem hiding this comment.
Thanks -- I'll take your word that the old test had no value :)
Taking one for the team here.
Risk Level: Low
Testing: New tests
Docs Changes: N/A
Release Notes: N/A