Use shared test facilities for time-tracking and dynamic-delay#541
Use shared test facilities for time-tracking and dynamic-delay#541mum4k merged 4 commits intoenvoyproxy:masterfrom
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. |
dubious90
left a comment
dubious90
left a comment
There was a problem hiding this comment.
migration looks good. Also have one chage-level question:
I'm surprised there are no corresponding bazel changes - do we need to bring in a dependency on the new fixture and remove the old one?
| 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?
| 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
dubious90
added
waiting-for-changes
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
|
@dubious90 feedback applied in 2fa83ee |
oschaaf
added
waiting-for-review
…r-prelude-5 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
mum4k
left a comment
mum4k
left a comment
There was a problem hiding this comment.
Looks good.
Can you kick Circle CI to ensure all checks pass?
mum4k
added
waiting-for-changes
…r-prelude-5 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
oschaaf
added
waiting-for-review
Done, merged master in here, and tests are passing now. |
3 participants
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