coverage: add some more tests to improve http2 code coverage #39149
Conversation
Signed-off-by: Rohit Agrawal <rohit.agrawal@databricks.com>
|
Coverage for this Pull Request will be rendered here: https://storage.googleapis.com/envoy-pr/39149/coverage/index.html The coverage results are (re-)rendered each time the CI |
|
/retest |
| #pragma once | ||
|
|
||
| #include <cstdint> | ||
| #include <deque> |
There was a problem hiding this comment.
Could you move this to frame_replay.h (in the other PR if splitting as suggested)? It's not used in this file.
There was a problem hiding this comment.
The changes in BUILD, frame_replay.h and frame_replay_test.h don't seem to be connected at all to the changes in http2_frame.h and http2_frame_test.h (other than the one misplaced #include). Could you split this into two PRs, and/or make FrameReplay in a PR that uses it? Seems like in this PR it just appears and tests itself but has no use-case.
| ASSERT_EQ(replay.frameCount(), 2); | ||
| ASSERT_EQ(replay.nextFrame().type(), Http2Frame::Type::Ping); | ||
| ASSERT_EQ(replay.nextFrame().type(), Http2Frame::Type::Ping); |
There was a problem hiding this comment.
Most of the ASSERTs in this PR should be EXPECTs. The exception being the frameCount ones, which should abort the test if not met because an underflow there would lead to a test-crash.
Ideally you'd use matchers for this sort of thing - i.e. instead of
ASSERT(count);
EXPECT(element_0, properties);
EXPECT(element_1, properties);
if you use some custom matchers and write the same thing something like
EXPECT_THAT(replay.allFrames(), HasElements(
FrameTypeIs(Http2Frame::Type::Ping),
FrameTypeIs(Http2Frame::Type::Ping),
));
then the output on failure is much more helpful - an ASSERT failure on the count would tell you it's the wrong number and then abort the test, so all you'd get is "expected 2 but got 3", while ElementsAre will validate the count, the contents, and the order, and will explain the expectations and content (and maybe the difference depending on how you implement the matcher).
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
github-actions
bot
added
the
stale
|
This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
2 participants
Description
This PR adds some more tests to improve the overall coverage of H2.
Commit Message: coverage: add some more tests to improve http2 code coverage
Additional Description: Additional tests for improving the overall coverage of H2
Risk Level: N/A
Testing: Adding Unit Tests
Docs Changes: N/A
Release Notes: N/A