Skip to content

Commit

Permalink
test: changing waitForDisconnect to only wait for the default timeout (…
Browse files Browse the repository at this point in the history 
…#11428)

This way if waitForDisconect fails one test case should fail rather than the entire test hanging.
Also tagged it with ABSL_MUST_USE_RESULT so tests which wait for a disconnect before doing work would correctly fast-fail.

Signed-off-by: Alyssa Wilk <[email protected]>
  • Loading branch information
@alyssawilk
alyssawilk authored Jun 4, 2020
1 parent 09732b1 commit 43ab77f
Show file tree
Hide file tree
Showing 13 changed files with 70 additions and 66 deletions.
8 changes: 4 additions & 4 deletions test/extensions/clusters/aggregate/cluster_integration_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ TEST_P(AggregateIntegrationTest, ClusterUpDownUp) {
EXPECT_EQ("503", response->headers().getStatusValue());

cleanupUpstreamAndDownstream();
codec_client_->waitForDisconnect();
ASSERT_TRUE(codec_client_->waitForDisconnect());

// Tell Envoy that cluster_1 is back.
EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Cluster, "42", {}, {}, {}));
Expand All @@ -218,7 +218,7 @@ TEST_P(AggregateIntegrationTest, TwoClusters) {
testRouterHeaderOnlyRequestAndResponse(nullptr, FirstUpstreamIndex, "/aggregatecluster");

cleanupUpstreamAndDownstream();
codec_client_->waitForDisconnect();
ASSERT_TRUE(codec_client_->waitForDisconnect());

// Tell Envoy that cluster_2 is here.
EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Cluster, "55", {}, {}, {}));
Expand All @@ -230,7 +230,7 @@ TEST_P(AggregateIntegrationTest, TwoClusters) {
// A request for aggregate cluster should be fine.
testRouterHeaderOnlyRequestAndResponse(nullptr, FirstUpstreamIndex, "/aggregatecluster");
cleanupUpstreamAndDownstream();
codec_client_->waitForDisconnect();
ASSERT_TRUE(codec_client_->waitForDisconnect());

// Tell Envoy that cluster_1 is gone.
EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Cluster, "42", {}, {}, {}));
Expand All @@ -242,7 +242,7 @@ TEST_P(AggregateIntegrationTest, TwoClusters) {

testRouterHeaderOnlyRequestAndResponse(nullptr, SecondUpstreamIndex, "/aggregatecluster");
cleanupUpstreamAndDownstream();
codec_client_->waitForDisconnect();
ASSERT_TRUE(codec_client_->waitForDisconnect());

// Tell Envoy that cluster_1 is back.
EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Cluster, "42", {}, {}, {}));
Expand Down
14 changes: 7 additions & 7 deletions test/integration/cds_integration_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ TEST_P(CdsIntegrationTest, CdsClusterUpDownUp) {
EXPECT_EQ("503", response->headers().getStatusValue());

cleanupUpstreamAndDownstream();
codec_client_->waitForDisconnect();
ASSERT_TRUE(codec_client_->waitForDisconnect());

// Tell Envoy that cluster_1 is back.
EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Cluster, "42", {}, {}, {}));
Expand All @@ -186,7 +186,7 @@ TEST_P(CdsIntegrationTest, TwoClusters) {
testRouterHeaderOnlyRequestAndResponse(nullptr, UpstreamIndex1, "/cluster1");

cleanupUpstreamAndDownstream();
codec_client_->waitForDisconnect();
ASSERT_TRUE(codec_client_->waitForDisconnect());

// Tell Envoy that cluster_2 is here.
EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Cluster, "55", {}, {}, {}));
Expand All @@ -198,7 +198,7 @@ TEST_P(CdsIntegrationTest, TwoClusters) {
// A request for cluster_2 should be fine.
testRouterHeaderOnlyRequestAndResponse(nullptr, UpstreamIndex2, "/cluster2");
cleanupUpstreamAndDownstream();
codec_client_->waitForDisconnect();
ASSERT_TRUE(codec_client_->waitForDisconnect());

// Tell Envoy that cluster_1 is gone.
EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Cluster, "42", {}, {}, {}));
Expand All @@ -211,7 +211,7 @@ TEST_P(CdsIntegrationTest, TwoClusters) {
// Even with cluster_1 gone, a request for cluster_2 should be fine.
testRouterHeaderOnlyRequestAndResponse(nullptr, UpstreamIndex2, "/cluster2");
cleanupUpstreamAndDownstream();
codec_client_->waitForDisconnect();
ASSERT_TRUE(codec_client_->waitForDisconnect());

// Tell Envoy that cluster_1 is back.
EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Cluster, "42", {}, {}, {}));
Expand All @@ -236,7 +236,7 @@ TEST_P(CdsIntegrationTest, VersionsRememberedAfterReconnect) {
// Calls our initialize(), which includes establishing a listener, route, and cluster.
testRouterHeaderOnlyRequestAndResponse(nullptr, UpstreamIndex1, "/cluster1");
cleanupUpstreamAndDownstream();
codec_client_->waitForDisconnect();
ASSERT_TRUE(codec_client_->waitForDisconnect());

// Close the connection carrying Envoy's xDS gRPC stream...
AssertionResult result = xds_connection_->close();
Expand Down Expand Up @@ -265,11 +265,11 @@ TEST_P(CdsIntegrationTest, VersionsRememberedAfterReconnect) {
// A request for cluster_1 should be fine.
testRouterHeaderOnlyRequestAndResponse(nullptr, UpstreamIndex1, "/cluster1");
cleanupUpstreamAndDownstream();
codec_client_->waitForDisconnect();
ASSERT_TRUE(codec_client_->waitForDisconnect());
// A request for cluster_2 should be fine.
testRouterHeaderOnlyRequestAndResponse(nullptr, UpstreamIndex2, "/cluster2");
cleanupUpstreamAndDownstream();
codec_client_->waitForDisconnect();
ASSERT_TRUE(codec_client_->waitForDisconnect());
}

} // namespace
Expand Down
2 changes: 1 addition & 1 deletion test/integration/http2_integration_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -512,7 +512,7 @@ TEST_P(Http2MetadataIntegrationTest, RequestMetadataReachSizeLimit) {
}

