dfp: adding queueing test#18748
Conversation
Signed-off-by: Alyssa Wilk <[email protected]>
RyanTheOptimist
left a comment
RyanTheOptimist
left a comment
There was a problem hiding this comment.
Looks great! Are there any stats that we might want to also consider checking for this case? (Or maybe some stats we should add?)
| setDownstreamProtocol(Http::CodecType::HTTP2); | ||
| setUpstreamProtocol(Http::CodecType::HTTP2); | ||
|
|
||
| // Ensure we only have one connection upstream, one request active at a time. |
There was a problem hiding this comment.
Thank you for this comment. There is so much boilerplate required to get this config working that I would probably not have figured out the intent.
There was a problem hiding this comment.
yeah a lot of Envoy tests are hard to read especially with heavy mock use so I encourage overcommenting rather than under :-P
| request_encoder_ = &encoder_decoder.first; | ||
| auto response = std::move(encoder_decoder.second); | ||
|
|
||
| // make sure the headers are received. |
| config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { | ||
| auto* static_resources = bootstrap.mutable_static_resources(); | ||
| auto* cluster = static_resources->mutable_clusters(0); | ||
| auto* circuit_breakers = cluster->mutable_circuit_breakers(); |
There was a problem hiding this comment.
nit: there are quite a few uses of auto here where the type is not really obvious to me. I think the style guide recommends against auto in cases like this. (Down on line 475, too)
There was a problem hiding this comment.
yeah I think envoy-internal style is to use auto pretty heavily especially for protos where the types are painfully long. I'm up for giving it a shot as beyond it being the style guide, I agree it can be confusing especially to newcomers
I've simply gotten in the habit of "grep for who has implements this function" =P
| test_server_->waitForCounterEq("http.config_test.downstream_rq_total", 2); | ||
| // Make sure the stream is not received. | ||
| ASSERT_FALSE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_, | ||
| std::chrono::milliseconds(100))); |
There was a problem hiding this comment.
This seems totally reasonable for an integration test, but it's mildly annoying that there is now way to "ensure something will never happen in the future". But I guess that's just not how the world works :)
Signed-off-by: Alyssa Wilk <[email protected]>
2 participants
Risk Level: n/a
Testing: new integration test
Docs Changes: n/a
Release Notes: n/a
Fixes #17611