Skip to content

Moving more tests over to the v2 API #2204

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
alyssawilk merged 1 commit into from
Dec 13, 2017
Merged

Moving more tests over to the v2 API #2204

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
50 changes: 46 additions & 4 deletions test/integration/cors_filter_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,52 @@ class CorsFilterIntegrationTest : public HttpIntegrationTest,
CorsFilterIntegrationTest() : HttpIntegrationTest(Http::CodecClient::Type::HTTP1, GetParam()) {}

void initialize() override {
initialized_ = true;
fake_upstreams_.emplace_back(new FakeUpstream(0, FakeHttpConnection::Type::HTTP1, version_));
registerPort("upstream_0", fake_upstreams_.back()->localAddress()->ip()->port());
createTestServer("test/config/integration/server_cors_filter.json", {"http"});
config_helper_.addFilter("name: envoy.cors");
config_helper_.addConfigModifier(
[&](envoy::api::v2::filter::network::HttpConnectionManager& hcm) -> void {
auto* route_config = hcm.mutable_route_config();
auto* virtual_host = route_config->mutable_virtual_hosts(0);
{
auto* cors = virtual_host->mutable_cors();
cors->add_allow_origin("*");
cors->set_allow_headers("content-type,x-grpc-web");
cors->set_allow_methods("GET,POST");
}

{
auto* route = virtual_host->mutable_routes(0);
route->mutable_match()->set_prefix("/cors-vhost-config");
}

{
auto* route = virtual_host->add_routes();
route->mutable_match()->set_prefix("/no-cors");
route->mutable_route()->set_cluster("cluster_0");
route->mutable_route()->mutable_cors()->mutable_enabled()->set_value(false);
}

{
auto* route = virtual_host->add_routes();
route->mutable_match()->set_prefix("/cors-route-config");
route->mutable_route()->set_cluster("cluster_0");
auto* cors = route->mutable_route()->mutable_cors();
cors->add_allow_origin("test-origin-1");
cors->add_allow_origin("test-host-2");
cors->set_allow_headers("content-type");
cors->set_allow_methods("POST");
cors->set_expose_headers("content-type");
cors->set_max_age("100");
}
{
auto* route = virtual_host->add_routes();
route->mutable_match()->set_prefix("/cors-credentials-allowed");
route->mutable_route()->set_cluster("cluster_0");
auto* cors = route->mutable_route()->mutable_cors();
cors->add_allow_origin("test-origin-1");
cors->mutable_allow_credentials()->set_value(true);
}
});
HttpIntegrationTest::initialize();
}

protected:
Expand Down
14 changes: 11 additions & 3 deletions test/integration/grpc_json_transcoder_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,17 @@ class GrpcJsonTranscoderIntegrationTest
* Global initializer for all integration tests.
*/
void SetUp() override {
fake_upstreams_.emplace_back(new FakeUpstream(0, FakeHttpConnection::Type::HTTP2, version_));
registerPort("upstream_0", fake_upstreams_.back()->localAddress()->ip()->port());
createTestServer("test/config/integration/server_grpc_json_transcoder.json", {"http"});
setUpstreamProtocol(FakeHttpConnection::Type::HTTP2);
const std::string filter =
R"EOF(
name: envoy.grpc_json_transcoder
config:
proto_descriptor : "{}"
services : "bookstore.Bookstore"
)EOF";
config_helper_.addFilter(
fmt::format(filter, TestEnvironment::runfilesPath("/test/proto/bookstore.descriptor")));
HttpIntegrationTest::initialize();
}

