-
Notifications
You must be signed in to change notification settings - Fork 83
Add test coverage to test-unit Makefile target #295
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: carlory <[email protected]>
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. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few questions regrading usage of the action and coverage mode
make test | ||
- name: Upload coverage to Codecov | ||
uses: codecov/codecov-action@18283e04ce6e62d37312384ff67231eb8fd56d24 # v5.4.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason we can't use a tagged release instead of a SHA?
- name: Upload coverage to Codecov | ||
uses: codecov/codecov-action@18283e04ce6e62d37312384ff67231eb8fd56d24 # v5.4.3 | ||
with: | ||
fail_ci_if_error: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's considered an error for coverage?
test-unit: download-tokenizer download-zmq | ||
@printf "\033[33;1m==== Running Unit Tests ====\033[0m\n" | ||
go test -ldflags="$(LDFLAGS)" -v ./... | ||
go test -ldflags="$(LDFLAGS)" -v -covermode=atomic -coverprofile=go-test-coverage.out ./... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we go with and pay the runtime penalty for covermode=atomic
due to concurrent code execution, might be worthwhile to also enable -race
detection in tests.
Hi, I was wondering about the rationale behind using an external service. What benefits would it bring? Since we already have the coverage file from go test, it seems possible to use tools like go tool cover—so I’m curious about which specific features we’re looking to leverage through CodeDev. |
@carlory are you still actively working on this? Let us know if you're still able/interested in pursuing this further. |
Fix #287