-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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.
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Christoph Pakulski <[email protected]>
Signed-off-by: Christoph Pakulski <[email protected]>
Signed-off-by: Christoph Pakulski <[email protected]>
Signed-off-by: Christoph Pakulski <[email protected]>
Signed-off-by: Christoph Pakulski <[email protected]>
Signed-off-by: Christoph Pakulski <[email protected]>
@ravenblackx IIRC you are maintainer for tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having argued for not using TEST_P for this example case, I'm not sure if there is a realistic case for where you'd really want to combine another parameterization on top of the IPv4/IPv6/Http1/Http2/Http3 combo that the integration tests already use.
I don't fundamentally object to making it possible to do so, but I would very much like to discourage doing so when it's not actually appropriate. We already have a bunch of TEST_P that shouldn't be.
: public HttpProtocolIntegrationTestWithParams<std::tuple< | ||
std::string, absl::string_view, absl::string_view, uint32_t, absl::string_view>> { |
There was a problem hiding this comment.
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.
Commit Message:
enable parameterized tests for
HttpProtocolIntegrationTest
Additional Description:
While working on #39947, I noticed that it was impossible to write parameterized tests for classes derived from
HttpProtocolIntegrationTest
, becauseHttpProtocolIntegrationTest
was derived fromtesting::TestWithParam<HttpProtocolTestParams>
, so adding new params was not possible. This resulted in lots of duplicated test routines with little differences. This PR reshuffles test code/classes to allow adding new params on top ofHttpProtocolTestParams
, but leaves existing routines derived fromHttpProtocolIntegrationTest
intact. All tests which I added in #39947 were parameterized.Risk Level: Low
Testing: All existing tests should pass
Docs Changes: No
Release Notes: No
Platform Specific Features: