-
Notifications
You must be signed in to change notification settings - Fork 12
chore: seperate test and bench job to accelerate the ci process #52
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
Conversation
- Introduced a new action to set up the Go environment with caching. - Updated CI workflow to utilize the new setup-go action for both testing and benchmarking jobs, improving code organization and reusability.
WalkthroughThe GitHub Actions workflow configuration was updated by renaming the "build" job to "test" and simplifying its test execution command. Additionally, a new "bench" job was introduced to run Go benchmark tests, with both jobs leveraging Go module caching. The coverage upload step remains unchanged. Changes
Sequence Diagram(s)sequenceDiagram
participant GitHub Actions
participant Test Job
participant Bench Job
GitHub Actions->>Test Job: Run Go tests (single-line command)
Test Job->>GitHub Actions: Upload coverage
GitHub Actions->>Bench Job: Run Go benchmarks (single-line command)
Possibly related PRs
Poem
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
Documentation and Community
|
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.
Actionable comments posted: 4
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
49-51: Consider persisting benchmark resultsYou’re running benchmarks but not capturing the output.
If historical performance tracking is valuable, upload thego testoutput (or a processed JSON) as an artifact or push to a dedicated benchmarking service.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/actions/setup-go/action.yml(1 hunks).github/workflows/ci.yml(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI
.github/actions/setup-go/action.yml
[error] 1-1: Can't find 'action.yml', 'action.yaml' or 'Dockerfile' under the directory. Possibly missing actions/checkout step before running the local action.
.github/actions/setup-go/action.yml
Outdated
| go-version: | ||
| description: "Go version to use" | ||
| required: false | ||
| default: "^1.24" |
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.
🛠️ Refactor suggestion
Default Go version should be a concrete, released version (or stable)
^1.24 is not a released Go version and may cause the job to resolve to an unexpected nightly build or fail once 1.24 RC is removed. Pin to a known-good major/minor (e.g. 1.22.x) or simply use stable.
- default: "^1.24"
+ # Use the latest released Go in the 1.22 line.
+ default: "1.22.x"📝 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.
| go-version: | |
| description: "Go version to use" | |
| required: false | |
| default: "^1.24" | |
| go-version: | |
| description: "Go version to use" | |
| required: false | |
| # Use the latest released Go in the 1.22 line. | |
| default: "1.22.x" |
🤖 Prompt for AI Agents
In .github/actions/setup-go/action.yml at lines 4 to 7, the default Go version
is set to "^1.24", which is not a concrete released version and may cause
resolution issues. Change the default value to a specific released version like
"1.22.x" or use "stable" to ensure the action uses a known-good Go version.
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
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.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
.github/workflows/ci.yml (2)
31-36: Use the newly-added custom action instead of the upstream oneThe PR description states that a custom
setup-goaction was created to centralise configuration & caching, yet this step still calls the publicactions/setup-go@v4.
Switching to the in-repo action (e.g../.github/actions/setup-go) will actually exercise the new code and avoid future divergence.- - name: Set up Go - uses: actions/setup-go@v4 + - name: Set up Go + uses: ./.github/actions/setup-goIf the intention is to keep the upstream action, please update the PR description accordingly to avoid confusion.
47-62: Bench job still runs unit tests – add-run=^$(and disable cache) for accurate benchmarks
go test -bench=. -benchmemalso executes all tests, which doubles the run time and can skew benchmark results.
Benchmarks should be executed with tests disabled and caching turned off:- - name: Run benchmark tests - run: go test -bench=. -benchmem + - name: Run benchmark tests + run: go test -bench=. -benchmem -run=^$ -count=1This change keeps the job focused, short, and reliable.
🧹 Nitpick comments (2)
.github/workflows/ci.yml (2)
37-38: Minor: preserve module reproducibility flagsConsider appending
-mod=readonly(and optionally-trimpath) so that the job fails whengo.mod/go.sumdrift and to produce deterministic paths in coverage files.- run: go test -race -coverprofile=coverage.txt -covermode=atomic ./... + run: go test -race -mod=readonly -coverprofile=coverage.txt -covermode=atomic ./...
55-58: Optional: tighten cache settings for monoreposIf the repo ever gains multiple
go.modfiles, specify them explicitly:with: go-version: "^1.24" cache: true cache-dependency-path: | **/go.sumThis prevents cross-module cache pollution.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml(3 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/ci.yml
55-55: the runner of "actions/setup-go@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
and reusability.
Description
Brief description of what this PR does.
Type of Change
Changes Made
Testing
Performance Impact
Checklist
Related Issues
Fixes #(issue number)
Additional Notes
Any additional information that reviewers should know.