Skip to content

test: enable parameterized test for HttpProtocolIntegrationTest #41635

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

Closed
wants to merge 6 commits into from
Closed

test: enable parameterized test for HttpProtocolIntegrationTest #41635

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c1861ee
Defined a template with extra params.
cpakulski Oct 9, 2025
8e03974
Template for integration tests with additional parameters.
cpakulski Oct 16, 2025
45d52fd
Chnaged outlier tests to parameterized.
cpakulski Oct 18, 2025
c6fb675
Format.
cpakulski Oct 18, 2025
580c34d
Corrected parameter type.
cpakulski Oct 18, 2025
208a16e
Merge remote-tracking branch 'upstream/main' into http_integr_paramed
cpakulski Oct 21, 2025
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
246 changes: 86 additions & 160 deletions test/integration/circuit_breakers_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,117 +196,45 @@ TEST_P(CircuitBreakersIntegrationTest, CircuitBreakerRuntimeProto) {
#endif
}

class OutlierDetectionIntegrationTest : public HttpProtocolIntegrationTest {
// Parameterized integration test. The parameters in the tuple are:
// - name of the test
// - basic (legacy) outlier detection config snippet
// - outlier detection extension config snippet to be added to the cluster
// - response code to be sent from upstream
// - optional response header value
class OutlierDetectionIntegrationTest
: public HttpProtocolIntegrationTestWithParams<std::tuple<
std::string, absl::string_view, absl::string_view, uint32_t, absl::string_view>> {
Comment on lines +206 to +207
Copy link
Contributor

@ravenblackx ravenblackx Oct 21, 2025

Choose a reason for hiding this comment

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

Please make this a struct rather than a tuple with explanatory comments and const indices.

Or, better for this purpose, don't use parameterized testing for this at all - it's much clearer to just have member variables in the test class, so instead of something like

TEST_P(OutlierDetectionIntegrationTest, TestEverything) {
  auto x = GetParam(1).x;
  auto y = GetParam(1).y;
  auto foo = initializeXY(x, y);
  auto expected = GetParam(1).expected;
  EXPECT_THAT(doStuff(foo), Eq(expected));
}

INSTANTIATE_TEST_SUITE_P(
  TestArgStruct{"1And2ResultsIn3", 1, 2, 3},
  TestArgStruct{"4And5ResultsIn9", 4, 5, 9},
);

you just do

TEST_F(OutlierDetectionIntegrationTest, Test1and2ResultsIn3) {
  auto foo = initializeXY(1, 2);
  EXPECT_THAT(doStuff(foo), Eq(3));
}

TEST_F(OutlierDetectionIntegrationTest, Test4and5ResultsIn9) {
  auto foo = initializeXY(4, 5);
  EXPECT_THAT(doStuff(foo), Eq(9));
}

Even though if there are many tests this may result in more lines (it typically doesn't actually because the fields usually end up one line each, and passing the same values into helper functions is also usually one line), it makes for test code that's dramatically easier to read and understand, and you don't need to include special stringification functions and struct/tuple definitions etc.

Helper functions in the test class, member variables in the test class for persistent stuff, and custom matchers, will typically be fewer lines overall than a monolithic "do all the same stuff with different inputs" function - you can still put all the repetitive stuff inside helper functions.

In general if you use TEST_P with only one test, you shouldn't be using TEST_P - it's useful for the combinatorial thing where you want to run multiple tests with a range of configuration states.

Copy link
Contributor Author

@cpakulski cpakulski Oct 28, 2025

Choose a reason for hiding this comment

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

@ravenblackx Thanks for reviewing. I think you have very good points here. What bothered me was the amount of repeated code and I was under the impression that the best way to solve them was using parameterized tests. But your point that it is difficult to read such tests is a very valid one! Especially, when COMBINE is used to built a matrix of parameters.
I am going to close this PR and refactor my tests to use helper functions (as you suggest), which should be much cleaner and easier to understand.

Copy link
Contributor

@ravenblackx ravenblackx Oct 28, 2025

Choose a reason for hiding this comment

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

One more minor suggestion that may or may not help - sometimes a good thing to do is subclass a test class with some common initialization, so you can have a "generic" test class you use for awkward cases, where you call the helper functions explicitly, and the "common" test class where all the initialization and teardown helpers are called with default args in the SetUp/TearDown/constructor/destructor of the class and you just do the one differing part explicitly.
(But also sometimes it may be clearer to just always explicitly call the helpers, or use a short version of the helpers like defaultInitialize().)

The subclass thing is especially likely to be useful in integration tests because there sometimes the integration test superclass performs some initialization, so you have to prepare different setup stuff during the constructor or SetUp function, via an overridden virtual function.

Copy link
Contributor Author

@cpakulski cpakulski Oct 28, 2025

Choose a reason for hiding this comment

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

The subclass thing is especially likely to be useful in integration tests because there sometimes the integration test superclass performs some initialization, so you have to prepare different setup stuff during the constructor or SetUp function, via an overridden virtual function.

This is a great point. I will try to refactor the same tests I tried to parameterize in this PR and will see where special initialization is needed.

public:
void initialize() override { HttpProtocolIntegrationTest::initialize(); }
};
static constexpr size_t TEST_NAME = 0;
static constexpr size_t BASIC_OD_CONFIG = 1;
static constexpr size_t EXT_OD_CONFIG = 2;
static constexpr size_t RESPONSE_CODE = 3;
static constexpr size_t OPTIONAL_HEADER = 4;

INSTANTIATE_TEST_SUITE_P(
Protocols, OutlierDetectionIntegrationTest,
testing::ValuesIn(HttpProtocolIntegrationTest::getProtocolTestParamsWithoutHTTP3()),
static std::string protocolTestParamsToString(const ::testing::TestParamInfo<ParamType>& params) {

HttpProtocolIntegrationTest::protocolTestParamsToString);

// Test verifies that empty outlier detection setting in protocol options
// do not interfere with existing outlier detection.
TEST_P(OutlierDetectionIntegrationTest, NoClusterOverwrite) {
config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) {
auto* static_resources = bootstrap.mutable_static_resources();
auto* cluster = static_resources->mutable_clusters(0);

cluster->mutable_common_lb_config()->mutable_healthy_panic_threshold()->set_value(0);

auto* outlier_detection = cluster->mutable_outlier_detection();

TestUtility::loadFromYaml(R"EOF(
return absl::StrCat(
HttpProtocolIntegrationTestBase::testNameFromTestParams(std::get<0>(params.param)), "_",
std::get<TEST_NAME>(std::get<1>(params.param)));
}
// Set of basic outlier detection configs.
static constexpr absl::string_view consecutive_5xx_only = R"EOF(
consecutive_5xx: 2
max_ejection_percent: 100
)EOF",
*outlier_detection);

ConfigHelper::HttpProtocolOptions protocol_options;
std::string protocol_options_yaml;
if (absl::StrContains(::testing::UnitTest::GetInstance()->current_test_info()->name(),
"Http2Upstream")) {
protocol_options_yaml += R"EOF(
explicit_http_config:
http2_protocol_options: {}
)EOF";
} else {
ASSERT(absl::StrContains(::testing::UnitTest::GetInstance()->current_test_info()->name(),
"HttpUpstream"));
protocol_options_yaml += R"EOF(
explicit_http_config:
http_protocol_options: {}
)EOF";
}
TestUtility::loadFromYaml(protocol_options_yaml, protocol_options);
ConfigHelper::setProtocolOptions(*bootstrap.mutable_static_resources()->mutable_clusters(0),
protocol_options);
});

