missing coverage in fault filter

[mattklein123]

@rshriram I noticed the following line is missing coverage in the fault filter. Do you mind adding a test for that case? It's pretty important to make sure the timer gets correctly disabled, etc.

https://github.com/lyft/envoy/blob/master/source/common/http/filter/fault_filter.cc#L121

[rshriram]

Correct me if I am wrong. I am not testing that function directly. However, see the following line https://github.com/lyft/envoy/blob/master/test/common/http/filter/fault_filter_test.cc#L85 that checks if timers are disabled after use. And here is an example test case: https://github.com/lyft/envoy/blob/master/test/common/http/filter/fault_filter_test.cc#L274 WDYT? Also, is there a tool that provides line by line code coverage stats?

On Tue, Jan 10, 2017 at 10:44 PM Matt Klein ***@***.***> wrote: @rshriram <https://github.com/rshriram> I noticed the following line is missing coverage in the fault filter. Do you mind adding a test for that case? It's pretty important to make sure the timer gets correctly disabled, etc. https://github.com/lyft/envoy/blob/master/source/common/http/filter/fault_filter.cc#L121 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#342>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AH0qd7XJfn_U5x8XxxeU4tnu8fA0MBacks5rRFAugaJpZM4LgKii> .

-- ~shriram

[mattklein123]

You need to have a test where you invoke the stream reset callback like this:
https://github.com/lyft/envoy/blob/master/test/common/http/filter/buffer_filter_test.cc#L110

This will insure that things are cleaned up properly in the reset case.

You can do a full coverage build like so:
https://github.com/lyft/envoy/blob/master/ci/do_ci.sh#L18

After that runs there will be a lot of HTML files including coverage.html which show you exactly what is covered.