[no-relnote] Enable Coveralls for code coverage #958
Conversation
There was a problem hiding this comment.
PR Overview
This PR enables CodeCov integration to track Go unit test coverage. It adds a new CodeCov upload step under the "test" job and makes minor adjustments to the GitHub Actions workflow configuration.
- Added CodeCov action in the "test" job.
- Configured environment variables for Go version extraction.
- Maintained setup consistency across testing and build steps.
Reviewed Changes
| File | Description |
|---|---|
| .github/workflows/golang.yaml | Added CodeCov step for coverage reporting and adjusted version setup |
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
.github/workflows/golang.yaml:96
- The variable substitution operator differs from the one in the test job (':='). Consider using the consistent syntax ':=' for extracting GOLANG_VERSION unless the differing behavior is intentional.
echo "GOLANG_VERSION=${GOLANG_VERSION##GOLANG_VERSION ?= }" >> $GITHUB_ENV
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
|
umm let's leave this for later on, we might need to move Go actions to hosted runners if we want to pass the secret during PR |
.github/workflows/golang.yaml
Outdated
| - name: Install Go | ||
| uses: actions/setup-go@v5 | ||
| with: | ||
| go-version: ${{ env.GOLANG_VERSION }} | ||
|
|
||
| - run: make test |
There was a problem hiding this comment.
Should we change this to make coverage instead?
There was a problem hiding this comment.
Well, it actually makes sense from the perspective of a Makefile file functionality; make coverage would actually generate an output artifact. So I am not against it, I'll leave it as your call
There was a problem hiding this comment.
Don't we need the coverage.out artifact?
There was a problem hiding this comment.
Well, it actually makes sense from the perspective of a Makefile file functionality; make coverage would actually generate an output artifact. So I am not against it, I'll leave it as your call
Sorry, I don't understand what you're saying here. We have two targets:
make testmake coverage
The first generates coverage.out and the second filters out coverage related to mocks to generate a .no-mocks file.
There was a problem hiding this comment.
Sorry I was not aware of the make coverage but for codecov we need the output file from make test
.github/workflows/golang.yaml
Outdated
| with: | ||
| token: ${{ secrets.CODECOV_TOKEN }} | ||
| files: ./coverage.out | ||
| fail_ci_if_error: true |
There was a problem hiding this comment.
Do we want to block CI on an external system failing?
That's not true. Secrets are not available on PRs from forks, but should be available from the |
ture, I forgot about copy-pr-bot I was thinking in the old way o passing secrets. still will retake this PR after we cut the release |
|
/ok to test |
ArangoGutierrez
force-pushed
the
codecov
branch
2 times, most recently
from
f8b888e to
ce27020
Compare
|
Here's a look of how we would be able to visualize the results https://app.codecov.io/gh/NVIDIA/nvidia-container-toolkit
|
|
@tariq1890 if you like this, we could replace the |
|
@ArangoGutierrez as a general question, how does |
It is free for OSS repos. |
|
@elezar, any thoughts on this? |
Please confirm whether this can be run from a PR through the copy bot? You also have not answered by question as to how this compares to |
Here's a detailed comparison Comparison: Coveralls vs. Codecov (GitHub Actions – Code Coverage)
Summary:
References: |
|
@elezar PTAL |
ArangoGutierrez
changed the title
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces a code coverage reporting step into the CI pipeline by uploading coverage reports to Codecov.
- Added a new Codecov upload step under the "Unit test" job.
- Introduced minor formatting adjustments and a change in the parameter expansion operator in the build job.
Comments suppressed due to low confidence (1)
.github/workflows/golang.yaml:96
- The parameter expansion operator in the build job has changed from ':=' in the test job to '?='. This inconsistency might result in unintended behavior. Consider standardizing the operator across both jobs.
echo "GOLANG_VERSION=${GOLANG_VERSION##GOLANG_VERSION ?= }" >> $GITHUB_ENV
Makefile
Outdated
| @@ -128,6 +146,12 @@ coverage: test | |||
| cat $(COVERAGE_FILE) | grep -v "_mock.go" > $(COVERAGE_FILE).no-mocks | |||
| go tool cover -func=$(COVERAGE_FILE).no-mocks | |||
|
|
|||
| cov-report: gcov2lcov test ## Build test coverage report in lcov format | |||
| $(GCOV2LCOV) -infile coverage.out -outfile lcov.info | |||
There was a problem hiding this comment.
nit: We should add lcov.info to the .gitignore
There was a problem hiding this comment.
Pull Request Overview
Adds Coveralls integration for repo-wide code coverage tracking by extending Makefile targets and CI workflow.
- Updated MAKE_TARGETS to include
cov-reportandgcov2lcov. - Defined a
go-install-toolhelper in the Makefile to installgcov2lcovand convertcoverage.outintolcov.info. - Modified GitHub Actions to run
make cov-reportand uploadlcov.infoto Coveralls.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Makefile | Added cov-report, gcov2lcov targets, and go-install-tool; updated MAKE_TARGETS. |
| .github/workflows/golang.yaml | Added steps to generate LCov report and upload to Coveralls; replaced direct make test with make cov-report. |
Comments suppressed due to low confidence (1)
.github/workflows/golang.yaml:95
- The prefix stripping here uses
?=, but earlier steps strip:=. This mismatch may prevent the Go version from being set correctly; align this to${GOLANG_VERSION##GOLANG_VERSION := }.
echo "GOLANG_VERSION=${GOLANG_VERSION##GOLANG_VERSION ?= }" >> $GITHUB_ENV
.github/workflows/golang.yaml
Outdated
| - name: Generate coverage report | ||
| run: make cov-report | ||
|
|
||
| - name: Upload to Coveralls | ||
| uses: coverallsapp/github-action@v2 | ||
| with: | ||
| github-token: ${{ secrets.GITHUB_TOKEN }} | ||
| path-to-lcov: lcov.info |
There was a problem hiding this comment.
Is the conversion required? According to https://github.com/coverallsapp/coverage-reporter?tab=readme-ov-file#built-in-support there is no built-in support for golang. Was that not working as expected?
There was a problem hiding this comment.
You were right, we can simply upload the coverage.out file.
ArangoGutierrez
force-pushed
the
codecov
branch
4 times, most recently
from
ae151c8 to
46494a9
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds Coveralls integration to track the repository's code coverage.
- Introduces tool configuration in the Makefile for handling coverage reports.
- Adds new workflow steps in the GitHub Actions configuration to generate and upload coverage reports to Coveralls.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Makefile | Adds tool configuration for gcov2lcov and version pinning. |
| .github/workflows/golang.yaml | Adds steps to generate coverage reports and upload to Coveralls; note potential variable substitution issue. |
Comments suppressed due to low confidence (1)
.github/workflows/golang.yaml:95
- The variable manipulation appears to contain a typo ('?=' instead of ':='), which may lead to unexpected behavior. Please update it to match the syntax used in the test job.
echo "GOLANG_VERSION=${GOLANG_VERSION##GOLANG_VERSION ?= }" >> $GITHUB_ENV
.github/workflows/golang.yaml
Outdated
| uses: coverallsapp/github-action@v2 | ||
| with: | ||
| github-token: ${{ secrets.GITHUB_TOKEN }} | ||
| path-to-lcov: coverage.out |
There was a problem hiding this comment.
Should this just be file:? https://github.com/coverallsapp/github-action?tab=readme-ov-file#inputs
There was a problem hiding this comment.
set to file
There was a problem hiding this comment.
We may want to make a similar change to the GPU Operator and k8s-operator-libs. cc @tariq1890
Makefile
Outdated
| # Tools | ||
| TOOLSDIR=$(CURDIR)/bin | ||
| GCOV2LCOV ?= $(TOOLSDIR)/gcov2lcov | ||
| GCOV2LCOV_VERSION ?= v1.1.1 | ||
|
|
There was a problem hiding this comment.
Let's also update the makefile as follows:
diff --git a/Makefile b/Makefile
index ac491762..86433fa2 100644
--- a/Makefile
+++ b/Makefile
@@ -122,11 +122,11 @@ licenses:
COVERAGE_FILE := coverage.out
test: build cmds
- go test -coverprofile=$(COVERAGE_FILE) $(MODULE)/...
+ go test -coverprofile=$(COVERAGE_FILE).with-mocks $(MODULE)/...
coverage: test
- cat $(COVERAGE_FILE) | grep -v "_mock.go" > $(COVERAGE_FILE).no-mocks
- go tool cover -func=$(COVERAGE_FILE).no-mocks
+ cat $(COVERAGE_FILE).with-mocks | grep -v "_mock.go" > $(COVERAGE_FILE)
+ go tool cover -func=$(COVERAGE_FILE)
generate:
go generate $(MODULE)/...
So that make coverage generates a coverage.out file that does not include the generated mocks.
.gitignore
Outdated
| @@ -9,3 +9,4 @@ | |||
| /release-* | |||
| /bin | |||
| /toolkit-test | |||
| /lcov.info | |||
Done |
.github/workflows/golang.yaml
Outdated
| - name: Install Go | ||
| uses: actions/setup-go@v5 | ||
| with: | ||
| go-version: ${{ env.GOLANG_VERSION }} | ||
| - run: make test | ||
|
|
||
| - name: Generate coverage report |
Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>
Add Coveralls to track repo code coverage