initialize();

codec_client_ = makeHttpConnection(lookupPort("http"));
// return en error from upstream server.
default_response_headers_.setStatus(500);
for (auto i = 1; i <= 2; i++) {
auto response =
sendRequestAndWaitForResponse(default_request_headers_, 0, default_response_headers_, 0);

ASSERT_TRUE(response->waitForEndStream());
// 500 error should be propagated to downstream client.
EXPECT_EQ("500", response->headers().getStatusValue());
}

// Send another request. It should not reach upstream and should be handled by envoy.
// The only existing endpoint in the cluster has been marked as unhealthy.
IntegrationStreamDecoderPtr response =
codec_client_->makeHeaderOnlyRequest(default_request_headers_);
ASSERT_TRUE(response->waitForEndStream());
EXPECT_TRUE(response->complete());
EXPECT_EQ("503", response->headers().getStatusValue());

codec_client_->close();
}

// Test verifies that non-5xx codes defined in cluster's protocol options
// are thread as errors and cause outlier detection to mark a host as unhealthy.
TEST_P(OutlierDetectionIntegrationTest, ClusterOverwriteNon5xxAsErrors) {
config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) {
auto* static_resources = bootstrap.mutable_static_resources();
auto* cluster = static_resources->mutable_clusters(0);

cluster->mutable_common_lb_config()->mutable_healthy_panic_threshold()->set_value(0);

auto* outlier_detection = cluster->mutable_outlier_detection();

TestUtility::loadFromYaml(R"EOF(
consecutive_5xx: 2
static constexpr absl::string_view gateway_only = R"EOF(
consecutive_5xx: 0
consecutive_gateway_failure: 2
enforcing_consecutive_gateway_failure: 100
max_ejection_percent: 100
)EOF",
*outlier_detection);

