As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

/assign @alyssawilk

Can you adjust the coverage threshold value for the filter, now the untested code was removed?

/wait-any

not just the filter but overall coverage and close off that bug, if this is sufficient

/wait

CC @envoyproxy/coverage-shephards: FYI only for changes made to (test/per_file_coverage.sh).
envoyproxy/coverage-shephards assignee is @RyanTheOptimist

It seems like this leaves us teetering on the edge of 96.1. Why does source/extensions/filters/http/rate_limit_quota have such an incredibly low coverage limit? Looking at coverage output, it seems like it would be pretty trivial to write a unit test for XRateLimitHeaderUtils::convertRateLimitUnit and RateLimitFilterConfig::createRouteSpecificFilterConfigTyped in order to get the coverage for this directory up to normal.

https://storage.googleapis.com/envoy-pr/9cb3b98/coverage/source/extensions/filters/http/ratelimit/ratelimit_headers.cc.gcov.html
https://storage.googleapis.com/envoy-pr/9cb3b98/coverage/source/extensions/filters/http/ratelimit/config.cc.gcov.html

It seems like this leaves us teetering on the edge of 96.1. Why does source/extensions/filters/http/rate_limit_quota have such an incredibly low coverage limit? Looking at coverage output, it seems like it would be pretty trivial to write a unit test for XRateLimitHeaderUtils::convertRateLimitUnit and RateLimitFilterConfig::createRouteSpecificFilterConfigTyped in order to get the coverage for this directory up to normal.

https://storage.googleapis.com/envoy-pr/9cb3b98/coverage/source/extensions/filters/http/ratelimit/ratelimit_headers.cc.gcov.html https://storage.googleapis.com/envoy-pr/9cb3b98/coverage/source/extensions/filters/http/ratelimit/config.cc.gcov.html

rate_limit_qutoa is a WIP that only partially implemented/merged due to large amount of code it requires and the spirit of small CLs. But these already merged code is needed so that I can continue the development as Github doesn't really support chained CLs. Since it is unfinished work, it has some boilerplate code required by compilation and for my future PRs but not tested.

The links you mentioned are for rateLimit which has nothing to do with rate_limit_quota here. The coverage for rate_limit_quota was at 86.2%. I added some more so it should be over 90% (Also some of line are coverage tool's false positive )

For the WIP, I feel it might be more worthy to continue the next phase development so that the code will be covered in next PRs.
Please let me know your thoughts

rate_limit_qutoa is a WIP that only partially implemented/merged due to large amount of code it requires and the spirit of small CLs. But these already merged code is needed so that I can continue the development as Github doesn't really support chained CLs. Since it is unfinished work, it has some boilerplate code required by compilation and for my future PRs but not tested.

The links you mentioned are for rateLimit which has nothing to do with rate_limit_quota here. The coverage for rate_limit_quota was at 86.2%. I added some more so it should be over 90% (Also some of line are coverage tool's false positive )

Oh, sheesh. Yes, you're right. Is the correct coverage for this PR is here? That seems to show 86.2% not 90%. Is that what you expect? In any case, can you update test/per_file_coverage.sh to the correct currently value (either 86.2% or 90% or whatever it is)?

For the WIP, I feel it might be more worthy to continue the next phase development so that the code will be covered in next PRs. Please let me know your thoughts

If we have a directory that is below the limit, we can easily lower the threshold for that directory in test/per_file_coverage.sh, as we've done. But the problem with that is that this counts against the global project coverage limits. So by keeping rate_limit_quota below the limit, we're making it more likely that some other PR might cause us to drop down below the global limit. This causes pain for the entire project. Can we remove the untested methods from the tree until we're ready to use them (in follow up PRs)?

Hi @RyanTheOptimist, I updated the code to increase test coverage, rate_limit_quota is now at 95% https://storage.googleapis.com/envoy-pr/761a7cc/coverage/index.html. I updated the limit to 94% though in case there are any flaky issues and will adjust after future PRs are landed.
The only lines left are dead code caused by exhaustive switch statement. I will think about updating this switch statement in next PR.

PTAL. Thanks!

/retest

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

The failed coverage is caused by Code coverage for source/common/common/posix is lower than limit of 94.5 (92.7), which was adjusted in #24458. Thus, I think this is not ready to be bumped? I reverted it in this PR.

I figured that in order to unblock the presubmit test faster, I probably should have a separate PR to fix posix test coverage rather than include it in this PR which has many other things already. So I send out #24463

/assign @yanavlasov

Sorry I didn't remove the request, I just click a request review from Ryan button and somehow Github removed it :(

Hm. Even though #24463 has landed, this PR still includes a change to source/common/common/posix. Is that expected?

Hm. Even though #24463 has landed, this PR still includes a change to source/common/common/posix. Is that expected?

Yea, The change in this PR for posix is exactly same as #24463. It is gone now after I merge with main.

Unfortunately I'm not quite sure the coverage results worked:
Checking per-extension coverage
/source/test/run_envoy_bazel_coverage.sh: line 121: ./test/per_file_coverage.sh: Permission denied
Per-extension coverage passed.
I don't think it was your PR but I'm not sure if the number here is correct as CI functionally didn't run in at least one pass. I'm fixing in #24529 and we'll see how it goes

for ref the permissions were changed in 614d03a

thankfully all good, coverage runs restored cleanly :-)