Skip to content
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
805f950
Add HttpFilterIntegrationTestBase
oschaaf Sep 9, 2020
0c6f182
Merge remote-tracking branch 'upstream/master' into extension-refacto…
oschaaf Sep 10, 2020
8d30820
Review feedback
oschaaf Sep 11, 2020
4827909
Merge remote-tracking branch 'upstream/master' into extension-refacto…
oschaaf Sep 11, 2020
5f18691
Merge remote-tracking branch 'upstream/master' into extension-refacto…
oschaaf Sep 14, 2020
7eeb7e9
Add setRequestHeader
oschaaf Sep 14, 2020
467e8a7
Add generic c++ integration test for uniform extension behavior.
oschaaf Sep 14, 2020
e4a50bd
Merge remote-tracking branch 'upstream/master' into extension-refacto…
oschaaf Sep 14, 2020
ca17f41
Review feedback
oschaaf Sep 14, 2020
9a2f69e
Merge branch 'extension-refactor-prelude' into extension-refactor-pre…
oschaaf Sep 15, 2020
97322d8
Review feedback sync
oschaaf Sep 15, 2020
139cfff
Merge remote-tracking branch 'upstream/master' into extension-refacto…
oschaaf Sep 16, 2020
2aff84a
Review feedback
oschaaf Sep 16, 2020
fa2ff48
Merge branch 'extension-refactor-prelude' into extension-refactor-pre…
oschaaf Sep 16, 2020
f0c8eba
Sync file I forgot
oschaaf Sep 16, 2020
3440fd9
Merge remote-tracking branch 'upstream/master' into extension-refacto…
oschaaf Sep 17, 2020
0f3110c
Merge remote-tracking branch 'upstream/master' into extension-refacto…
oschaaf Sep 18, 2020
ab3585b
Merge remote-tracking branch 'upstream/master' into extension-refacto…
oschaaf Sep 22, 2020
5ff6d56
Review feedback
oschaaf Sep 22, 2020
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
16 changes: 13 additions & 3 deletions test/server/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,26 @@ envoy_cc_test_library(
],
)

envoy_cc_test(
name = "http_filter_base_test",
srcs = ["http_filter_base_test.cc"],
repository = "@envoy",
deps = [
":http_filter_integration_test_base_lib",
"//source/server:http_dynamic_delay_filter_config",
"//source/server:http_test_server_filter_config",
"//source/server:http_time_tracking_filter_config",
],
)

envoy_cc_test(
name = "http_test_server_filter_integration_test",
srcs = ["http_test_server_filter_integration_test.cc"],
repository = "@envoy",
deps = [
":http_filter_integration_test_base_lib",
"//source/server:http_test_server_filter_config",
"@envoy//include/envoy/upstream:cluster_manager_interface_with_external_headers",
"@envoy//source/common/api:api_lib_with_external_headers",
"@envoy//test/integration:http_integration_lib",
],
)