ConfigHelper::HttpProtocolOptions protocol_options;
std::string protocol_options_yaml;
if (absl::StrContains(::testing::UnitTest::GetInstance()->current_test_info()->name(),
"Http2Upstream")) {
protocol_options_yaml += R"EOF(
explicit_http_config:
http2_protocol_options: {}
)EOF";
} else {
ASSERT(absl::StrContains(::testing::UnitTest::GetInstance()->current_test_info()->name(),
"HttpUpstream"));
protocol_options_yaml += R"EOF(
explicit_http_config:
http_protocol_options: {}
)EOF";
}
)EOF";

// Configure any response with code 300-305 or response test-header containing
// string "treat-as-error" to be treated as 5xx code.
protocol_options_yaml += R"EOF(
// set of different outlier detection extension configs.
// Configure any response with code 300-305 or response test-header containing
// string "treat-as-error" to be treated as 5xx code.
static constexpr absl::string_view error_3xx_and_header = R"EOF(
outlier_detection:
error_matcher:
or_match:
Expand All @@ -322,46 +250,25 @@ TEST_P(OutlierDetectionIntegrationTest, ClusterOverwriteNon5xxAsErrors) {
- name: "test-header"
string_match:
contains: "treat-as-error"
)EOF",

TestUtility::loadFromYaml(protocol_options_yaml, protocol_options);
ConfigHelper::setProtocolOptions(*bootstrap.mutable_static_resources()->mutable_clusters(0),
protocol_options);
});

initialize();

codec_client_ = makeHttpConnection(lookupPort("http"));

// respond with status code 301. It should be treated as error.
default_response_headers_.setStatus(301);
auto response =
sendRequestAndWaitForResponse(default_request_headers_, 0, default_response_headers_, 0);
ASSERT_TRUE(response->waitForEndStream());
EXPECT_EQ("301", response->headers().getStatusValue());

// Respond with status code 200 with "test-header".
// It should be treated as error.
default_response_headers_.setStatus(200);
default_response_headers_.appendCopy(Http::LowerCaseString("test-header"), "treat-as-error");

response =
sendRequestAndWaitForResponse(default_request_headers_, 0, default_response_headers_, 0);
ASSERT_TRUE(response->waitForEndStream());
EXPECT_EQ("200", response->headers().getStatusValue());
)EOF";

// Now send a request. It will be captured by Envoy and 503 will be returned as the only upstream
// is unhealthy now..
response = codec_client_->makeHeaderOnlyRequest(default_request_headers_);
ASSERT_TRUE(response->waitForEndStream());
EXPECT_TRUE(response->complete());
EXPECT_EQ("503", response->headers().getStatusValue());
static constexpr absl::string_view gateway_error = R"EOF(
outlier_detection:
error_matcher:
http_response_headers_match:
headers:
- name: ":status"
range_match:
start: 502
end: 503
)EOF";
};

