Skip to content

test: Add timeouts to methods that could wait forever in test/integration/fake_upstream.h. #3936

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 12 commits into from
Jul 31, 2018
Merged

test: Add timeouts to methods that could wait forever in test/integration/fake_upstream.h. #3936

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
1ee408c
Add timeouts to methods in fake_upstream.h that could wait forever.
mkbehr Jul 10, 2018
e4dcd85
Fix tests outside of test/integration.
mkbehr Jul 23, 2018
d7cb98e
Use RELEASE_ASSERT in tests that aren't ready to propagate timeout er…
mkbehr Jul 24, 2018
b2012c7
Merge remote-tracking branch 'upstream/master' into fake-upstream-tim…
mkbehr Jul 24, 2018
5d26fca
Bugfixes for fake_upstream timeouts
mkbehr Jul 25, 2018
ce59cf0
Merge remote-tracking branch 'upstream/master' into fake-upstream-tim…
mkbehr Jul 25, 2018
22fe837
Missing VERIFY_ASSERTION in FakeUpstream::waitForRawConnection
mkbehr Jul 26, 2018
20521f4
Merge remote-tracking branch 'upstream/master' into fake-upstream-tim…
mkbehr Jul 26, 2018
d1c829b
Merge branch 'master' of https://github.com/envoyproxy/envoy into fak…
mkbehr Jul 27, 2018
a92ce84
Merge branch 'master' of https://github.com/envoyproxy/envoy into fak…
mkbehr Jul 30, 2018
f2a8bb4
Code review changes
mkbehr Jul 31, 2018
45fe22f
Merge branch 'master' of https://github.com/envoyproxy/envoy into fak…
mkbehr Jul 31, 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
8 changes: 4 additions & 4 deletions test/common/grpc/grpc_client_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,10 +308,10 @@ TEST_P(GrpcClientIntegrationTest, ResetAfterCloseLocal) {
initialize();
auto stream = createStream(empty_metadata_);
stream->grpc_stream_->closeStream();
stream->fake_stream_->waitForEndStream(dispatcher_helper_.dispatcher_);
ASSERT_TRUE(stream->fake_stream_->waitForEndStream(dispatcher_helper_.dispatcher_));
stream->grpc_stream_->resetStream();
dispatcher_helper_.dispatcher_.run(Event::Dispatcher::RunType::NonBlock);
stream->fake_stream_->waitForReset();
ASSERT_TRUE(stream->fake_stream_->waitForReset());
}

// Validate that request cancel() works.
Expand All @@ -323,7 +323,7 @@ TEST_P(GrpcClientIntegrationTest, CancelRequest) {
EXPECT_CALL(*request->child_span_, finishSpan());
request->grpc_request_->cancel();
dispatcher_helper_.dispatcher_.run(Event::Dispatcher::RunType::NonBlock);
request->fake_stream_->waitForReset();
ASSERT_TRUE(request->fake_stream_->waitForReset());
}

