Skip to content

ci: Fix flaky coverage limit#24320

Merged
phlax merged 1 commit intoenvoyproxy:mainfrom
phlax:ci-coverage-flake
Dec 2, 2022
Merged

ci: Fix flaky coverage limit#24320
phlax merged 1 commit intoenvoyproxy:mainfrom
phlax:ci-coverage-flake

Conversation

@phlax
Copy link
Copy Markdown
Member

@phlax phlax commented Dec 2, 2022

this can currently fail in builds that dont change anything

Signed-off-by: Ryan Northey ryan@synca.io

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Ryan Northey <ryan@synca.io>
Copy link
Copy Markdown
Contributor

@jpsim jpsim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to unblock PRs but if we're at 96.0 already this just kicks the can very slightly down the road. Shouldn't we give more of a buffer like 95.0?

@phlax
Copy link
Copy Markdown
Member Author

phlax commented Dec 2, 2022

Shouldn't we give more of a buffer like 95.0

probably yes - altho there is an intention of constantly pushing it up - not sure what leeway we should add

cc @alyssawilk

@jpsim
Copy link
Copy Markdown
Contributor

jpsim commented Dec 2, 2022

I think Alyssa's out today so let's unblock at 96.0 and resume the discussion about how much buffer we should have next week.

@phlax phlax merged commit 02b2ff1 into envoyproxy:main Dec 2, 2022
jpsim added a commit to alyssawilk/envoy that referenced this pull request Dec 2, 2022
* origin/main:
  ci: Fix flaky coverage limit (envoyproxy#24320)
  ci: Fix change detection (part 2) (envoyproxy#24264)

Signed-off-by: JP Simard <jp@jpsim.com>
jpsim added a commit that referenced this pull request Dec 2, 2022
…/docs/sphinx-5.3.0

* origin/main: (50 commits)
  ci: Bump mobile build images (#24317)
  ci: update llvm on bazel CI (#24296)
  ci: Fix flaky coverage limit (#24320)
  ci: Fix change detection (part 2) (#24264)
  Start slow start window on first successful HC, make slow start re-enterable (#23946)
  contrib-sipproxy: rework sipproxy stats (#24165)
  build(deps): bump github/codeql-action from 2.1.32 to 2.1.35 (#24292)
  build(deps): bump node from `c59fb39` to `80844b6` in /examples/ext_authz/auth/http-service (#24274)
  build(deps): bump actions/upload-artifact from 2 to 3 (#24121)
  build(deps): bump mysql from `96439dd` to `66efaaa` in /examples/mysql (#24273)
  build(deps): bump grpcio from 1.50.0 to 1.51.1 in /examples/grpc-bridge/client (#24272)
  Allow mobile/library/common/jni/ to be built on non-Android platforms. (#24299)
  generic proxy: added drain support to generic proxy to doing graceful closes on connections when possible (#24220)
  Reduce Route/VirtualHost memory utilization by avoiding RateLimitPolicy instances when not needed (#24243)
  Reduce VirtualHost memory utilization by avoiding CatchAllVirtualCluster when not needed (#24182)
  Fix mobile/tools/check_format.sh to re-apply #2698 (#24300)
  upstream: don't require `source_address` in `upstream_bind_config` (#24250)
  Quiche roll 20221201150327 (#24290)
  Cleanup threading/ownership semantics of PlatformBridgeCertValidator (#2713)
  Add unit tests of the PlatformBridgeCertValidator (#2704)
  ...

Signed-off-by: JP Simard <jp@jpsim.com>
jpsim added a commit that referenced this pull request Dec 2, 2022
…/docs/pygments-2.13.0

* origin/main: (25 commits)
  ci: Bump mobile build images (#24317)
  ci: update llvm on bazel CI (#24296)
  ci: Fix flaky coverage limit (#24320)
  ci: Fix change detection (part 2) (#24264)
  Start slow start window on first successful HC, make slow start re-enterable (#23946)
  contrib-sipproxy: rework sipproxy stats (#24165)
  build(deps): bump github/codeql-action from 2.1.32 to 2.1.35 (#24292)
  build(deps): bump node from `c59fb39` to `80844b6` in /examples/ext_authz/auth/http-service (#24274)
  build(deps): bump actions/upload-artifact from 2 to 3 (#24121)
  build(deps): bump mysql from `96439dd` to `66efaaa` in /examples/mysql (#24273)
  build(deps): bump grpcio from 1.50.0 to 1.51.1 in /examples/grpc-bridge/client (#24272)
  Allow mobile/library/common/jni/ to be built on non-Android platforms. (#24299)
  generic proxy: added drain support to generic proxy to doing graceful closes on connections when possible (#24220)
  Reduce Route/VirtualHost memory utilization by avoiding RateLimitPolicy instances when not needed (#24243)
  Reduce VirtualHost memory utilization by avoiding CatchAllVirtualCluster when not needed (#24182)
  Fix mobile/tools/check_format.sh to re-apply #2698 (#24300)
  upstream: don't require `source_address` in `upstream_bind_config` (#24250)
  Quiche roll 20221201150327 (#24290)
  Cleanup threading/ownership semantics of PlatformBridgeCertValidator (#2713)
  Add unit tests of the PlatformBridgeCertValidator (#2704)
  ...

Signed-off-by: JP Simard <jp@jpsim.com>
@alyssawilk
Copy link
Copy Markdown
Contributor

if we're seeing flakes I think we improve coverage not drop the bar 1% :-(

@jpsim
Copy link
Copy Markdown
Contributor

jpsim commented Dec 5, 2022

if we're seeing flakes I think we improve coverage not drop the bar 1% :-(

I don't think it was flaky, I think there were two PRs which individually were over 96.1 that when they were both merged to main went under 96.1.

@alyssawilk
Copy link
Copy Markdown
Contributor

yeah I wasn't responding to this PR, which was a fix to a deterministic failure, I was responding to @jmarantz suggesting we drop a full point.
Do we know which 2 PRs they were so we can ask for more unit testing?

@jpsim
Copy link
Copy Markdown
Contributor

jpsim commented Dec 5, 2022

I was responding to @jmarantz suggesting we drop a full point.

That was me in #24320 (review)

Do we know which 2 PRs they were so we can ask for more unit testing?

I don't know which PRs reduced code coverage, but a motivated person could bisect main with code coverage with higher accuracy to see what changes increased vs decreased coverage.

Personally, I don't think it's realistic to say that no change can lower the code coverage amount, but that's my 2 cents.

@alyssawilk
Copy link
Copy Markdown
Contributor

oh we regularly allow tweaks due to refactors that say remove code (and thus lower coverage)
what I don't like about this is it lowers coverage for each individual directory rather than exempting whatever was causing the underlying problem, which means we permanently lower the bar for the whole project. I'll try to throw together a change I like better :-)

@alyssawilk
Copy link
Copy Markdown
Contributor

(to be more clear this was 110% a reasonable short term fix, I'll take on what I think is a better long term change)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants