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

ci: integrate the codecov #251

Merged
merged 1 commit into from
Nov 1, 2023
Merged

ci: integrate the codecov #251

merged 1 commit into from
Nov 1, 2023

Conversation

qclc
Copy link
Collaborator

@qclc qclc commented Oct 25, 2023

Add code coverage detection and ensure that 60%or higher of new code is covered with unit tests.

Currently we only focus on the coverage in the ./pkg directory, and will ignore the following packages or files in this directory:

  • "pkg/apis"
  • "pkg/client"
  • "*_test.go"
  • "fake_*.go"

@SOF3
Copy link
Member

SOF3 commented Oct 25, 2023

Does the coverage data here include packages without any test files? See golang/go#24570

I did a workaround in kelemetry because of this issue kubewharf/kelemetry@2647219

@qclc
Copy link
Collaborator Author

qclc commented Oct 26, 2023

Does the coverage data here include packages without any test files? See golang/go#24570

I noticed this problem and it seems to be solved by adding the -coverpkg=./pkg/... parameter to the go test command.

When I experimented in my repository, it seemed that the coverage would include all packages in the ./pkg directory, even if there was no test file: qclc#2 (comment)

According to experimental results, the old test report will not cover packages that do not contain single tests(coverage of ./pkg is ~ 37%), but the new test report will cover all packages(coverage of ./pkg is ~ 24%), even if there are no unit tests in them.

Actually, I'm not sure why the above solution works, but it seems to solve the problem😂. Can you please help confirm if this solution meets expectations? @SOF3

@SOF3
Copy link
Member

SOF3 commented Oct 26, 2023

@qclc It seems the packages were included simply because your PR had diff in those packages. Maybe try with another commit that does not involve those packages? Would the total coverage decrease again?

@qclc
Copy link
Collaborator Author

qclc commented Oct 27, 2023

@qclc It seems the packages were included simply because your PR had diff in those packages. Maybe try with another commit that does not involve those packages? Would the total coverage decrease again?

@SOF3 I experimented again and only modified the package containing the unit test. The coverage remained unchanged, which was also ~24% : https://app.codecov.io/gh/qclc/kubeadmiral/pull/4 . I also ran go test locally and observed the test report. After adding the -coverpkg=./pkg/... parameter, the test report will include all the packages in the ./pkg directory. Otherwise, it won't. It seems that the test report after adding the -coverpkg=./pkg/... parameter is fully covered.

@qclc qclc force-pushed the add-codecov branch 3 times, most recently from 8b0cd36 to b9cf506 Compare October 27, 2023 05:28
@codecov-commenter
Copy link

Welcome to Codecov 🎉

Once merged to your default branch, Codecov will compare your coverage reports and display the results in this comment.

Thanks for integrating Codecov - We've got you covered ☂️

@qclc
Copy link
Collaborator Author

qclc commented Oct 27, 2023

Sorry to miss this comment on issue (golang/go#24570 (comment)) : using -coverpkg does not actually help if the package is never called in any test at all. -coverpkg is a cover-up that allows packages from a different package to provide coverage for this package.

The end result still requires some additional work to ensure coverage is reported for every package, as @SOF3 did in repository kelemetry. Please take a review.

.codecov.yml Show resolved Hide resolved
@gary-lgy gary-lgy merged commit 3c336c6 into kubewharf:main Nov 1, 2023
5 checks passed
miraclejzd pushed a commit to miraclejzd/kubeadmiral that referenced this pull request Jan 3, 2024
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.

4 participants