make: avoid coverage for packages without tests #4092
Conversation
- Extract package exclusion pattern into reusable variable `PACKAGE_EXCLUDE` - Modify `unit_test`, `unit_test_in_verify_ci`, and `unit_test_in_verify_ci_next_gen` targets to split packages into those with test files and those without - Run coverage tests only on packages containing test files to avoid errors - Execute non-test packages separately without coverage flags to ensure all packages are tested appropriately Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
ti-chi-bot
bot
added
the
release-note-none
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Summary of ChangesHello @tenfyzhong, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a recurring issue in the CI pipeline where unit test targets would erroneously fail when attempting to generate code coverage for Go packages that do not contain any test files. By introducing a mechanism to differentiate between packages with and without tests, the build system now intelligently applies coverage only where appropriate, ensuring that all unit test targets complete successfully and provide meaningful coverage metrics without encountering Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
ti-chi-bot
bot
added
the
size/M
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses an issue where the test process would fail due to coverage being applied to packages without tests. The fix involves separating packages with tests from those without, and only applying coverage to the former. This is a good solution to the problem.
My review focuses on improving the maintainability of the Makefile. I've noticed significant code duplication across several test targets. I've left a comment with a suggestion to refactor this using a define block, which would make future changes easier. I've also suggested a small improvement to the go list command to make it cleaner.
| test_pkgs=$$($(GO) list -tags="$(TEST_FLAG)" -f '{{if or .TestGoFiles .XTestGoFiles}} {{.ImportPath}} {{end}}' ./... | tr -s ' ' '\n' | grep -vE '$(PACKAGE_EXCLUDE)'); \ | ||
| no_test_pkgs=$$($(GO) list -tags="$(TEST_FLAG)" -f '{{if not (or .TestGoFiles .XTestGoFiles)}} {{.ImportPath}} {{end}}' ./... | tr -s ' ' '\n' | grep -vE '$(PACKAGE_EXCLUDE)'); \ |
There was a problem hiding this comment.
This logic to separate packages based on the presence of test files is duplicated in the unit_test, unit_test_in_verify_ci, and unit_test_in_verify_ci_next_gen targets. This duplication can make the Makefile difficult to maintain.
Consider refactoring this repeated logic into a define block to improve maintainability.
Additionally, you can simplify the go list command by using a format string that produces newline-separated output, which eliminates the need for tr.
test_pkgs=$$($(GO) list -tags="$(TEST_FLAG)" -f '{{if or .TestGoFiles .XTestGoFiles}}{{.ImportPath}}{{"\n"}}{{end}}' ./... | grep -vE '$(PACKAGE_EXCLUDE)'); \
no_test_pkgs=$$($(GO) list -tags="$(TEST_FLAG)" -f '{{if not (or .TestGoFiles .XTestGoFiles)}}{{.ImportPath}}{{"\n"}}{{end}}' ./... | grep -vE '$(PACKAGE_EXCLUDE)'); \
📝 WalkthroughWalkthroughThe Makefile introduces a centralized Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@Makefile`:
- Around line 289-303: The Makefile runs tools/bin/gocov convert on
"$(TEST_DIR)/cov.unit.out" even when no tests ran; guard that step by checking
the existence and non-zero size of the coverage file (cov.unit.out) before
invoking tools/bin/gocov convert and tools/bin/gocov-xml. Modify the block that
defines test_pkgs/no_test_pkgs (and the final conversion lines) to only run the
convert/ XML generation when "$(TEST_DIR)/cov.unit.out" exists and is non-empty
(or equivalently only when test_pkgs is non-empty), while preserving the
existing FAILPOINT_DISABLE invocation.
- Around line 268-281: The Makefile step assumes "$(TEST_DIR)/cov.unit.out"
exists but if test_pkgs is empty no coverage file is created; modify the target
to guard the gocov conversion by checking for the existence and non-zero size of
"$(TEST_DIR)/cov.unit.out" (or create an empty fallback file) before running
tools/bin/gocov convert and tools/bin/gocov-xml, using the existing variables
TEST_DIR and cov.unit.out and keeping the current behavior when tests produce
coverage; ensure this check occurs after the test_pkgs/no_test_pkgs blocks that
create the file.
🧹 Nitpick comments (1)
Makefile (1)
251-260: Consider removing theno_test_pkgsexecution block.Running
go teston packages without test files is redundant—it only outputs[no test files]and exits successfully. While harmless, it adds execution time without providing value.If the intent is to verify compilation with test build tags, this is already achieved when building the test binary or during CI builds.
♻️ Proposed simplification (optional)
test_pkgs=$$($(GO) list -tags="$(TEST_FLAG)" -f '{{if or .TestGoFiles .XTestGoFiles}} {{.ImportPath}} {{end}}' ./... | tr -s ' ' '\n' | grep -vE '$(PACKAGE_EXCLUDE)'); \ - no_test_pkgs=$$($(GO) list -tags="$(TEST_FLAG)" -f '{{if not (or .TestGoFiles .XTestGoFiles)}} {{.ImportPath}} {{end}}' ./... | tr -s ' ' '\n' | grep -vE '$(PACKAGE_EXCLUDE)'); \ if [ -n "$$test_pkgs" ]; then \ $(GOTEST) -cover -covermode=atomic -coverprofile="$(TEST_DIR)/cov.unit.out" $$test_pkgs \ || { $(FAILPOINT_DISABLE); exit 1; }; \ - fi; \ - if [ -n "$$no_test_pkgs" ]; then \ - $(GOTEST) $$no_test_pkgs \ - || { $(FAILPOINT_DISABLE); exit 1; }; \ fi
| test_pkgs=$$($(GO) list -tags="intest" -f '{{if or .TestGoFiles .XTestGoFiles}} {{.ImportPath}} {{end}}' ./... | tr -s ' ' '\n' | grep -vE '$(PACKAGE_EXCLUDE)'); \ | ||
| no_test_pkgs=$$($(GO) list -tags="intest" -f '{{if not (or .TestGoFiles .XTestGoFiles)}} {{.ImportPath}} {{end}}' ./... | tr -s ' ' '\n' | grep -vE '$(PACKAGE_EXCLUDE)'); \ | ||
| if [ -n "$$test_pkgs" ]; then \ | ||
| CGO_ENABLED=1 tools/bin/gotestsum --junitfile cdc-junit-report.xml -- -v -timeout 300s -p $(P) --race --tags=intest \ | ||
| -parallel=16 \ | ||
| -covermode=atomic -coverprofile="$(TEST_DIR)/cov.unit.out" $$test_pkgs \ | ||
| || { $(FAILPOINT_DISABLE); exit 1; }; \ | ||
| fi; \ | ||
| if [ -n "$$no_test_pkgs" ]; then \ | ||
| CGO_ENABLED=1 $(GO) test -v -timeout 300s -p $(P) --race --tags=intest \ | ||
| -parallel=16 $$no_test_pkgs \ | ||
| || { $(FAILPOINT_DISABLE); exit 1; }; \ | ||
| fi | ||
| tools/bin/gocov convert "$(TEST_DIR)/cov.unit.out" | tools/bin/gocov-xml > cdc-coverage.xml |
There was a problem hiding this comment.
Potential edge case: coverage file may not exist if no packages have tests.
If test_pkgs is empty (unlikely but possible), cov.unit.out won't be generated, and line 281 will fail when gocov convert attempts to read it.
Consider adding a guard or touching an empty coverage file as a fallback.
🛡️ Proposed defensive fix
if [ -n "$$no_test_pkgs" ]; then \
CGO_ENABLED=1 $(GO) test -v -timeout 300s -p $(P) --race --tags=intest \
-parallel=16 $$no_test_pkgs \
|| { $(FAILPOINT_DISABLE); exit 1; }; \
fi
+ `@if` [ ! -f "$(TEST_DIR)/cov.unit.out" ]; then \
+ echo "mode: atomic" > "$(TEST_DIR)/cov.unit.out"; \
+ fi
tools/bin/gocov convert "$(TEST_DIR)/cov.unit.out" | tools/bin/gocov-xml > cdc-coverage.xml📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test_pkgs=$$($(GO) list -tags="intest" -f '{{if or .TestGoFiles .XTestGoFiles}} {{.ImportPath}} {{end}}' ./... | tr -s ' ' '\n' | grep -vE '$(PACKAGE_EXCLUDE)'); \ | |
| no_test_pkgs=$$($(GO) list -tags="intest" -f '{{if not (or .TestGoFiles .XTestGoFiles)}} {{.ImportPath}} {{end}}' ./... | tr -s ' ' '\n' | grep -vE '$(PACKAGE_EXCLUDE)'); \ | |
| if [ -n "$$test_pkgs" ]; then \ | |
| CGO_ENABLED=1 tools/bin/gotestsum --junitfile cdc-junit-report.xml -- -v -timeout 300s -p $(P) --race --tags=intest \ | |
| -parallel=16 \ | |
| -covermode=atomic -coverprofile="$(TEST_DIR)/cov.unit.out" $$test_pkgs \ | |
| || { $(FAILPOINT_DISABLE); exit 1; }; \ | |
| fi; \ | |
| if [ -n "$$no_test_pkgs" ]; then \ | |
| CGO_ENABLED=1 $(GO) test -v -timeout 300s -p $(P) --race --tags=intest \ | |
| -parallel=16 $$no_test_pkgs \ | |
| || { $(FAILPOINT_DISABLE); exit 1; }; \ | |
| fi | |
| tools/bin/gocov convert "$(TEST_DIR)/cov.unit.out" | tools/bin/gocov-xml > cdc-coverage.xml | |
| test_pkgs=$$($(GO) list -tags="intest" -f '{{if or .TestGoFiles .XTestGoFiles}} {{.ImportPath}} {{end}}' ./... | tr -s ' ' '\n' | grep -vE '$(PACKAGE_EXCLUDE)'); \ | |
| no_test_pkgs=$$($(GO) list -tags="intest" -f '{{if not (or .TestGoFiles .XTestGoFiles)}} {{.ImportPath}} {{end}}' ./... | tr -s ' ' '\n' | grep -vE '$(PACKAGE_EXCLUDE)'); \ | |
| if [ -n "$$test_pkgs" ]; then \ | |
| CGO_ENABLED=1 tools/bin/gotestsum --junitfile cdc-junit-report.xml -- -v -timeout 300s -p $(P) --race --tags=intest \ | |
| -parallel=16 \ | |
| -covermode=atomic -coverprofile="$(TEST_DIR)/cov.unit.out" $$test_pkgs \ | |
| || { $(FAILPOINT_DISABLE); exit 1; }; \ | |
| fi; \ | |
| if [ -n "$$no_test_pkgs" ]; then \ | |
| CGO_ENABLED=1 $(GO) test -v -timeout 300s -p $(P) --race --tags=intest \ | |
| -parallel=16 $$no_test_pkgs \ | |
| || { $(FAILPOINT_DISABLE); exit 1; }; \ | |
| fi | |
| `@if` [ ! -f "$(TEST_DIR)/cov.unit.out" ]; then \ | |
| echo "mode: atomic" > "$(TEST_DIR)/cov.unit.out"; \ | |
| fi | |
| tools/bin/gocov convert "$(TEST_DIR)/cov.unit.out" | tools/bin/gocov-xml > cdc-coverage.xml |
🤖 Prompt for AI Agents
In `@Makefile` around lines 268 - 281, The Makefile step assumes
"$(TEST_DIR)/cov.unit.out" exists but if test_pkgs is empty no coverage file is
created; modify the target to guard the gocov conversion by checking for the
existence and non-zero size of "$(TEST_DIR)/cov.unit.out" (or create an empty
fallback file) before running tools/bin/gocov convert and tools/bin/gocov-xml,
using the existing variables TEST_DIR and cov.unit.out and keeping the current
behavior when tests produce coverage; ensure this check occurs after the
test_pkgs/no_test_pkgs blocks that create the file.
| test_pkgs=$$($(GO) list -tags="intest,nextgen" -f '{{if or .TestGoFiles .XTestGoFiles}} {{.ImportPath}} {{end}}' ./... | tr -s ' ' '\n' | grep -vE '$(PACKAGE_EXCLUDE)'); \ | ||
| no_test_pkgs=$$($(GO) list -tags="intest,nextgen" -f '{{if not (or .TestGoFiles .XTestGoFiles)}} {{.ImportPath}} {{end}}' ./... | tr -s ' ' '\n' | grep -vE '$(PACKAGE_EXCLUDE)'); \ | ||
| if [ -n "$$test_pkgs" ]; then \ | ||
| CGO_ENABLED=1 tools/bin/gotestsum --junitfile cdc-junit-report.xml -- -v -timeout 300s -p $(P) --race --tags=intest,nextgen \ | ||
| -parallel=16 \ | ||
| -covermode=atomic -coverprofile="$(TEST_DIR)/cov.unit.out" $$test_pkgs \ | ||
| || { $(FAILPOINT_DISABLE); exit 1; }; \ | ||
| fi; \ | ||
| if [ -n "$$no_test_pkgs" ]; then \ | ||
| CGO_ENABLED=1 $(GO) test -v -timeout 300s -p $(P) --race --tags=intest,nextgen \ | ||
| -parallel=16 $$no_test_pkgs \ | ||
| || { $(FAILPOINT_DISABLE); exit 1; }; \ | ||
| fi | ||
| tools/bin/gocov convert "$(TEST_DIR)/cov.unit.out" | tools/bin/gocov-xml > cdc-coverage.xml | ||
| $(FAILPOINT_DISABLE) |
There was a problem hiding this comment.
Same edge case concern as unit_test_in_verify_ci.
The coverage file guard suggestion from the previous comment applies here as well—if test_pkgs is empty, gocov convert on line 302 would fail.
Otherwise, the implementation correctly mirrors the other test targets with the appropriate intest,nextgen tags.
🤖 Prompt for AI Agents
In `@Makefile` around lines 289 - 303, The Makefile runs tools/bin/gocov convert
on "$(TEST_DIR)/cov.unit.out" even when no tests ran; guard that step by
checking the existence and non-zero size of the coverage file (cov.unit.out)
before invoking tools/bin/gocov convert and tools/bin/gocov-xml. Modify the
block that defines test_pkgs/no_test_pkgs (and the final conversion lines) to
only run the convert/ XML generation when "$(TEST_DIR)/cov.unit.out" exists and
is non-empty (or equivalently only when test_pkgs is non-empty), while
preserving the existing FAILPOINT_DISABLE invocation.
1 participant
What problem does this PR solve?
Issue Number: close #4089
The
make unit_test_in_verify_citarget exits with code 1 even when all tests pass, because coverage is applied to packages without tests and triggerscovdataerrors.What is changed and how it works?
PACKAGE_EXCLUDE.go listand build tags.unit_test,unit_test_in_verify_ci, andunit_test_in_verify_ci_next_gen.Check List
Tests
make unit_test_in_verify_ci)Questions
Will it cause performance regression or break compatibility?
No.
Do you need to update user documentation, design documentation or monitoring documentation?
No.
Release note
Summary by CodeRabbit
Chores
Tests
Note: This release contains internal build and testing infrastructure improvements with no user-facing changes.
✏️ Tip: You can customize this high-level summary in your review settings.