chore: run all tests in ci and coverage#2302
Conversation
|
Warning Rate limit exceeded@tac0turtle has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 0 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe changes introduce a new approach for running Go unit tests and collecting coverage across multiple modules. A new Go script and Makefile target automate test execution and coverage merging. The CI workflow is updated to use this new process, while redundant jobs are removed. The code coverage configuration now ignores mock directories. Changes
Sequence Diagram(s)sequenceDiagram
participant Workflow
participant Makefile
participant CoverageScript
participant GoModules
Workflow->>Makefile: make test-cover
Makefile->>CoverageScript: go run scripts/test_cover.go
loop For each Go module
CoverageScript->>GoModules: go test -coverprofile=cover.out
GoModules-->>CoverageScript: cover.out (if successful)
end
CoverageScript->>CoverageScript: Merge all cover.out files into coverage.txt
CoverageScript->>CoverageScript: Cleanup individual cover.out files
CoverageScript-->>Makefile: Exit status
Makefile-->>Workflow: Test results and coverage.txt
Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
The latest Buf updates on your PR. Results from workflow CI and Release / buf-check (pull_request).
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2302 +/- ##
==========================================
- Coverage 55.79% 50.95% -4.84%
==========================================
Files 53 81 +28
Lines 5008 7093 +2085
==========================================
+ Hits 2794 3614 +820
- Misses 1929 3102 +1173
- Partials 285 377 +92 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new Go script to run tests with coverage reporting and consolidates test execution in CI.
- Added scripts/test_cover.go to detect modules, run tests with coverage, merge cover profiles, and clean up.
- Updated scripts/test.mk and CI workflows in .github/workflows/test.yml to use the new test-cover target.
- Adjusted codecov.yml to ignore additional directories.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| scripts/test_cover.go | New script to run tests with coverage, merge outputs, and clean up |
| scripts/test.mk | Added target to invoke the coverage script with go run |
| codecov.yml | Updated ignore list for coverage analysis |
| .github/workflows/test.yml | Modified test job to run the new target and removed extra test jobs |
Comments suppressed due to low confidence (1)
.github/workflows/test.yml:42
- [nitpick] Ensure the updated CI workflow using 'make test-cover' is reflected in the project documentation so team members understand the new testing and coverage process.
run: make test-cover
| "strings" | ||
| ) | ||
|
|
||
| func main() { |
There was a problem hiding this comment.
[nitpick] The main function encompasses multiple phases (running tests, merging coverage files, and cleaning up). Consider refactoring into smaller helper functions to improve readability and maintainability.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
tac0turtle
force-pushed
the
marko/ci_cover
branch
from
5a1b7a2 to
8bb6616
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
scripts/test.mk (1)
26-26: Fix comment to match target name and correct grammarThe comment should read "## test-cover: generate code coverage report." to match the target name and have correct grammar.
-## cover: generate to code coverage report. +## test-cover: generate code coverage report.scripts/test_cover.go (2)
16-125: 🛠️ Refactor suggestionRefactor main function into smaller helper functions
The main function is very long (over 100 lines) and handles multiple responsibilities: finding modules, running tests, merging coverage files, and cleaning up. This makes it harder to maintain and test.
Consider refactoring into smaller functions like:
findModules(rootDir string) ([]string, error)runTestsWithCoverage(moduleDir string) (string, error)mergeCoverageFiles(coverFiles []string, outputFile string) errorcleanup(files []string) errorThis would improve readability and make the code more maintainable.
41-41: 🛠️ Refactor suggestionAdd race detection and timeout to test command
The test command is missing the
-raceflag that was present in the original CI workflow. Without this flag, race conditions in concurrent code might go undetected.Also, consider adding a timeout to prevent tests from hanging indefinitely:
-cmd := exec.Command("go", "test", "./...", "-coverprofile="+relativeCoverProfileArg, "-covermode=atomic") +cmd := exec.Command("go", "test", "./...", "-race", "-timeout=10m", "-coverprofile="+relativeCoverProfileArg, "-covermode=atomic")
🧹 Nitpick comments (3)
scripts/test_cover.go (3)
16-18: Add command-line flags for better configurabilityThe script would benefit from command-line flags to make it more flexible and reusable. Currently, parameters like the root directory and test flags are hardcoded.
Consider using the
flagpackage to add options like:func main() { rootDir := flag.String("dir", ".", "Root directory to scan for Go modules") testFlags := flag.String("test-flags", "-race -timeout=10m", "Additional flags to pass to 'go test'") outputFile := flag.String("output", "coverage.txt", "Output file for merged coverage") verbose := flag.Bool("verbose", false, "Enable verbose output") flag.Parse() // Use the flags in your code // ... }This would allow users to customize the script's behavior without modifying the code.
73-77: Improve error handling and add context to errorsThe error handling could be more robust, especially for file operations. Consider using
fmt.Errorfto add context to errors and implementing proper cleanup on failure.For example:
-out, err := os.Create(mergedCoverageFile) -if err != nil { - log.Fatalf("Failed to create merged coverage file %s: %v\n", mergedCoverageFile, err) -} -defer out.Close() +out, err := os.Create(mergedCoverageFile) +if err != nil { + return fmt.Errorf("failed to create merged coverage file %s: %w", mergedCoverageFile, err) +} +defer func() { + if cerr := out.Close(); cerr != nil && err == nil { + err = fmt.Errorf("failed to close merged coverage file: %w", cerr) + } +}()Also consider adding signal handling to clean up temporary files when the process is interrupted.
28-31: Consider more robust directory filteringThe current directory filtering approach may miss some directories that should be excluded from testing, such as
.github,node_modules, or other common non-Go directories.Consider using a more comprehensive approach:
-if d.IsDir() && (d.Name() == "vendor" || d.Name() == ".git" || strings.Contains(path, "testdata")) { +// Define a list of common directories to skip +skipDirs := []string{"vendor", ".git", ".github", "node_modules", "build", "dist"} +// Check if directory should be skipped +if d.IsDir() { + if strings.Contains(path, "testdata") { + return filepath.SkipDir + } + for _, skipDir := range skipDirs { + if d.Name() == skipDir { + return filepath.SkipDir + } + } +}This would make the directory filtering more robust and easier to maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
.github/workflows/test.yml(1 hunks)codecov.yml(1 hunks)scripts/test.mk(1 hunks)scripts/test_cover.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (3)
codecov.yml (1)
14-14: Good addition to coverage exclusionsAdding mock directories to the ignore list is appropriate since they shouldn't be considered in coverage metrics.
scripts/test.mk (1)
27-30: LGTM: Well-structured make targetThe implementation of the test-cover target follows the Makefile conventions used throughout the file.
.github/workflows/test.yml (1)
42-42: Good workflow simplificationUsing the new
make test-covertarget centralizes test execution and coverage collection, making the CI workflow more maintainable.
Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>
Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>
| func main() { | ||
| rootDir := "." | ||
| var coverFiles []string | ||
| var testFailures bool |
There was a problem hiding this comment.
The variable testFailures would be more descriptive with a name that follows boolean naming conventions. Consider renaming to hasTestFailures or testsFailed to make its purpose immediately clear to readers.
| var testFailures bool | |
| var testsFailed bool |
Spotted by Diamond (based on custom rules)
Is this helpful? React 👍 or 👎 to let us know.
Overview
this pr creates a test-cover.go file allowing it to run in ci and produce a large coverage reports
ref #2256
Summary by CodeRabbit