Use shared test facilities for time-tracking and dynamic-delay #541
Conversation
Switch tests for these extensions to use the recent shared test facilities. Eliminate tests we generalized in envoyproxy#533. Split out from envoyproxy#512 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
|
@dubious90 please review and assign back to me once done. |
| }; | ||
|
|
||
| INSTANTIATE_TEST_SUITE_P(IpVersions, HttpDynamicDelayIntegrationTest, | ||
| testing::ValuesIn(Envoy::TestEnvironment::getIpVersionsForTest())); | ||
|
|
||
| // Verify expectations with an empty dynamic-delay configuration. | ||
| TEST_P(HttpDynamicDelayIntegrationTest, NoStaticConfiguration) { | ||
| setup(R"( | ||
| initializeFilterConfiguration(R"( | ||
| name: dynamic-delay | ||
| typed_config: | ||
| "@type": type.googleapis.com/nighthawk.server.ResponseOptions | ||
| )"); | ||
| // Don't send any config request header |
There was a problem hiding this comment.
Contxt for a few comments here: If I understand correctly, we're saving some effort by having these all be one test to prevent having to initialize a filter repeatedly (which I assume is slow?). That makes sense here.
However, I think it would be good if we clarified individual test cases within the TEST_P block a little.
-
"Don't send any config request header" is describing what we are doing - can we add in what behavior we're expecting this to produce?
-
Can we space out all of the individual test cases separately:
e.g.
getResponse();
EXPECT_EQ()
// Comment for test 2
...```
| setRequestLevelConfiguration("{}"); | ||
| getResponse(ResponseOrigin::UPSTREAM); | ||
| EXPECT_EQ(upstream_request_->headers().get(kDelayHeaderString), nullptr); | ||
| // Send a config request header, this should become effective. |
There was a problem hiding this comment.
Rather than saying this should become effective, can we describe what the behavior should be?
| // Send a config request header with an empty / default config. Should be a no-op. | ||
| getResponse("{}"); | ||
| setRequestLevelConfiguration("{}"); | ||
| getResponse(ResponseOrigin::UPSTREAM); | ||
| // Send a config request header, this should become effective. |
There was a problem hiding this comment.
same comments here about describing behaviors
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
|
@dubious90 feedback applied in 2fa83ee |
…r-prelude-5 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
…r-prelude-5 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Done, merged master in here, and tests are passing now. |
Switch tests for these extensions to use the recent shared test
facilities. Eliminate tests we generalize in #533.
Split out from #512
Signed-off-by: Otto van der Schaaf oschaaf@we-amp.com