/**
Expand Down
25 changes: 21 additions & 4 deletions test/integration/integration_admin_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ INSTANTIATE_TEST_CASE_P(IpVersions, IntegrationAdminTest,
testing::ValuesIn(TestEnvironment::getIpVersionsForTest()));

TEST_P(IntegrationAdminTest, HealthCheck) {
initialize();

BufferingStreamDecoderPtr response = IntegrationUtil::makeSingleRequest(
lookupPort("http"), "GET", "/healthcheck", "", downstreamProtocol(), version_);
EXPECT_TRUE(response->complete());
Expand All @@ -40,14 +42,21 @@ TEST_P(IntegrationAdminTest, HealthCheck) {
downstreamProtocol(), version_);
EXPECT_TRUE(response->complete());
EXPECT_STREQ("200", response->headers().Status()->value().c_str());
}

response = IntegrationUtil::makeSingleRequest(lookupPort("http_buffer"), "GET", "/healthcheck",
"", downstreamProtocol(), version_);
TEST_P(IntegrationAdminTest, HealthCheckWithBufferFilter) {
config_helper_.addFilter(ConfigHelper::DEFAULT_BUFFER_FILTER);
initialize();

BufferingStreamDecoderPtr response = IntegrationUtil::makeSingleRequest(
lookupPort("http"), "GET", "/healthcheck", "", downstreamProtocol(), version_);
EXPECT_TRUE(response->complete());
EXPECT_STREQ("200", response->headers().Status()->value().c_str());
}

TEST_P(IntegrationAdminTest, AdminLogging) {
initialize();

BufferingStreamDecoderPtr response = IntegrationUtil::makeSingleRequest(
lookupPort("admin"), "GET", "/logging", "", downstreamProtocol(), version_);
EXPECT_TRUE(response->complete());
Expand Down Expand Up @@ -92,6 +101,8 @@ TEST_P(IntegrationAdminTest, AdminLogging) {
}

TEST_P(IntegrationAdminTest, Admin) {
initialize();

BufferingStreamDecoderPtr response = IntegrationUtil::makeSingleRequest(
lookupPort("admin"), "GET", "/", "", downstreamProtocol(), version_);
EXPECT_TRUE(response->complete());
Expand Down Expand Up @@ -136,13 +147,13 @@ TEST_P(IntegrationAdminTest, Admin) {
testing::HasSubstr("# TYPE envoy_cluster_upstream_cx_active gauge\n"));
EXPECT_THAT(
response->body(),
testing::HasSubstr("envoy_cluster_upstream_cx_active{envoy_cluster_name=\"cds\"} 0\n"));
testing::HasSubstr("envoy_cluster_upstream_cx_active{envoy_cluster_name=\"cluster_0\"} 0\n"));
response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", "/clusters", "",
downstreamProtocol(), version_);
EXPECT_TRUE(response->complete());
EXPECT_STREQ("200", response->headers().Status()->value().c_str());
EXPECT_THAT(response->body(), testing::HasSubstr("added_via_api"));
EXPECT_THAT(response->body(), testing::HasSubstr("version_info::\n"));
EXPECT_THAT(response->body(), testing::HasSubstr("version_info::static\n"));
Copy link
Copy Markdown
Contributor Author

@alyssawilk alyssawilk Dec 13, 2017

Choose a reason for hiding this comment

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

This is the one functional change I'm unsure about. Hadn't tracked down the cause.

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 Dec 13, 2017

Choose a reason for hiding this comment

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

I'm guessing it's because server.json has CDS setup but there is no version info yet. This raises a point that for better or worse server.json has a bunch of random stuff in it mainly just to make sure that basic config elements don't break even if there is no usable upstream CDS server, stats backend, etc. We should try to make sure that we don't lose this "coverage."

Copy link
Copy Markdown
Contributor Author

@alyssawilk alyssawilk Dec 13, 2017

Choose a reason for hiding this comment

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

Sorry, I'm not sure I'm understanding the ask here.
Are you asking that the utility class preserve standard things like health checking + admin config from server.json rather than the relevant tests adding them, or something else?

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 Dec 13, 2017

Choose a reason for hiding this comment

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

I'm not really asking for anything. I was just making a comment that there is some test coverage that we had gained from the server.json file that we may lose with the simplistic basic config template and targeted additions we are now using for v2 tests (as seen by this delta). It would be worth an audit of the delta to see if we are losing anything and want to bring any of it back in a targeted "sanity test" of random features. We can do this in follow ups...

Copy link
Copy Markdown
Contributor Author

@alyssawilk alyssawilk Dec 13, 2017

Choose a reason for hiding this comment

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

Ahhh, so we're missing coverage of config loading if those features aren't explicitly tested. Sure I'll take a look at what's missing once I get the rest of the tests moved over.


response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", "/cpuprofiler", "",
downstreamProtocol(), version_);
Expand Down Expand Up @@ -185,6 +196,12 @@ TEST_P(IntegrationAdminTest, Admin) {
#ifdef TCMALLOC

TEST_P(IntegrationAdminTest, AdminCpuProfilerStart) {
config_helper_.addConfigModifier([&](envoy::api::v2::Bootstrap& bootstrap) -> void {
auto* admin = bootstrap.mutable_admin();
admin->set_profile_path(TestEnvironment::temporaryPath("/envoy.prof"));
});

initialize();
BufferingStreamDecoderPtr response = IntegrationUtil::makeSingleRequest(
lookupPort("admin"), "GET", "/cpuprofiler?enable=y", "", downstreamProtocol(), version_);
EXPECT_TRUE(response->complete());
Expand Down
14 changes: 4 additions & 10 deletions test/integration/integration_admin_test.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,10 @@ class IntegrationAdminTest : public HttpIntegrationTest,
public testing::TestWithParam<Network::Address::IpVersion> {
public:
IntegrationAdminTest() : HttpIntegrationTest(Http::CodecClient::Type::HTTP1, GetParam()) {}
/**
* Initializer for an individual test.
*/
void SetUp() override {
fake_upstreams_.emplace_back(new FakeUpstream(0, FakeHttpConnection::Type::HTTP1, version_));
registerPort("upstream_0", fake_upstreams_.back()->localAddress()->ip()->port());
fake_upstreams_.emplace_back(new FakeUpstream(0, FakeHttpConnection::Type::HTTP1, version_));
registerPort("upstream_1", fake_upstreams_.back()->localAddress()->ip()->port());
createTestServer("test/config/integration/server.json",
{"http", "http_buffer", "tcp_proxy", "rds"});

void initialize() override {
config_helper_.addFilter(ConfigHelper::DEFAULT_HEALTH_CHECK_FILTER);
HttpIntegrationTest::initialize();
}

/**
Expand Down
13 changes: 6 additions & 7 deletions test/integration/proxy_proto_integration_test.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,12 @@ namespace Envoy {
class ProxyProtoIntegrationTest : public HttpIntegrationTest,
public testing::TestWithParam<Network::Address::IpVersion> {
public:
ProxyProtoIntegrationTest() : HttpIntegrationTest(Http::CodecClient::Type::HTTP1, GetParam()) {}

void initialize() override {
initialized_ = true;
fake_upstreams_.emplace_back(new FakeUpstream(0, FakeHttpConnection::Type::HTTP1, version_));
registerPort("upstream_0", fake_upstreams_.back()->localAddress()->ip()->port());
createTestServer("test/config/integration/server_proxy_proto.json", {"http"});
ProxyProtoIntegrationTest() : HttpIntegrationTest(Http::CodecClient::Type::HTTP1, GetParam()) {
config_helper_.addConfigModifier([&](envoy::api::v2::Bootstrap& bootstrap) -> void {
auto* listener = bootstrap.mutable_static_resources()->mutable_listeners(0);
auto* filter_chain = listener->mutable_filter_chains(0);
filter_chain->mutable_use_proxy_proto()->set_value(true);
});
}
};
} // namespace Envoy
8 changes: 4 additions & 4 deletions test/integration/xds_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,11 @@ namespace {
class XdsIntegrationTest : public HttpIntegrationTest,
public testing::TestWithParam<Network::Address::IpVersion> {
public:
XdsIntegrationTest() : HttpIntegrationTest(Http::CodecClient::Type::HTTP2, GetParam()) {}
XdsIntegrationTest() : HttpIntegrationTest(Http::CodecClient::Type::HTTP2, GetParam()) {
setUpstreamProtocol(FakeHttpConnection::Type::HTTP2);
}

void initialize() override {
initialized_ = true;
fake_upstreams_.emplace_back(new FakeUpstream(0, FakeHttpConnection::Type::HTTP2, version_));
void createEnvoy() override {
registerPort("upstream_0", fake_upstreams_.back()->localAddress()->ip()->port());
createApiTestServer(
{
Expand Down
11 changes: 11 additions & 0 deletions test/test_common/utility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,17 @@ class TestHeaderMapImpl : public HeaderMapImpl {
TestHeaderMapImpl(const std::initializer_list<std::pair<std::string, std::string>>& values);
TestHeaderMapImpl(const HeaderMap& rhs);

friend std::ostream& operator<<(std::ostream& os, const TestHeaderMapImpl& p) {
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 Dec 13, 2017

Choose a reason for hiding this comment

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

Wherever you found this useful, I think if you included printers.h it would "just work" without this change. I've never found a great way of dealing with the existence of printers.h. I almost would prefer to auto include it in all tests. Worth exploring?

Copy link
Copy Markdown
Contributor Author

@alyssawilk alyssawilk Dec 13, 2017

Choose a reason for hiding this comment

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

Oh interesting. I was wondering what was up with printers.h

Unfortunately that fix doesn't seem to work for my use case. Am I doing something wrong?
Removing my ostream and adding
#include "test/test_common/printers.h"
to the top of cors_filter_integration_test.cc and tweaking the test to fail I get the ugly version again

test/integration/cors_filter_integration_test.cc:90: Failure

Expected equality of these values:
expected_response_headers
Which is: 512-byte object <80-3A AC-01 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 D0-43 83-04 00-00 00-00 10-4A 83-04 00-00 00-00 10-40 83-04 00-00 00-00 00-00 00-00 00-00 00-00 90-42 83-04 00-00 00-00 ... 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 C0-43 83-04 00-00 00-00 C0-48 83-04 00-00 00-00>
response_headers
Which is: 512-byte object <80-3A AC-01 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 10-AF 72-04 00-00 00-00 50-CB 77-04 00-00 00-00 10-CA 77-04 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 ... 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-AF 72-04 00-00 00-00 80-D6 77-04 00-00 00-00>

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 Dec 13, 2017

Choose a reason for hiding this comment

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

Hmm. TBH I have no idea off the top of my head. I would think that printers.h void PrintTo(const HeaderMap& headers, std::ostream* os); would cover this case, but maybe it's not selecting the base class version appropriately? This is not a big deal. I actually think we should get rid of printers.h/printers.cc and just put the printers next to the class definitions, even if they are in public code (and just add some basic tests). WDYT? Open up an issue to track that?

Copy link
Copy Markdown
Contributor Author

@alyssawilk alyssawilk Dec 13, 2017

Choose a reason for hiding this comment

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

#2205

Please add any details you think would be helpful!

p.iterate(
[](const HeaderEntry& header, void* context) -> HeaderMap::Iterate {
std::ostream* local_os = static_cast<std::ostream*>(context);
*local_os << header.key().c_str() << " " << header.value().c_str() << std::endl;
return HeaderMap::Iterate::Continue;
},
&os);
return os;
}

using HeaderMapImpl::addCopy;
void addCopy(const std::string& key, const std::string& value);
std::string get_(const std::string& key);
Expand Down