etcd-io
Worrying trend with test coverage
What would you like to be added?
Code coverage value doesn't imply muchy, however we can infer some information from its trends.
Within last 12 months the etcd code coverage fell by 6%.
Link: https://app.codecov.io/gh/etcd-io/etcd/tree/main?search=&displayType=tree&trend=12%20months
I encourage all contributors, reviewers and approvers to ensure that we not continue this trend. As described in https://github.com/etcd-io/etcd/blob/main/Documentation/contributor-guide/features.md all features should have a proper testing. We should ensure that all PRs, both features and bugfixes, come with basic set of unit test that ensure that feature/bugfix does what it claims to be.
If you can please recall any PRs that you have reviewed and would benefit from more testing. We should ensure there are a proper followups to add those tests.
cc @jmhbnz @ahrtr @wenjiaswe
Why is this needed?
Testing is the foundation of project maintainability. Without them, we cannot add any features or fix any bugs without knowing that something else will break
I recall we have a coverage workflow running for each PR, but got removed for some reasons I don't remember anymore. Can we start with adding it back?
I'm unsure if the trend would be visible on a PR basis, and I don't like per-PR coverage information because it encourages writing low-quality tests just for coverage. Instead, we should identify areas where high coverage is crucial, and encourage contributors to write meaningful tests when and where they're needed.
Coverage page also includes information about subdirectories and I think it's ok.
The low coverage of etcdctl and etcdutl makes sense as they are just command line tools that are mostly tested by e2e tests that are not included in the coverage
and I don't like per-PR coverage information because it encourages writing low-quality tests just for coverage
In my opinion, this is a viewpoint with obvious personal subjective bias. Adding a coverage workflow is a ver feasible way to keep the test coverage. With respect to "low quality tests", it's the scope of review.
If we are concerned pr's are not hitting requirements for testing and coverage is reducing I completely agree with @ahrtr that the way to address this would be to surface coverage information directly within pull requests.
It's incumbent on reviewers of pull requests to provide feedback on the quality of tests written and ensure tests add value before hitting merge.
Thanks @serathius for opening this issue! 6% decrease is not a good trend, let's avoid that.
+1 on adding back per PR test coverage. While itself doesn't guarantee test quality, it definitely help reviewing and encouraging all contributors to pay closer attention to test coverage. @henrybear327 Thank you for taking this!
And we should definitely continue paying attention to identify areas where high coverage is crucial.
We actually have codecov running, but we didn't set it to comment on the PR. Testing it now https://github.com/etcd-io/etcd/pull/18143
Update: works! https://github.com/etcd-io/etcd/pull/18143#issuecomment-2155156645
Small notes on test coverage variation across runs https://github.com/etcd-io/etcd/pull/18143#issuecomment-2162182830
I will unassign myself since the PR to improve code coverage is completed, and leave this issue for tracking further test coverage improvement!
As found in https://github.com/etcd-io/etcd/pull/18767#issuecomment-2432435819, the Codecov integration/bot has been adding the following warning to the comment it's posting in the pull requests:
⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.
At the bottom, it says:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
It sounds like a GitHub admin would need to install the application. Would this be a reasonable ask (considering the background on rejection of requests they get)? Or should we just keep it as is and ignore the warning?
As found in #18767 (comment), the Codecov integration/bot has been adding the following warning to the comment it's posting in the pull requests:
⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.
At the bottom, it says:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
It sounds like a GitHub admin would need to install the application. Would this be a reasonable ask (considering the background on rejection of requests they get)? Or should we just keep it as is and ignore the warning?
I believe we need to raise a k/org issue similar to https://github.com/kubernetes/org/issues/4439. These have been completed historically so should be ok.
I believe we need to raise a
k/orgissue similar to kubernetes/org#4439. These have been completed historically so should be ok.
Thanks, I'll go ahead and do it.
/assign
Opened https://github.com/kubernetes/org/issues/5231 to request the installation.
/unassign
CodeCov is working again. Link to https://github.com/etcd-io/etcd/pull/18964
Discussed during sig-etcd triage meeting. We've now enabled full codecov integration and reviewing updated 12 month trend coverage seems much more stable with only a 1% reduction over the year: https://app.codecov.io/gh/etcd-io/etcd/tree/main?displayType=tree&trend=12%20months
@serathius can we close this now?
Discussed during sig-etcd triage meeting, closing as coverage is now stable.