// Parameterize the loopback test server socket address and gRPC client type.
Expand Down Expand Up @@ -352,7 +352,7 @@ TEST_P(GrpcSslClientIntegrationTest, BasicSslRequestWithClientCert) {
class GrpcAccessTokenClientIntegrationTest : public GrpcSslClientIntegrationTest {
public:
void expectExtraHeaders(FakeStream& fake_stream) override {
fake_stream.waitForHeadersComplete();
ASSERT(fake_stream.waitForHeadersComplete());

@lizan lizan Jul 23, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This will be compiled out in release builds, which is not your intention, no?

@mkbehr mkbehr Jul 24, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right, yeah. Changed these to use RELEASE_ASSERT. Do you think it's worth adding a macro for this use? (RELEASE_ASSERT on an AssertionResult, using the AssertionResult's message as the RELEASE_ASSERT's message)

@zuercher zuercher Jul 24, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not use ASSERT_TRUE?

@mkbehr mkbehr Jul 25, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ASSERT_TRUE will return from the helper function but keep the test running, so the test will probably crash anyway but in a more confusing way. (And for helpers that return things, like SquashFilterIntegrationTest::sendSquash, it won't compile.)

Http::TestHeaderMapImpl stream_headers(fake_stream.headers());
if (access_token_value_ != "") {
if (access_token_value_2_ == "") {
Expand Down
22 changes: 12 additions & 10 deletions test/common/grpc/grpc_client_integration_test_harness.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ class HelloworldStream : public MockAsyncStreamCallbacks<helloworld::HelloReply>
grpc_stream_->sendMessage(request_msg, end_stream);

helloworld::HelloRequest received_msg;
fake_stream_->waitForGrpcMessage(dispatcher_helper_.dispatcher_, received_msg);
ASSERT(fake_stream_->waitForGrpcMessage(dispatcher_helper_.dispatcher_, received_msg));
EXPECT_THAT(request_msg, ProtoEq(received_msg));
}

Expand Down Expand Up @@ -162,7 +162,7 @@ class HelloworldStream : public MockAsyncStreamCallbacks<helloworld::HelloReply>

void closeStream() {
grpc_stream_->closeStream();
fake_stream_->waitForEndStream(dispatcher_helper_.dispatcher_);
ASSERT(fake_stream_->waitForEndStream(dispatcher_helper_.dispatcher_));
}

DispatcherHelper& dispatcher_helper_;
Expand Down Expand Up @@ -224,8 +224,8 @@ class GrpcClientIntegrationTest : public GrpcClientIntegrationParamTest {

void TearDown() override {
if (fake_connection_) {
fake_connection_->close();
fake_connection_->waitForDisconnect();
ASSERT(fake_connection_->close());
ASSERT(fake_connection_->waitForDisconnect());
fake_connection_.reset();
}
}
Expand Down Expand Up @@ -291,7 +291,7 @@ class GrpcClientIntegrationTest : public GrpcClientIntegrationParamTest {
}

void expectInitialHeaders(FakeStream& fake_stream, const TestMetadata& initial_metadata) {
fake_stream.waitForHeadersComplete();
ASSERT(fake_stream.waitForHeadersComplete());
Http::TestHeaderMapImpl stream_headers(fake_stream.headers());
EXPECT_EQ("POST", stream_headers.get_(":method"));
EXPECT_EQ("/helloworld.Greeter/SayHello", stream_headers.get_(":path"));
Expand Down Expand Up @@ -333,17 +333,18 @@ class GrpcClientIntegrationTest : public GrpcClientIntegrationParamTest {
EXPECT_NE(request->grpc_request_, nullptr);

if (!fake_connection_) {
fake_connection_ = fake_upstream_->waitForHttpConnection(dispatcher_);
ASSERT(fake_upstream_->waitForHttpConnection(dispatcher_, &fake_connection_));
}
fake_streams_.push_back(fake_connection_->waitForNewStream(dispatcher_));
fake_streams_.emplace_back();
ASSERT(fake_connection_->waitForNewStream(dispatcher_, &fake_streams_.back()));
auto& fake_stream = *fake_streams_.back();
request->fake_stream_ = &fake_stream;

expectInitialHeaders(fake_stream, initial_metadata);
expectExtraHeaders(fake_stream);

helloworld::HelloRequest received_msg;
fake_stream.waitForGrpcMessage(dispatcher_, received_msg);
ASSERT(fake_stream.waitForGrpcMessage(dispatcher_, received_msg));
EXPECT_THAT(request_msg, ProtoEq(received_msg));

return request;
Expand All @@ -362,9 +363,10 @@ class GrpcClientIntegrationTest : public GrpcClientIntegrationParamTest {
EXPECT_NE(stream->grpc_stream_, nullptr);

if (!fake_connection_) {
fake_connection_ = fake_upstream_->waitForHttpConnection(dispatcher_);
ASSERT(fake_upstream_->waitForHttpConnection(dispatcher_, &fake_connection_));
}
fake_streams_.push_back(fake_connection_->waitForNewStream(dispatcher_));
fake_streams_.emplace_back();
ASSERT(fake_connection_->waitForNewStream(dispatcher_, &fake_streams_.back()));
auto& fake_stream = *fake_streams_.back();
stream->fake_stream_ = &fake_stream;

Expand Down
1 change: 1 addition & 0 deletions test/extensions/access_loggers/http_grpc/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,5 +46,6 @@ envoy_extension_cc_test(
"//source/extensions/access_loggers/http_grpc:config",
"//test/common/grpc:grpc_client_integration_lib",
"//test/integration:http_integration_lib",
"//test/test_common:utility_lib",
],
)
42 changes: 25 additions & 17 deletions test/extensions/access_loggers/http_grpc/grpc_access_log_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,12 @@

#include "test/common/grpc/grpc_client_integration.h"
#include "test/integration/http_integration.h"
#include "test/test_common/utility.h"

#include "gtest/gtest.h"

using testing::AssertionResult;

namespace Envoy {
namespace {

Expand Down Expand Up @@ -49,17 +52,20 @@ class AccessLogIntegrationTest : public HttpIntegrationTest,
HttpIntegrationTest::initialize();
}

void waitForAccessLogConnection() {
fake_access_log_connection_ = fake_upstreams_[1]->waitForHttpConnection(*dispatcher_);
ABSL_MUST_USE_RESULT
AssertionResult waitForAccessLogConnection() {
return fake_upstreams_[1]->waitForHttpConnection(*dispatcher_, &fake_access_log_connection_);
}

void waitForAccessLogStream() {
access_log_request_ = fake_access_log_connection_->waitForNewStream(*dispatcher_);
ABSL_MUST_USE_RESULT
AssertionResult waitForAccessLogStream() {
return fake_access_log_connection_->waitForNewStream(*dispatcher_, &access_log_request_);
}

void waitForAccessLogRequest(const std::string& expected_request_msg_yaml) {
ABSL_MUST_USE_RESULT
AssertionResult waitForAccessLogRequest(const std::string& expected_request_msg_yaml) {
envoy::service::accesslog::v2::StreamAccessLogsMessage request_msg;
access_log_request_->waitForGrpcMessage(*dispatcher_, request_msg);
VERIFY_ASSERTION(access_log_request_->waitForGrpcMessage(*dispatcher_, request_msg));
EXPECT_STREQ("POST", access_log_request_->headers().Method()->value().c_str());
EXPECT_STREQ("/envoy.service.accesslog.v2.AccessLogService/StreamAccessLogs",
access_log_request_->headers().Path()->value().c_str());
Expand All @@ -78,12 +84,14 @@ class AccessLogIntegrationTest : public HttpIntegrationTest,
log_entry->mutable_common_properties()->clear_time_to_last_downstream_tx_byte();
log_entry->mutable_request()->clear_request_id();
EXPECT_EQ(request_msg.DebugString(), expected_request_msg.DebugString());

return AssertionSuccess();
}

void cleanup() {
if (fake_access_log_connection_ != nullptr) {
fake_access_log_connection_->close();
fake_access_log_connection_->waitForDisconnect();
ASSERT(fake_access_log_connection_->close());
ASSERT(fake_access_log_connection_->waitForDisconnect());
}
}

Expand All @@ -97,9 +105,9 @@ INSTANTIATE_TEST_CASE_P(IpVersionsCientType, AccessLogIntegrationTest,
// Test a basic full access logging flow.
TEST_P(AccessLogIntegrationTest, BasicAccessLogFlow) {
testRouterNotFound();
waitForAccessLogConnection();
waitForAccessLogStream();
waitForAccessLogRequest(fmt::format(R"EOF(
ASSERT_TRUE(waitForAccessLogConnection());
ASSERT_TRUE(waitForAccessLogStream());
ASSERT_TRUE(waitForAccessLogRequest(fmt::format(R"EOF(
identifier:
node:
id: node_name
Expand All @@ -124,13 +132,13 @@ TEST_P(AccessLogIntegrationTest, BasicAccessLogFlow) {
value: 404
response_headers_bytes: 54
)EOF",
VersionInfo::version()));
VersionInfo::version())));

BufferingStreamDecoderPtr response = IntegrationUtil::makeSingleRequest(
lookupPort("http"), "GET", "/notfound", "", downstream_protocol_, version_);
EXPECT_TRUE(response->complete());
EXPECT_STREQ("404", response->headers().Status()->value().c_str());
waitForAccessLogRequest(R"EOF(
ASSERT_TRUE(waitForAccessLogRequest(R"EOF(
http_logs:
log_entry:
common_properties:
Expand All @@ -146,7 +154,7 @@ TEST_P(AccessLogIntegrationTest, BasicAccessLogFlow) {
response_code:
value: 404
response_headers_bytes: 54
)EOF");
)EOF"));

// Send an empty response and end the stream. This should never happen but make sure nothing
// breaks and we make a new stream on a follow up request.
Expand All @@ -168,8 +176,8 @@ TEST_P(AccessLogIntegrationTest, BasicAccessLogFlow) {
downstream_protocol_, version_);
EXPECT_TRUE(response->complete());
EXPECT_STREQ("404", response->headers().Status()->value().c_str());
waitForAccessLogStream();
waitForAccessLogRequest(fmt::format(R"EOF(
ASSERT_TRUE(waitForAccessLogStream());
ASSERT_TRUE(waitForAccessLogRequest(fmt::format(R"EOF(
identifier:
node:
id: node_name
Expand All @@ -194,7 +202,7 @@ TEST_P(AccessLogIntegrationTest, BasicAccessLogFlow) {
value: 404
response_headers_bytes: 54
)EOF",
VersionInfo::version()));
VersionInfo::version())));

cleanup();
}
Expand Down
13 changes: 7 additions & 6 deletions test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,11 @@ class GrpcJsonTranscoderIntegrationTest
response = codec_client_->makeHeaderOnlyRequest(request_headers);
}

fake_upstream_connection_ = fake_upstreams_[0]->waitForHttpConnection(*dispatcher_);
upstream_request_ = fake_upstream_connection_->waitForNewStream(*dispatcher_);
ASSERT_TRUE(
fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, &fake_upstream_connection_));
ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, &upstream_request_));
if (!grpc_request_messages.empty()) {
upstream_request_->waitForEndStream(*dispatcher_);
ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_));

Grpc::Decoder grpc_decoder;
std::vector<Grpc::Frame> frames;
Expand Down Expand Up @@ -114,7 +115,7 @@ class GrpcJsonTranscoderIntegrationTest
}
EXPECT_TRUE(upstream_request_->complete());
} else {
upstream_request_->waitForReset();
ASSERT_TRUE(upstream_request_->waitForReset());
}

response->waitForEndStream();
Expand All @@ -136,8 +137,8 @@ class GrpcJsonTranscoderIntegrationTest
}

codec_client_->close();
fake_upstream_connection_->close();
fake_upstream_connection_->waitForDisconnect();
ASSERT_TRUE(fake_upstream_connection_->close());
ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect());
}
};

Expand Down
14 changes: 7 additions & 7 deletions test/extensions/filters/http/jwt_authn/filter_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,9 +109,9 @@ class RemoteJwksIntegrationTest : public HttpProtocolIntegrationTest {
}

void waitForJwksResponse(const std::string& status, const std::string& jwks_body) {
fake_jwks_connection_ = fake_upstreams_[1]->waitForHttpConnection(*dispatcher_);
jwks_request_ = fake_jwks_connection_->waitForNewStream(*dispatcher_);
jwks_request_->waitForEndStream(*dispatcher_);
ASSERT(fake_upstreams_[1]->waitForHttpConnection(*dispatcher_, &fake_jwks_connection_));
ASSERT(fake_jwks_connection_->waitForNewStream(*dispatcher_, &jwks_request_));
ASSERT(jwks_request_->waitForEndStream(*dispatcher_));

Http::TestHeaderMapImpl response_headers{{":status", status}};
jwks_request_->encodeHeaders(response_headers, false);
Expand All @@ -122,12 +122,12 @@ class RemoteJwksIntegrationTest : public HttpProtocolIntegrationTest {
void cleanup() {
codec_client_->close();
if (fake_jwks_connection_ != nullptr) {
fake_jwks_connection_->close();
fake_jwks_connection_->waitForDisconnect();
ASSERT(fake_jwks_connection_->close());
ASSERT(fake_jwks_connection_->waitForDisconnect());
}
if (fake_upstream_connection_ != nullptr) {
fake_upstream_connection_->close();
fake_upstream_connection_->waitForDisconnect();
ASSERT(fake_upstream_connection_->close());
ASSERT(fake_upstream_connection_->waitForDisconnect());
}
}

Expand Down
20 changes: 10 additions & 10 deletions test/extensions/filters/http/lua/lua_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,12 +72,12 @@ class LuaIntegrationTest : public HttpIntegrationTest,
void cleanup() {
codec_client_->close();
if (fake_lua_connection_ != nullptr) {
fake_lua_connection_->close();
fake_lua_connection_->waitForDisconnect();
ASSERT(fake_lua_connection_->close());
ASSERT(fake_lua_connection_->waitForDisconnect());
}
if (fake_upstream_connection_ != nullptr) {
fake_upstream_connection_->close();
fake_upstream_connection_->waitForDisconnect();
ASSERT(fake_upstream_connection_->close());
ASSERT(fake_upstream_connection_->waitForDisconnect());
}
}

Expand Down Expand Up @@ -234,9 +234,9 @@ name: envoy.lua
{"x-forwarded-for", "10.0.0.1"}};
auto response = codec_client_->makeHeaderOnlyRequest(request_headers);

fake_lua_connection_ = fake_upstreams_[1]->waitForHttpConnection(*dispatcher_);
lua_request_ = fake_lua_connection_->waitForNewStream(*dispatcher_);
lua_request_->waitForEndStream(*dispatcher_);
ASSERT_TRUE(fake_upstreams_[1]->waitForHttpConnection(*dispatcher_, &fake_lua_connection_));
ASSERT_TRUE(fake_lua_connection_->waitForNewStream(*dispatcher_, &lua_request_));
ASSERT_TRUE(lua_request_->waitForEndStream(*dispatcher_));
Http::TestHeaderMapImpl response_headers{{":status", "200"}, {"foo", "bar"}};
lua_request_->encodeHeaders(response_headers, false);
Buffer::OwnedImpl response_data1("good");
Expand Down Expand Up @@ -292,9 +292,9 @@ name: envoy.lua
{"x-forwarded-for", "10.0.0.1"}};
auto response = codec_client_->makeHeaderOnlyRequest(request_headers);

fake_lua_connection_ = fake_upstreams_[1]->waitForHttpConnection(*dispatcher_);
lua_request_ = fake_lua_connection_->waitForNewStream(*dispatcher_);
lua_request_->waitForEndStream(*dispatcher_);
ASSERT_TRUE(fake_upstreams_[1]->waitForHttpConnection(*dispatcher_, &fake_lua_connection_));
ASSERT_TRUE(fake_lua_connection_->waitForNewStream(*dispatcher_, &lua_request_));
ASSERT_TRUE(lua_request_->waitForEndStream(*dispatcher_));
Http::TestHeaderMapImpl response_headers{{":status", "200"}, {"foo", "bar"}};
lua_request_->encodeHeaders(response_headers, true);

Expand Down
11 changes: 6 additions & 5 deletions test/extensions/filters/http/squash/squash_filter_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,19 +22,20 @@ class SquashFilterIntegrationTest : public HttpIntegrationTest,

~SquashFilterIntegrationTest() {
if (fake_squash_connection_) {
fake_squash_connection_->close();
fake_squash_connection_->waitForDisconnect();
ASSERT(fake_squash_connection_->close());
ASSERT(fake_squash_connection_->waitForDisconnect());
}
}

FakeStreamPtr sendSquash(const std::string& status, const std::string& body) {

if (!fake_squash_connection_) {
fake_squash_connection_ = fake_upstreams_[1]->waitForHttpConnection(*dispatcher_);
ASSERT(fake_upstreams_[1]->waitForHttpConnection(*dispatcher_, &fake_squash_connection_));
}

FakeStreamPtr request_stream = fake_squash_connection_->waitForNewStream(*dispatcher_);
request_stream->waitForEndStream(*dispatcher_);
FakeStreamPtr request_stream;
ASSERT(fake_squash_connection_->waitForNewStream(*dispatcher_, &request_stream));
ASSERT(request_stream->waitForEndStream(*dispatcher_));
if (body.empty()) {
request_stream->encodeHeaders(Http::TestHeaderMapImpl{{":status", status}}, true);
} else {
Expand Down
Loading