source: more GCOVR removal#19707
Conversation
alyssawilk
force-pushed
the
gcovr_nextnext
branch
3 times, most recently
from
9c379d0 to
7bc57f8
Compare
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
alyssawilk
force-pushed
the
gcovr_nextnext
branch
from
7bc57f8 to
e05953c
Compare
snowp
left a comment
snowp
left a comment
There was a problem hiding this comment.
Looks good to me, a few questions
| Network::DnsLookupFamily | ||
| getDnsLookupFamilyFromEnum(envoy::config::cluster::v3::Cluster::DnsLookupFamily family) { | ||
| switch (family) { | ||
| PANIC_ON_PROTO_ENUM_SENTINEL_VALUES; |
There was a problem hiding this comment.
For my understanding, when do we need this? Why don't we specify this for the enum in matchers.cc for example?
There was a problem hiding this comment.
More detail in linked issue, but proto creates sentinel values so it can test to see if you go beyond MIN and MAX, and if you want to do an exhaustive switch you need to handle those.
If you don't do exhaustive switch you end up introducing bugs when you add new fields, like the fact this switch was missing handling for ALL and would have panic'd if that config path was hit in production :-(
| NOT_REACHED_GCOVR_EXCL_LINE; | ||
| PANIC("unexpected"); | ||
| } | ||
| NOT_REACHED_GCOVR_EXCL_LINE; | ||
| PANIC("unexpected"); |
There was a problem hiding this comment.
Fallthrough here as elsewhere?
| "source/common/protobuf:94.8" | ||
| "source/common/quic:91.8" | ||
| "source/common/router:96.5" | ||
| "source/common/router:96.3" |
There was a problem hiding this comment.
Is there no way to maintain coverage of this new code? Is it going down because of lines of code we need in order to satisfy the compiler but that won't actually get hit?
There was a problem hiding this comment.
unfortunately the only way to test PANIC is with death test, which doesn't help coverage.
there's a follow-up issue to move these panics over sensible defaults and actual error handling and that should come with unit tests. As this shows, we have WAY too many PANICs in the data plane, due to this macro being misleading
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
CI sorted. |
Risk Level: medium Testing: existing tests Docs Changes: n/a Release Notes: n/a part of envoyproxy#19172 Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: Josh Perry <josh.perry@mx.com>
2 participants
Risk Level: medium
Testing: existing tests
Docs Changes: n/a
Release Notes: n/a
part of #19172