codec_client_->close();
}
// Test verifies that 5xx gateway errors configured in cluster protocol options are
// forwarded to outlier detection in the original form and are not converted to code 500.
TEST_P(OutlierDetectionIntegrationTest, ClusterOverwriteGatewayErrors) {
// Test configures outlier detection extensions in cluster.
// Then it sends several http requests and responses.
// Responses match configured outlier extensions and cause the endpoint to be marked
// as unhealthy.
TEST_P(OutlierDetectionIntegrationTest, ExtensionsTest) {
config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) {
auto* static_resources = bootstrap.mutable_static_resources();
auto* cluster = static_resources->mutable_clusters(0);
Expand All @@ -370,12 +277,7 @@ TEST_P(OutlierDetectionIntegrationTest, ClusterOverwriteGatewayErrors) {

auto* outlier_detection = cluster->mutable_outlier_detection();

TestUtility::loadFromYaml(R"EOF(
consecutive_5xx: 0
consecutive_gateway_failure: 2
enforcing_consecutive_gateway_failure: 100
max_ejection_percent: 100
)EOF",
TestUtility::loadFromYaml(std::string(std::get<BASIC_OD_CONFIG>(std::get<1>(GetParam()))),
*outlier_detection);

ConfigHelper::HttpProtocolOptions protocol_options;
Expand All @@ -395,36 +297,35 @@ TEST_P(OutlierDetectionIntegrationTest, ClusterOverwriteGatewayErrors) {
)EOF";
}

protocol_options_yaml += R"EOF(
outlier_detection:
error_matcher:
http_response_headers_match:
headers:
- name: ":status"
range_match:
start: 502
end: 503
)EOF",
// Add config outlier detection extension config
protocol_options_yaml += std::get<EXT_OD_CONFIG>(std::get<1>(GetParam()));

TestUtility::loadFromYaml(protocol_options_yaml, protocol_options);
TestUtility::loadFromYaml(protocol_options_yaml, protocol_options);
ConfigHelper::setProtocolOptions(*bootstrap.mutable_static_resources()->mutable_clusters(0),
protocol_options);
});

initialize();

codec_client_ = makeHttpConnection(lookupPort("http"));
default_response_headers_.setStatus(502);
// Set the response code.
const uint32_t response_code = std::get<RESPONSE_CODE>(std::get<1>(GetParam()));
default_response_headers_.setStatus(response_code);
// Add header if test parameter includes one.
if (!std::get<OPTIONAL_HEADER>(std::get<1>(GetParam())).empty()) {
default_response_headers_.appendCopy(Http::LowerCaseString("test-header"),
std::get<OPTIONAL_HEADER>(std::get<1>(GetParam())));
}
for (auto i = 1; i <= 2; i++) {
auto response =
sendRequestAndWaitForResponse(default_request_headers_, 0, default_response_headers_, 0);

ASSERT_TRUE(response->waitForEndStream());
EXPECT_EQ("502", response->headers().getStatusValue());
EXPECT_EQ(std::to_string(response_code), response->headers().getStatusValue());
}

// Now send a request. It will be captured by Envoy and 503 will be returned as the only upstream
// is unhealthy now..
// is unhealthy now.
IntegrationStreamDecoderPtr response =
codec_client_->makeHeaderOnlyRequest(default_request_headers_);
ASSERT_TRUE(response->waitForEndStream());
Expand All @@ -434,5 +335,30 @@ TEST_P(OutlierDetectionIntegrationTest, ClusterOverwriteGatewayErrors) {
codec_client_->close();
}