Expand All @@ -50,7 +61,6 @@ envoy_cc_test(
deps = [
":http_filter_integration_test_base_lib",
"//source/server:http_time_tracking_filter_config",
"@envoy//include/envoy/upstream:cluster_manager_interface_with_external_headers",
"@envoy//source/common/api:api_lib_with_external_headers",
"@envoy//test/test_common:simulated_time_system_lib",
],
Expand Down
89 changes: 89 additions & 0 deletions test/server/http_filter_base_test.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
#include "server/http_dynamic_delay_filter.h"
#include "server/http_test_server_filter.h"
#include "server/http_time_tracking_filter.h"

#include "test/server/http_filter_integration_test_base.h"

#include "gtest/gtest.h"

namespace Nighthawk {

using namespace testing;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do we need this, since most symbols from the namespace are qualified? E.g. ::testing::Combine.

If you do prefer to shorten them, can we list them explicitly instead of subscribing to the entire testing namespace which could cause a conflict and confuse someone in the future?

E.g. for symbols that repeat a lot we can:

using ::testing::Combine;
using ::testing::ValuesIn;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done; added an anonymous namespace as well based on that it can only benefit us.


class HttpFilterBaseIntegrationTest
: public HttpFilterIntegrationTestBase,
public testing::TestWithParam<
std::tuple<Envoy::Network::Address::IpVersion, absl::string_view, bool>> {
public:
HttpFilterBaseIntegrationTest() : HttpFilterIntegrationTestBase(std::get<0>(GetParam())){};
};

INSTANTIATE_TEST_SUITE_P(
IpVersions, HttpFilterBaseIntegrationTest,
::testing::Combine(testing::ValuesIn(Envoy::TestEnvironment::getIpVersionsForTest()),
testing::ValuesIn({absl::string_view(R"EOF(
name: time-tracking
typed_config:
"@type": type.googleapis.com/nighthawk.server.ResponseOptions
emit_previous_request_delta_in_response_header: "foo"
)EOF"),
absl::string_view(R"EOF(
name: dynamic-delay
typed_config:
"@type": type.googleapis.com/nighthawk.server.ResponseOptions
static_delay: 0.1s
)EOF"),
absl::string_view("name: test-server")}),
testing::ValuesIn({true, false})));

// Verify extensions are well-behaved when it comes to:
// - GET requests.
// - POST requests with an entity body (takes a slightly different code path).
// - Valid but empty configuration.
// - Bad request-level json configuration.
// We will be low on functional expectations for the extensions, but this will catch hangs and
// ensure that bad configuration handling is in-place.
TEST_P(HttpFilterBaseIntegrationTest, BasicExtensionFlows) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we split these into separate tests? I'm not sure what the preference in the code base is, but it seems like each chunk is testing a different behaviour and might deserve a separate test?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I can see your point, and in general I agree that the clearer test names that arise out of more finely grained separate tests are better.
But in this case, when splitting this, up we would have to duplicate the parameter unpacking for the TEST_P params across the newly split up tests and/or add facilities to help with that.
This was also sort of pre-existing (as we can eliminate three different instances of similar code in the test code for three extensions).

So personally I lean towards the compact version as is is now. With the context above, are you comfortable with leaving this as-is?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, makes sense to me.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Adding to what @wjuan-AFK commented on - it would really help if we can separate these into individual test cases - one per behavior. Such approach simplifies future code changes, because it ensures that the tests won't be affected when a new behavior is added or that less tests are affected when a behavior is changed. Additionally when a behavior breaks, we get a clear signal which behavior broke, rather than a generic failure saying "the BasicExtensionFlows no longer works".

I see your point about duplicating the parameter unpacking. I wonder if we could place that into a separate helper function. If we figure out we can't, I think we should prefer more instances of smaller independent tests for individual behavior to shorter code that uses parametrised tests. In other words, if we need to drop the parametrised test approach and write few more test cases, I think that is a worthwhile price to pay for shorter and easier to maintain test cases.

Can we burn another cycle on this and see if we can find a win-win solution?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I will take a look. Hopefully a convenient helper will suffice to support this, and then I can split these out into different tests. I'd like to avoid the need to drop parameterized testing here if we can: we would not only have to duplicate the test(s) for our three current extensions, but also require that for future extensions that would want to piggy back on this.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done in 5ff6d56.
@wjuan-AFK sorry, this was actually straight forward, and looking at the results, I rather like it better compared to what it was.

absl::string_view config = std::get<1>(GetParam());
initializeFilterConfiguration(std::string(config));
bool is_post = std::get<2>(GetParam());
if (is_post) {
switchToPostWithEntityBody();
}

// Modulo the test-server, extensions are expected to need an upstream to synthesize a reply
// when effective configuration is valid.
ResponseOrigin happy_flow_response_origin = config.find_first_of("name: test-server") == 0
? ResponseOrigin::EXTENSION
: ResponseOrigin::UPSTREAM;
// Post without any request-level configuration. Should succeed.
Envoy::IntegrationStreamDecoderPtr response = getResponse(happy_flow_response_origin);
ASSERT_TRUE(response->complete());
EXPECT_EQ("200", response->headers().Status()->value().getStringView());
EXPECT_TRUE(response->body().empty());

// Test with a valid but empty request-level configuration.
setRequestLevelConfiguration("{}");
response = getResponse(happy_flow_response_origin);
ASSERT_TRUE(response->complete());
EXPECT_EQ("200", response->headers().Status()->value().getStringView());
EXPECT_TRUE(response->body().empty());

const std::string kBadConfigErrorSentinel =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not use status instead of doing this substring check?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We do check the http response code, but then also look for this substring in the response body because we know it should look like that if the extension under test is using the shared configuration handling code. Does that sound good?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes. That also makes sense.

"didn't understand the request: Error merging json config: Unable to parse "
"JSON as proto (INVALID_ARGUMENT:Unexpected";

// When sending bad request-level configuration, the extension ought to reply directly.
setRequestLevelConfiguration("bad_json");
response = getResponse(ResponseOrigin::EXTENSION);
EXPECT_EQ(Envoy::Http::Utility::getResponseStatus(response->headers()), 500);
EXPECT_THAT(response->body(), HasSubstr(kBadConfigErrorSentinel));

// When sending empty request-level configuration, the extension ought to reply directly.
setRequestLevelConfiguration("");
response = getResponse(ResponseOrigin::EXTENSION);
EXPECT_EQ(Envoy::Http::Utility::getResponseStatus(response->headers()), 500);
EXPECT_THAT(response->body(), HasSubstr(kBadConfigErrorSentinel));
}

} // namespace Nighthawk