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.

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

Update codecov config to support commenting in PR #18143

Merged
merged 1 commit into from
Jun 12, 2024

Conversation

henrybear327
Copy link
Contributor

@henrybear327 henrybear327 commented Jun 7, 2024

  • add missing directory fixing go.etcd.io/etcd/etcdutl/v3/::etcdutl/
  • update codecov config to support commenting in PR

Reference:

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

@henrybear327 henrybear327 force-pushed the ci/add_coverage_tool branch 7 times, most recently from 8a004fe to e02b101 Compare June 7, 2024 16:04
@k8s-ci-robot k8s-ci-robot added the github_actions Pull requests that update GitHub Actions code label Jun 7, 2024
@henrybear327
Copy link
Contributor Author

henrybear327 commented Jun 7, 2024

@serathius I was checking other project's codecov configuration file and the official documentation, we should consider putting codecov token in Github Secret (maybe we need to regenerate a new token first, but we need to have access to the codecov account, which I don't know who to ping).

I can't do this on my own since I don't have Github Repo Setting access, so this part I might need your help to look into this!

Thanks

@codecov-commenter
Copy link

codecov-commenter commented Jun 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.83%. Comparing base (5790774) to head (6615b5e).

Current head 6615b5e differs from pull request most recent head 1579c57

Please upload reports for the commit 1579c57 to get more accurate results.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

see 22 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #18143      +/-   ##
==========================================
- Coverage   69.08%   68.83%   -0.25%     
==========================================
  Files         416      416              
  Lines       35127    35127              
==========================================
- Hits        24266    24181      -85     
- Misses       9475     9544      +69     
- Partials     1386     1402      +16     

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5790774...1579c57. Read the comment docs.

@jmhbnz
Copy link
Member

jmhbnz commented Jun 8, 2024

/retest

@henrybear327 henrybear327 force-pushed the ci/add_coverage_tool branch 3 times, most recently from db3968a to 973fd14 Compare June 8, 2024 08:55
Copy link
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

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

LGTM - Thanks @henrybear327, the preview above in this pr is looking good.

@henrybear327 henrybear327 force-pushed the ci/add_coverage_tool branch 2 times, most recently from 3631092 to 7d424f5 Compare June 9, 2024 12:41
@henrybear327
Copy link
Contributor Author

LGTM - Thanks @henrybear327, the preview above in this pr is looking good.

Thanks for reviewing @jmhbnz! :)

Currently, the Codecov comment is set to update the existing code coverage comment when a new commit is pushed (default behavior), instead of deleting the existing comment and creating a new one.

Using the default behavior means that the code coverage information will likely be at the top of the comment, so PRs with long discussion threads will require some scrolling to view the code coverage information. Deleting the old one and adding a new comment with each commit will generate more email notifications though.

In order to avoid email spamming, I think it's better that we use the default behavior.

What do you think? :)

@henrybear327 henrybear327 requested a review from jmhbnz June 9, 2024 17:06
@henrybear327
Copy link
Contributor Author

/retest

/cc @serathius @ahrtr @wenjiaswe

@ahrtr
Copy link
Member

ahrtr commented Jun 9, 2024

see 20 files with indirect coverage changes

Thanks @henrybear327 for the PR. But I am curious why the coverage decreased for many go files even this PR doesn't change any go source code?

@jmhbnz
Copy link
Member

jmhbnz commented Jun 9, 2024

In order to avoid email spamming, I think it's better that we use the default behavior.

Agreed. Let's keep it to one coverage comment per pr that is always up to date.

@henrybear327
Copy link
Contributor Author

see 20 files with indirect coverage changes

Thanks @henrybear327 for the PR. But I am curious why the coverage decreased for many go files even this PR doesn't change any go source code?

@ahrtr I suspect it's due to rebasing. I will look into this!

@wenjiaswe
Copy link
Contributor

/lgtm thank you @henrybear327 !

@henrybear327
Copy link
Contributor Author

henrybear327 commented Jun 12, 2024

see 20 files with indirect coverage changes

Thanks @henrybear327 for the PR. But I am curious why the coverage decreased for many go files even this PR doesn't change any go source code?

@ahrtr I suspect it's due to rebasing. I will look into this!

This is due to indirect changes [1].

I suspect that during test coverage execution, we have some of the tests written in a way that is not deterministic. Because when I check the "indirect changes" of PRs from this list, hit/miss differences come from things like select cases (e.g. error handling, timeout, etc.), cache eviction, etc.

Screenshot 2024-06-12 at 8 07 18 AM Screenshot 2024-06-12 at 8 07 00 AM

So our test coverage percentage will have a small variation across runs, as you can see here.

Do you think this makes sense @ahrtr?

Reference:
[1] https://docs.codecov.com/docs/unexpected-coverage-changes#reasons-for-indirect-changes

@ahrtr
Copy link
Member

ahrtr commented Jun 12, 2024

[1] https://docs.codecov.com/docs/unexpected-coverage-changes#reasons-for-indirect-changes

Thanks for the clarification, which basically makes sense. Can you pls add a comment to include the link for reference?

@henrybear327
Copy link
Contributor Author

[1] https://docs.codecov.com/docs/unexpected-coverage-changes#reasons-for-indirect-changes

Thanks for the clarification, which basically makes sense. Can you pls add a comment to include the link for reference?

Thanks @ahrtr! I have

  • updated the PR description
  • updated the commit message
  • added link to the discussion comment in the issue

Not sure if this is what you expected :)

@ahrtr
Copy link
Member

ahrtr commented Jun 12, 2024

Sorry for the confusion, I was saying to add a comment into the repo, e.g. above the cov_pass

function cov_pass {

@henrybear327
Copy link
Contributor Author

Add missing directory fixing go.etcd.io/etcd/etcdutl/v3/::etcdutl/

Note: we have some of the tests written in a way that is
non-deterministic across runs, thus, even when there is no code changes
there might still have slight variation for test coverage [2]

Reference:
[1] etcd-io#18131
[2] https://docs.codecov.com/docs/unexpected-coverage-changes#reasons-for-indirect-changes

Signed-off-by: Chun-Hung Tseng <henrybear327@gmail.com>
Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @henrybear327

@ahrtr ahrtr merged commit 5c31909 into etcd-io:main Jun 12, 2024
49 checks passed
@henrybear327 henrybear327 deleted the ci/add_coverage_tool branch June 12, 2024 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github_actions Pull requests that update GitHub Actions code size/S
Development

Successfully merging this pull request may close these issues.

6 participants