integration tests - 2nd round #32
Conversation
nahratzah
left a comment
nahratzah
left a comment
There was a problem hiding this comment.
I haven't properly reviewed the other two tests, but left some comments in the first test that hopefully help you. <3
| }; | ||
| } | ||
|
|
||
| void aggregateClusterConfigModifier(const circuitBreakerThresholds& circuit_breaker_thresholds, |
There was a problem hiding this comment.
This function needs a better name, that describes what it does. <3
| } | ||
|
|
||
| TEST_P(AggregateIntegrationTest, CircuitBreakerTestMaxConnections) { | ||
| circuitBreakerThresholds circuitBreakerThresholds{.max_connections = 1}; |
There was a problem hiding this comment.
I usually recommend against having a variable have the same name as a type, because it tends to become confusing as more code is added/changed/removed over time. (Something I learned the hard way.) :)
| const int FirstUpstreamIndex = 2; | ||
| const int SecondUpstreamIndex = 3; | ||
|
|
||
| struct thresholds { |
There was a problem hiding this comment.
Think the envoy coding standard is to put capitals on all types.
| struct thresholds { | |
| struct Thresholds { |
| // after sending the response | ||
| // aggregate_cluster |
There was a problem hiding this comment.
Would mention that we expect the circuit breaker to be unchanged, because the one from aggregate cluster is unused.
| test_server_->waitForGaugeEq("cluster.cluster_1.circuit_breakers.default.cx_open", 0); | ||
| test_server_->waitForGaugeGe("cluster.cluster_1.circuit_breakers.default.remaining_cx", 1); | ||
|
|
||
| codec_client_ = makeHttpConnection(lookupPort("http")); |
There was a problem hiding this comment.
Doesn't the codec_client_ come with a connection object already installed?
There was a problem hiding this comment.
I don't think so , as deleting it make test fail with : Caught Segmentation fault, suspect faulting address 0x88 which indicates trying to access a null pointer . I think codec_client_ is not initialised.
| waitForNextUpstreamRequest(FirstUpstreamIndex); | ||
|
|
||
| // after creating a connection and sending a request to cluster_1 via aggregate cluster | ||
| // aggregate_cluster |
There was a problem hiding this comment.
It's a good idea to mention why you expect this to be the case.
| // aggregate_cluster | |
| // aggregate_cluster: we expect the circuit breakers to not have changed, because they're not used |
| test_server_->waitForGaugeEq("cluster.aggregate_cluster.circuit_breakers.default.cx_open", 0); | ||
| test_server_->waitForGaugeGe("cluster.aggregate_cluster.circuit_breakers.default.remaining_cx", | ||
| 1); | ||
| // cluster_1 |
There was a problem hiding this comment.
It's a good idea to mention why you expect this to be the case.
| // cluster_1 | |
| // cluster_1: we expect the circuit breakers to respond to the first connection |
| test_server_->waitForCounterGe("cluster.aggregate_cluster.upstream_cx_overflow", 0); | ||
| test_server_->waitForCounterGe("cluster.cluster_1.upstream_cx_overflow", 1); | ||
|
|
||
| // send a 3rd request directly to cluster_1 |
There was a problem hiding this comment.
Would explain why:
| // send a 3rd request directly to cluster_1 | |
| // send a 3rd request directly to cluster_1, to show that the circuit breakers on cluster_1 are used by both the cluster_1 and aggregate_cluster. |
2 participants
Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]