// Verifies client connection will be closed.
codec_client_->waitForDisconnect();
ASSERT_TRUE(codec_client_->waitForDisconnect());
ASSERT_FALSE(response->complete());
}

Expand Down
23 changes: 13 additions & 10 deletions test/integration/http_integration.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,10 @@ IntegrationCodecClient::startRequest(const Http::RequestHeaderMap& headers) {
return {encoder, std::move(response)};
}

bool IntegrationCodecClient::waitForDisconnect(std::chrono::milliseconds time_to_wait) {
AssertionResult IntegrationCodecClient::waitForDisconnect(std::chrono::milliseconds time_to_wait) {
if (disconnected_) {
return AssertionSuccess();
}
Event::TimerPtr wait_timer;
bool wait_timer_triggered = false;
if (time_to_wait.count()) {
Expand All @@ -171,11 +174,11 @@ bool IntegrationCodecClient::waitForDisconnect(std::chrono::milliseconds time_to
}

if (wait_timer_triggered && !disconnected_) {
return false;
return AssertionFailure() << "Timed out waiting for disconnect";
}
EXPECT_TRUE(disconnected_);

return true;
return AssertionSuccess();
}

void IntegrationCodecClient::ConnectionCallbacks::onEvent(Network::ConnectionEvent event) {
Expand Down Expand Up @@ -537,7 +540,7 @@ void HttpIntegrationTest::testRouterUpstreamDisconnectBeforeRequestComplete() {
response->waitForEndStream();

if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) {
codec_client_->waitForDisconnect();
ASSERT_TRUE(codec_client_->waitForDisconnect());
} else {
codec_client_->close();
}
Expand All @@ -564,7 +567,7 @@ void HttpIntegrationTest::testRouterUpstreamDisconnectBeforeResponseComplete(
ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect());

if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) {
codec_client_->waitForDisconnect();
ASSERT_TRUE(codec_client_->waitForDisconnect());
} else {
response->waitForReset();
codec_client_->close();
Expand Down Expand Up @@ -661,7 +664,7 @@ void HttpIntegrationTest::testRouterUpstreamResponseBeforeRequestComplete() {
}

if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) {
codec_client_->waitForDisconnect();
ASSERT_TRUE(codec_client_->waitForDisconnect());
} else {
codec_client_->close();
}
Expand Down Expand Up @@ -992,7 +995,7 @@ void HttpIntegrationTest::testLargeRequestHeaders(uint32_t size, uint32_t count,
auto response = std::move(encoder_decoder.second);

if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) {
codec_client_->waitForDisconnect();
ASSERT_TRUE(codec_client_->waitForDisconnect());
EXPECT_TRUE(response->complete());
EXPECT_EQ("431", response->headers().getStatusValue());
} else {
Expand Down Expand Up @@ -1031,7 +1034,7 @@ void HttpIntegrationTest::testLargeRequestTrailers(uint32_t size, uint32_t max_s

if (size >= max_size) {
if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) {
codec_client_->waitForDisconnect();
ASSERT_TRUE(codec_client_->waitForDisconnect());
EXPECT_TRUE(response->complete());
EXPECT_EQ("431", response->headers().getStatusValue());
} else {
Expand Down Expand Up @@ -1236,7 +1239,7 @@ void HttpIntegrationTest::testMaxStreamDuration() {
test_server_->waitForCounterGe("cluster.cluster_0.upstream_rq_max_duration_reached", 1);

if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) {
codec_client_->waitForDisconnect();
ASSERT_TRUE(codec_client_->waitForDisconnect());
} else {
response->waitForReset();
codec_client_->close();
Expand Down Expand Up @@ -1281,7 +1284,7 @@ void HttpIntegrationTest::testMaxStreamDurationWithRetry(bool invoke_retry_upstr
if (invoke_retry_upstream_disconnect) {
test_server_->waitForCounterGe("cluster.cluster_0.upstream_rq_max_duration_reached", 2);
if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) {
codec_client_->waitForDisconnect();
ASSERT_TRUE(codec_client_->waitForDisconnect());
} else {
response->waitForReset();
codec_client_->close();
Expand Down
3 changes: 2 additions & 1 deletion test/integration/http_integration.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ class IntegrationCodecClient : public Http::CodecClientProd {
void sendMetadata(Http::RequestEncoder& encoder, Http::MetadataMap metadata_map);
std::pair<Http::RequestEncoder&, IntegrationStreamDecoderPtr>
startRequest(const Http::RequestHeaderMap& headers);
bool waitForDisconnect(std::chrono::milliseconds time_to_wait = std::chrono::milliseconds(0));
ABSL_MUST_USE_RESULT AssertionResult
waitForDisconnect(std::chrono::milliseconds time_to_wait = TestUtility::DefaultTimeout);
Network::ClientConnection* connection() const { return connection_.get(); }
Network::ConnectionEvent lastConnectionEvent() const { return last_connection_event_; }
Network::Connection& rawConnection() { return *connection_; }
Expand Down
2 changes: 1 addition & 1 deletion test/integration/idle_timeout_integration_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ class IdleTimeoutIntegrationTest : public HttpProtocolIntegrationTest {
void waitForTimeout(IntegrationStreamDecoder& response, absl::string_view stat_name = "",
absl::string_view stat_prefix = "http.config_test") {
if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) {
codec_client_->waitForDisconnect();
ASSERT_TRUE(codec_client_->waitForDisconnect());
} else {
response.waitForReset();
codec_client_->close();
Expand Down
10 changes: 5 additions & 5 deletions test/integration/integration_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ TEST_P(IntegrationTest, ConnectionClose) {
{":authority", "host"},
{"connection", "close"}});
response->waitForEndStream();
codec_client_->waitForDisconnect();
ASSERT_TRUE(codec_client_->waitForDisconnect());

EXPECT_TRUE(response->complete());
EXPECT_THAT(response->headers(), HttpStatusIs("200"));
Expand Down Expand Up @@ -809,7 +809,7 @@ TEST_P(IntegrationTest, UpstreamProtocolError) {
ASSERT_TRUE(fake_upstream_connection->waitForData(187, &data));
ASSERT_TRUE(fake_upstream_connection->write("bad protocol data!"));
ASSERT_TRUE(fake_upstream_connection->waitForDisconnect());
codec_client_->waitForDisconnect();
ASSERT_TRUE(codec_client_->waitForDisconnect());

EXPECT_TRUE(response->complete());
EXPECT_EQ("503", response->headers().getStatusValue());
Expand Down Expand Up @@ -953,7 +953,7 @@ TEST_P(IntegrationTest, ViaAppendHeaderOnly) {
EXPECT_THAT(upstream_request_->headers(), HeaderValueOf(Headers::get().Via, "foo, bar"));
upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "200"}}, true);
response->waitForEndStream();
codec_client_->waitForDisconnect();
ASSERT_TRUE(codec_client_->waitForDisconnect());
EXPECT_TRUE(response->complete());
EXPECT_THAT(response->headers(), HttpStatusIs("200"));
EXPECT_THAT(response->headers(), HeaderValueOf(Headers::get().Via, "bar"));
Expand Down Expand Up @@ -1134,7 +1134,7 @@ TEST_P(IntegrationTest, ProcessObjectHealthy) {
{":authority", "host"},
{"connection", "close"}});
response->waitForEndStream();
codec_client_->waitForDisconnect();
ASSERT_TRUE(codec_client_->waitForDisconnect());

EXPECT_TRUE(response->complete());
EXPECT_THAT(response->headers(), HttpStatusIs("200"));
Expand All @@ -1155,7 +1155,7 @@ TEST_P(IntegrationTest, ProcessObjectUnealthy) {
{":authority", "host"},
{"connection", "close"}});
response->waitForEndStream();
codec_client_->waitForDisconnect();
ASSERT_TRUE(codec_client_->waitForDisconnect());

EXPECT_TRUE(response->complete());
EXPECT_THAT(response->headers(), HttpStatusIs("500"));
Expand Down
10 changes: 5 additions & 5 deletions test/integration/local_reply_integration_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ TEST_P(LocalReplyIntegrationTest, MapStatusCodeAndFormatToJson) {
response->waitForEndStream();

if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) {
codec_client_->waitForDisconnect();
ASSERT_TRUE(codec_client_->waitForDisconnect());
} else {
codec_client_->close();
}
Expand Down Expand Up @@ -134,7 +134,7 @@ TEST_P(LocalReplyIntegrationTest, MapStatusCodeAndFormatToJsonForFirstMatchingFi
response->waitForEndStream();

if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) {
codec_client_->waitForDisconnect();
ASSERT_TRUE(codec_client_->waitForDisconnect());
} else {
codec_client_->close();
}
Expand Down Expand Up @@ -206,7 +206,7 @@ TEST_P(LocalReplyIntegrationTest, ShouldNotMatchAnyFilter) {
response->waitForEndStream();

if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) {
codec_client_->waitForDisconnect();
ASSERT_TRUE(codec_client_->waitForDisconnect());
} else {
codec_client_->close();
}
Expand Down Expand Up @@ -267,7 +267,7 @@ TEST_P(LocalReplyIntegrationTest, ShouldMapResponseCodeAndMapToDefaultTextRespon
response->waitForEndStream();

if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) {
codec_client_->waitForDisconnect();
ASSERT_TRUE(codec_client_->waitForDisconnect());
} else {
codec_client_->close();
}
Expand Down Expand Up @@ -324,7 +324,7 @@ TEST_P(LocalReplyIntegrationTest, ShouldFormatResponseToCustomString) {
response->waitForEndStream();

if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) {
codec_client_->waitForDisconnect();
ASSERT_TRUE(codec_client_->waitForDisconnect());
} else {
codec_client_->close();
}
Expand Down
2 changes: 1 addition & 1 deletion test/integration/overload_integration_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ TEST_P(OverloadIntegrationTest, DisableKeepaliveWhenOverloaded) {
Http::TestRequestHeaderMapImpl request_headers{
{":method", "GET"}, {":path", "/test/long/url"}, {":scheme", "http"}, {":authority", "host"}};
auto response = sendRequestAndWaitForResponse(request_headers, 1, default_response_headers_, 1);
codec_client_->waitForDisconnect();
ASSERT_TRUE(codec_client_->waitForDisconnect());

EXPECT_TRUE(response->complete());
EXPECT_EQ("200", response->headers().getStatusValue());
Expand Down
Loading

0 comments on commit 43ab77f

Please sign in to comment.