Fix benchmark_http_client_test.cc and add test cases.#368
Merged
mum4k merged 4 commits intoenvoyproxy:masterfrom Jun 19, 2020
qqustc:fix-bm-client-test
Merged
Fix benchmark_http_client_test.cc and add test cases.#368mum4k merged 4 commits intoenvoyproxy:masterfrom qqustc:fix-bm-client-test
mum4k merged 4 commits intoenvoyproxy:masterfrom
qqustc:fix-bm-client-test
Conversation
Fix benchmark_http_client_test.cc which has a bug in function testBasicFunctionality(). Added 2 test cases to verify the expected behavior. Signed-off-by: qqustc <qqin@google.com>
Signed-off-by: qqustc <qqin@google.com>
Signed-off-by: qqustc <qqin@google.com>
oschaaf
requested changes
Jun 19, 2020
Member
oschaaf
left a comment
There was a problem hiding this comment.
Nice! LGTM overall, one small request.
| }; | ||
|
|
||
| // Note we we explicitly do not expect encodeData to be called. | ||
| EXPECT_CALL(stream_encoder_, encodeData(_, _)).Times(0); |
Member
There was a problem hiding this comment.
(Optional) now that we actually have an explicit expectation in place instead of an implied one, we could probably do without the comment above this line?
Contributor
Author
There was a problem hiding this comment.
Done.
Thank you Otto for the review.
Verified the change pass all unit tests except termination_predicate_test which is failing at master branch.
bazel test test:all
Member
There was a problem hiding this comment.
Great, thank you, this looks good to me. As for the termination_predicate_test failure, could you paste the details here or in a separate issue? Maybe it is flaking (I can't find an occurrence of that, neither on master).
Signed-off-by: qqustc <qqin@google.com>
oschaaf
approved these changes
Jun 19, 2020
mum4k
approved these changes
Jun 19, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix benchmark_http_client_test.cc which has a bug in function testBasicFunctionality().
For example, if we have a test fixture with testBasicFunctionality(max_pending=0,connection_limit=15,amount_of_request=10),
then EXPECT_FALSE(client_->tryStartRequest(f)) and
EXPECT_EQ(max_pending == 0 ? 1 : max_pending, inflight_response_count) will fail.
For test fixture with
testBasicFunctionality(max_pending=2,connection_limit=3,amount_of_request=10),
EXPECT_EQ(max_pending == 0 ? 1 : max_pending, inflight_response_count) will fail.
Added 2 test cases to verify the expected behavior fixed by this change.
Tested:
Unit test.