Skip to content

test: deflaking a test, improving debugability #3829

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mattklein123 merged 3 commits into from
Jul 10, 2018
Merged

test: deflaking a test, improving debugability #3829

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
02639b5
test: deflaking a test, improving debugability
alyssawilk Jul 10, 2018
29ce8f1
unit test and fix_format
alyssawilk Jul 10, 2018
fe102b7
Backing logging changes out, and putting them in their own PR
alyssawilk Jul 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions test/integration/fake_upstream.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ void FakeStream::waitForData(Event::Dispatcher& client_dispatcher, uint64_t body
}

void FakeStream::waitForEndStream(Event::Dispatcher& client_dispatcher) {
ENVOY_LOG(trace, "FakeStream waiting for end stream");
Thread::LockGuard lock(lock_);
while (!end_stream_) {
decoder_event_.waitFor(lock_, std::chrono::milliseconds(5));
Expand All @@ -127,6 +128,7 @@ void FakeStream::waitForEndStream(Event::Dispatcher& client_dispatcher) {
client_dispatcher.run(Event::Dispatcher::RunType::NonBlock);
}
}
ENVOY_LOG(trace, "FakeStream done waiting for end stream");
}

void FakeStream::waitForReset() {
Expand Down Expand Up @@ -185,6 +187,7 @@ Http::StreamDecoder& FakeHttpConnection::newStream(Http::StreamEncoder& encoder)
}

