-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
feat: add coverage scripts #15018
feat: add coverage scripts #15018
Conversation
Signed-off-by: Eshaan Aggarwal <[email protected]>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Signed-off-by: Eshaan Aggarwal <[email protected]>
Signed-off-by: Eshaan Aggarwal <[email protected]>
I use the following alias to check to code coverage of any go package: |
Seems cool, but how would you customize the package that you want to test and see the reports for with this alias? Running the tests for all the packages every time is not feasible. |
coverage: | ||
go install golang.org/x/tools/cmd/cover | ||
go test -coverprofile=coverage.out $(PACKAGE) | ||
go tool cover -html=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.
This feels very similar to the already existing unit_test_cover
Makefile target. The following snippet is going to be important is testing the sqlparser
package:
# Handle go tool cover failures due to not handling `//line` directives, which
# the goyacc compiler adds to the generated parser in sql.go. See:
# https://github.com/golang/go/issues/41222
sed -i'' -e '/^vitess.io\/vitess\/go\/vt\/sqlparser\/yaccpar/d' 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.
Ahh got it. Should I rename the same to test.coverage
to avoid the target conflict?
|
||
coverage: | ||
go install golang.org/x/tools/cmd/cover | ||
go test -coverprofile=coverage.out $(PACKAGE) |
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.
Just a thought, but I often find myself using the -v
flag too as it helps find failing tests. Might be worth adding
This PR is being marked as stale because it has been open for 30 days with no activity. To rectify, you may do any of the following:
If no action is taken within 7 days, this PR will be closed. |
This PR was closed because it has been stale for 7 days with no activity. |
Description
Edited the
Makefile
to add three new scripts:coverage
: This target runs the tests and generates a coverage report in the coverage.out file. It utilizes the go test command with the -coverprofile flag to collect coverage data.It can be run as
make PACKAGE=<path to go package> coverage
After running the same, the browser shows an HTML page indicating the current coverage.
coverage-clean
: This target removes the generated coverage file (coverage.out
). It's useful for cleaning up after running coverage tests.coverage-help
: This target displays a usage message with information about available options and commands related to coverage.Related Issue(s)
Fixes #15003
Checklist
Deployment Notes