Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

coverage: adding tests, bumping up #24351

Merged
merged 2 commits into from
Dec 5, 2022
Merged

coverage: adding tests, bumping up #24351

merged 2 commits into from
Dec 5, 2022

Conversation

alyssawilk
Copy link
Contributor

@alyssawilk alyssawilk commented Dec 5, 2022 •
edited
Loading

Partt of #24353

@alyssawilk alyssawilk force-pushed the coverage branch 2 times, most recently from 5810892 to 2855611 Compare December 5, 2022 16:09
@repokitteh-read-only
Copy link

CC @envoyproxy/mobile-maintainers: FYI only for changes made to (mobile/).
envoyproxy/mobile-maintainers assignee is @jpsim

🐱

Caused by: #24351 was synchronize by alyssawilk.

see: more, trace.

jpsim
jpsim previously approved these changes Dec 5, 2022
View reviewed changes
@alyssawilk alyssawilk enabled auto-merge (squash) December 5, 2022 16:52
@tyxia tyxia mentioned this pull request Dec 5, 2022
Closed
@tyxia
Copy link
Member

tyxia commented Dec 5, 2022 •
edited
Loading

Looking at the coverage report here, the problem is Code coverage for source/extensions/clusters/common is lower than limit of 96.6 (90.3).

The main coverage is good now (Overall coverage rate: lines......: 96.1% (121370 of 126334 lines) functions..: 76.1% (25767 of 33838 functions) Code coverage 96.1 is good and higher than limit of 96.0)

@alyssawilk
Copy link
Contributor Author

@tyxia I know how to read coverage reports, thanks.
After this goes in you're welcome to submit a PR to revert the lower limit, but I am not including a global limit change in this PR in case it needs to be rolled back. Easier to land solo

jpsim
jpsim previously approved these changes Dec 5, 2022
View reviewed changes
@alyssawilk
partial fix
23f0c83 
Signed-off-by: Alyssa Wilk <[email protected]>
@repokitteh-read-only
Copy link

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

🐱

Caused by: #24351 was synchronize by alyssawilk.

see: more, trace.

RyanTheOptimist
RyanTheOptimist approved these changes Dec 5, 2022
View reviewed changes
source/extensions/clusters/common/logical_host.h
// this.
/* TODO: the health checker only gets the first address in the list and
* will not walk the full happy eyeballs list. We should eventually fix
* this. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this change un-confuse the coverage checker?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the goal was to test that but it does not :-(
https://storage.googleapis.com/envoy-pr/e78a246/coverage/source/extensions/clusters/common/logical_host.h.gcov.html

@alyssawilk alyssawilk merged commit f9da82c into envoyproxy:main Dec 5, 2022
@alyssawilk alyssawilk deleted the coverage branch December 6, 2022 13:27
jpsim added a commit that referenced this pull request Dec 6, 2022
@jpsim
Merge remote-tracking branch 'origin/main' into mobile-update-bazelw-…
5c71677 
…to-pull-bazelisk-1.15.0

* origin/main:
  coverage: fixing a TODO (#24357)
  ci: Fix change detection (part 3) (#24347)
  wasm: Improve the coverage report for Wasm (#23055)
  coverage: adding tests, bumping up (#24351)

Signed-off-by: JP Simard <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpsim jpsim jpsim approved these changes

@RyanTheOptimist RyanTheOptimist RyanTheOptimist approved these changes

@Augustyniak Augustyniak Awaiting requested review from Augustyniak

@abeyad abeyad Awaiting requested review from abeyad

Assignees

@RyanTheOptimist RyanTheOptimist

Labels
mobile
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alyssawilk @tyxia @jpsim @RyanTheOptimist