void FakeConnectionBase::waitForDisconnect(bool ignore_spurious_events) {
ENVOY_LOG(trace, "FakeConnectionBase waiting for disconnect");
Thread::LockGuard lock(lock_);
while (shared_connection_.connected()) {
connection_event_.wait(lock_); // Safe since CondVar::wait won't throw.
Expand All @@ -199,6 +202,7 @@ void FakeConnectionBase::waitForDisconnect(bool ignore_spurious_events) {
}

ASSERT(!shared_connection_.connected());
ENVOY_LOG(trace, "FakeConnectionBase done waiting for disconnect");
}

void FakeConnectionBase::waitForHalfClose(bool ignore_spurious_events) {
Expand Down
4 changes: 2 additions & 2 deletions test/integration/fake_upstream.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ class QueuedConnectionWrapper : public LinkedObject<QueuedConnectionWrapper> {
/**
* Base class for both fake raw connections and fake HTTP connections.
*/
class FakeConnectionBase {
class FakeConnectionBase : public Logger::Loggable<Logger::Id::testing> {
public:
virtual ~FakeConnectionBase() {
ASSERT(initialized_);
Expand Down Expand Up @@ -341,7 +341,7 @@ typedef std::unique_ptr<FakeHttpConnection> FakeHttpConnectionPtr;
/**
* Fake raw connection for integration testing.
*/
class FakeRawConnection : Logger::Loggable<Logger::Id::testing>, public FakeConnectionBase {
class FakeRawConnection : public FakeConnectionBase {
public:
FakeRawConnection(SharedConnectionWrapper& shared_connection)
: FakeConnectionBase(shared_connection) {}
Expand Down
8 changes: 0 additions & 8 deletions test/integration/hds_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,14 +66,6 @@ class HdsIntegrationTest : public HttpIntegrationTest,
test_server_->waitForCounterGe("hds_delegate.requests", ++hds_requests_);
}

void cleanupUpstreamConnection() {
codec_client_->close();
if (fake_upstream_connection_ != nullptr) {
fake_upstream_connection_->close();
fake_upstream_connection_->waitForDisconnect();
}
}

void cleanupHdsConnection() {
if (fake_hds_connection_ != nullptr) {
fake_hds_connection_->close();
Expand Down
2 changes: 1 addition & 1 deletion test/integration/http2_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,11 +368,11 @@ void Http2IntegrationTest::simultaneousRequest(int32_t request1_bytes, int32_t r
EXPECT_EQ(request2_bytes, response1->body().size());

// Cleanup both downstream and upstream
codec_client_->close();
fake_upstream_connection1->close();
fake_upstream_connection1->waitForDisconnect();
fake_upstream_connection2->close();
fake_upstream_connection2->waitForDisconnect();
codec_client_->close();
Copy link
Member

@htuch htuch Jul 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment explaining how the ordering here matters/helps?

Copy link
Contributor Author

@alyssawilk alyssawilk Jul 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one doesn't actually matter. I added it in case future code copy-pasted and it did matter.

}

TEST_P(Http2IntegrationTest, SimultaneousRequest) { simultaneousRequest(1024, 512); }
Expand Down
10 changes: 7 additions & 3 deletions test/integration/http_integration.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,13 +214,17 @@ IntegrationStreamDecoderPtr HttpIntegrationTest::sendRequestAndWaitForResponse(
}

void HttpIntegrationTest::cleanupUpstreamAndDownstream() {
if (codec_client_) {
codec_client_->close();
}
// Close the upstream connection first. If there's an outstanding request,
// closing the client may result in a FIN being sent upstream, and FakeConnectionBase::close
// will interpret that as an unexpected disconnect. The codec client is not
// subject to the same failure mode.
if (fake_upstream_connection_) {
fake_upstream_connection_->close();
fake_upstream_connection_->waitForDisconnect();
}
if (codec_client_) {
codec_client_->close();
}
}

void HttpIntegrationTest::waitForNextUpstreamRequest(uint64_t upstream_index) {
Expand Down
8 changes: 2 additions & 6 deletions test/integration/integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,25 +277,21 @@ TEST_P(IntegrationTest, TestBind) {
initialize();

codec_client_ = makeHttpConnection(lookupPort("http"));
// Request 1.

auto response =
codec_client_->makeRequestWithBody(Http::TestHeaderMapImpl{{":method", "GET"},
{":path", "/test/long/url"},
{":scheme", "http"},
{":authority", "host"}},
1024);

fake_upstream_connection_ = fake_upstreams_[0]->waitForHttpConnection(*dispatcher_);
std::string address =
fake_upstream_connection_->connection().remoteAddress()->ip()->addressAsString();
EXPECT_EQ(address, address_string);
upstream_request_ = fake_upstream_connection_->waitForNewStream(*dispatcher_);
upstream_request_->waitForEndStream(*dispatcher_);
// Cleanup both downstream and upstream
codec_client_->close();
fake_upstream_connection_->close();
fake_upstream_connection_->waitForDisconnect();

cleanupUpstreamAndDownstream();
}

TEST_P(IntegrationTest, TestFailedBind) {
Expand Down
14 changes: 3 additions & 11 deletions test/integration/load_stats_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,14 +277,6 @@ class LoadStatsIntegrationTest : public HttpIntegrationTest,
return locality_stats;
}

void cleanupUpstreamConnection() {
codec_client_->close();
if (fake_upstream_connection_ != nullptr) {
fake_upstream_connection_->close();
fake_upstream_connection_->waitForDisconnect();
}
}

void cleanupLoadStatsConnection() {
if (fake_loadstats_connection_ != nullptr) {
fake_loadstats_connection_->close();
Expand All @@ -295,7 +287,7 @@ class LoadStatsIntegrationTest : public HttpIntegrationTest,
void sendAndReceiveUpstream(uint32_t endpoint_index, uint32_t response_code = 200) {
initiateClientConnection();
waitForUpstreamResponse(endpoint_index, response_code);
cleanupUpstreamConnection();
cleanupUpstreamAndDownstream();
}

static constexpr uint32_t upstream_endpoints_ = 5;
Expand Down Expand Up @@ -515,7 +507,7 @@ TEST_P(LoadStatsIntegrationTest, InProgress) {
waitForLoadStatsRequest({localityStats("winter", 0, 0, 1)});

waitForUpstreamResponse(0, 503);
cleanupUpstreamConnection();
cleanupUpstreamAndDownstream();

EXPECT_EQ(1, test_server_->counter("load_reporter.requests")->value());
EXPECT_LE(2, test_server_->counter("load_reporter.responses")->value());
Expand Down Expand Up @@ -544,7 +536,7 @@ TEST_P(LoadStatsIntegrationTest, Dropped) {
response_->waitForEndStream();
ASSERT_TRUE(response_->complete());
EXPECT_STREQ("503", response_->headers().Status()->value().c_str());
cleanupUpstreamConnection();
cleanupUpstreamAndDownstream();

waitForLoadStatsRequest({}, 1);

Expand Down
6 changes: 1 addition & 5 deletions test/integration/ratelimit_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,15 +130,11 @@ class RatelimitIntegrationTest : public HttpIntegrationTest,
}

void cleanup() {
codec_client_->close();
if (fake_ratelimit_connection_ != nullptr) {
fake_ratelimit_connection_->close();
fake_ratelimit_connection_->waitForDisconnect();
}
if (fake_upstream_connection_ != nullptr) {
fake_upstream_connection_->close();
fake_upstream_connection_->waitForDisconnect();
}
cleanupUpstreamAndDownstream();
}

FakeHttpConnectionPtr fake_ratelimit_connection_;
Expand Down