INSTANTIATE_TEST_SUITE_P(
Protocols, OutlierDetectionIntegrationTest,
testing::Combine(
testing::ValuesIn(HttpProtocolIntegrationTestBase::getProtocolTestParamsWithoutHTTP3()),
testing::Values(
// Regression test. Test verifies that empty outlier detection setting in protocol
// options do not interfere with existing outlier detection.
std::make_tuple(std::string("test1"),
OutlierDetectionIntegrationTest::consecutive_5xx_only, "", 500, ""),
// In this test, outlier extensions define 3xx codes as errors.
std::make_tuple(std::string("test2"),
OutlierDetectionIntegrationTest::consecutive_5xx_only,
OutlierDetectionIntegrationTest::error_3xx_and_header, 301, ""),
// Test verifies that when outlier extensions include gateway code 502, that code
// is forwarded in original form and is not converted to generic code 500.
std::make_tuple(std::string("test3"), OutlierDetectionIntegrationTest::gateway_only,
OutlierDetectionIntegrationTest::gateway_error, 502, ""),
// Test verifies that responses where a matcher matches not code, but response
// header are treated as errors.
std::make_tuple(std::string("test4"),
OutlierDetectionIntegrationTest::consecutive_5xx_only,
OutlierDetectionIntegrationTest::error_3xx_and_header, 200,
"treat-as-error"))),
OutlierDetectionIntegrationTest::protocolTestParamsToString);

} // namespace
} // namespace Envoy
23 changes: 14 additions & 9 deletions test/integration/http_protocol_integration.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
#include "absl/strings/str_cat.h"

namespace Envoy {
std::vector<HttpProtocolTestParams> HttpProtocolIntegrationTest::getProtocolTestParams(
std::vector<HttpProtocolTestParams> HttpProtocolIntegrationTestBase::getProtocolTestParams(
const std::vector<Http::CodecType>& downstream_protocols,
const std::vector<Http::CodecType>& upstream_protocols) {
std::vector<HttpProtocolTestParams> ret;
Expand Down Expand Up @@ -54,16 +54,21 @@ std::vector<HttpProtocolTestParams> HttpProtocolIntegrationTest::getProtocolTest
return ret;
}

std::string HttpProtocolIntegrationTest::protocolTestParamsToString(
std::string
HttpProtocolIntegrationTestBase::testNameFromTestParams(const HttpProtocolTestParams& params) {
return absl::StrCat((params.version == Network::Address::IpVersion::v4 ? "IPv4_" : "IPv6_"),
downstreamToString(params.downstream_protocol),
upstreamToString(params.upstream_protocol),
http2ImplementationToString(params.http2_implementation),
params.use_universal_header_validator ? "Uhv" : "Legacy");
}

std::string HttpProtocolIntegrationTestBase::protocolTestParamsToString(
const ::testing::TestParamInfo<HttpProtocolTestParams>& params) {
return absl::StrCat((params.param.version == Network::Address::IpVersion::v4 ? "IPv4_" : "IPv6_"),
downstreamToString(params.param.downstream_protocol),
upstreamToString(params.param.upstream_protocol),
http2ImplementationToString(params.param.http2_implementation),
params.param.use_universal_header_validator ? "Uhv" : "Legacy");
return testNameFromTestParams(params.param);
}

void HttpProtocolIntegrationTest::setUpstreamOverrideStreamErrorOnInvalidHttpMessage() {
void HttpProtocolIntegrationTestBase::setUpstreamOverrideStreamErrorOnInvalidHttpMessage() {
config_helper_.addConfigModifier([&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void {
RELEASE_ASSERT(bootstrap.mutable_static_resources()->clusters_size() >= 1, "");
ConfigHelper::HttpProtocolOptions protocol_options;
Expand All @@ -83,7 +88,7 @@ void HttpProtocolIntegrationTest::setUpstreamOverrideStreamErrorOnInvalidHttpMes
});
}

void HttpProtocolIntegrationTest::setDownstreamOverrideStreamErrorOnInvalidHttpMessage() {
void HttpProtocolIntegrationTestBase::setDownstreamOverrideStreamErrorOnInvalidHttpMessage() {
config_helper_.addConfigModifier(
[](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
hcm) -> void {
Expand Down
Loading