Conversation
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
test/common/http/dispatcher_test.cc
Outdated
| ASSERT_EQ(cc.on_complete_calls, 1); | ||
| } | ||
|
|
||
| TEST_F(DispatcherTest, BasicStreamWithTrailers) { |
There was a problem hiding this comment.
This test feels quite large and it's kind of difficult to understand what suite of behaviors we're trying to exercise here. Do you know if we can isolate the specific behaviors and have smaller individual tests?
There was a problem hiding this comment.
Some of these tests can be condensed a little bit by taking the repeated parts into utility functions in the test class. I was thinking about this as I was adding more tests. Will do in a subsequent PR #794
There was a problem hiding this comment.
I think that's great to do also (I am slightly opinionated about test utils but we can talk about that separately)
The confusion I have is more on the behavior exercised by this test (since it seems to be asserting on all the different handlers). Does this test primarily exercise a general end to end behavior of a basic stream?
There was a problem hiding this comment.
yeah, it is an http stream with trailers. There are individual tests for a headers only request, one with a body, and now this one with trailers.
| http_dispatcher_.synchronizer().signal("dispatch_encode_final_data"); | ||
| } | ||
|
|
||
| // TODO(junr03): test with envoy local reply with local stream not close, which causes a reset fired |
There was a problem hiding this comment.
| // TODO(junr03): test with envoy local reply with local stream not close, which causes a reset fired | |
| // TODO(junr03): test with envoy local reply with local stream not closed, which causes a reset fired |
There was a problem hiding this comment.
if you are ok with it, I will fix this in #791 to avoid re-running CI
|
|
||
| // Ensure that the callbacks on the bridge_callbacks were called. | ||
| ASSERT_EQ(cc.on_complete_calls, 0); | ||
| ASSERT_EQ(cc.on_error_calls, 1); |
There was a problem hiding this comment.
I agree with @buildbreaker's comment above regarding encapsulating testable pieces so we don't have large test functions. Another idea might be to break up this file with those smaller individual tests (it's ~1500LOC)
buildbreaker
left a comment
buildbreaker
left a comment
There was a problem hiding this comment.
@junr03 I like the separation within the tests! It's a lot clearer to me what things we are exercising at each test.
I feel like we could break them down even more but it might be my own personal preferences. Happy to chat more!
|
Yeah open to more suggestions @buildbreaker. I opened #794 yesterday to track. We can chat, or you can drop an example in the issue |
3 participants
Description: this PR adds tests based on local coverage runs to raise coverage. This PR adds all non-silly coverage, but still hits misses on some accessors and because
NOT_IMPLEMENTED_GCOVR_EXCL_LINEis being included (known problem in Envoy upstream since we stopped using gcovr).Risk Level: lowest! we are increasing our code coverage :)
Testing: local coverage runs to see missing spots, and then CI
Signed-off-by: Jose Nino jnino@lyft.com