Add GitHub Actions to run the tests on every push#26
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a GitHub Actions workflow to automate continuous integration by running tests, builds, formatting checks, and linting on every push, pull request, and manual trigger.
Changes:
- Added a new CI workflow configuration file with build, test, fmt, and clippy verification steps
- Configured the workflow to run on push, pull_request, and workflow_dispatch events
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: Run clippy | ||
| run: cargo clippy -- --deny warnings |
There was a problem hiding this comment.
The cargo clippy command requires the clippy component to be installed. Add a step to install clippy with "rustup component add clippy" before this step, or handle the case where it might not be available.
| run: cargo fmt --check | ||
|
|
||
| - name: Run clippy | ||
| run: cargo clippy -- --deny warnings |
There was a problem hiding this comment.
The clippy flags are inconsistent with the README.md documentation which uses "-D warnings", while this workflow uses "--deny warnings". While both work, consider using the same flag format as documented in README.md (line 131) for consistency.
| run: cargo clippy -- --deny warnings | |
| run: cargo clippy -- -D warnings |
| # - cron: '0 * * * *' | ||
|
|
||
| jobs: | ||
| generate: |
There was a problem hiding this comment.
The job name "generate" is misleading and doesn't accurately describe what this job does. This job builds, tests, formats, and lints the code - consider renaming it to something more descriptive like "test" or "build-and-test".
| generate: | |
| build-and-test: |
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v6 | ||
|
|
There was a problem hiding this comment.
Consider adding caching for Cargo dependencies to improve CI performance. This can be done using actions/cache or the Swatinem/rust-cache action, which will cache the target directory and Cargo registry to speed up subsequent builds.
| - name: Cache Rust build | |
| uses: Swatinem/rust-cache@v2 |
| - name: Run cargo fmt --check | ||
| run: cargo fmt --check |
There was a problem hiding this comment.
The cargo fmt command requires the rustfmt component to be installed. Add a step to install rustfmt with "rustup component add rustfmt" before this step, or handle the case where it might not be available.
ueneid
left a comment
There was a problem hiding this comment.
Thanks for adding CI! I've left some comments based on GitHub Actions best practices and security considerations. Please take a look when you get a chance.
| @@ -0,0 +1,38 @@ | |||
| name: CI | |||
|
|
|||
| on: | |||
There was a problem hiding this comment.
Consider adding the following workflow-level settings:
- concurrency — Automatically cancels outdated runs on the same PR, saving time and cost:
concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: true
defaults.run.shell: bash— Enablesset -eo pipefailautomatically, preventing silent failures in piped commands.env: CARGO_TERM_COLOR: always— Colorized cargo output improves CI log readability.
| name: CI | ||
|
|
||
| on: | ||
| push: |
There was a problem hiding this comment.
Without a branch filter, CI runs on every push to every branch. Combined with the pull_request trigger, this causes duplicate runs for PR branches.
Suggestion:
push:
branches: [main]
This way push only triggers for merges to main, and pull_request covers all PR activity.
| push: | ||
| pull_request: | ||
| workflow_dispatch: | ||
| # schedule: |
There was a problem hiding this comment.
Commented-out code should either be removed or accompanied by a comment explaining why it's kept. If scheduled runs are planned for the future, consider adding them in a separate PR when ready.
| # - cron: '0 * * * *' | ||
|
|
||
| jobs: | ||
| generate: |
There was a problem hiding this comment.
The job name generate is misleading — this job builds, tests, and lints. Consider renaming to ci or check to better reflect its purpose. This name is also displayed in the GitHub UI (e.g., status checks on PRs).
|
|
||
| jobs: | ||
| generate: | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
Missing timeout-minutes. The default is 360 minutes (6 hours), which means a hung job could silently burn runner minutes.
Suggestion:
timeout-minutes: 10
| generate: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v6 |
There was a problem hiding this comment.
Security: Pin actions to a full commit SHA instead of a mutable tag. Tags can be overwritten (e.g., via a compromised action repo), but commit hashes are immutable.
Suggestion:
- uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
The version comment preserves readability. Tools like Dependabot/Renovate can still auto-update hash-pinned actions.
|
|
||
| - name: Update rust | ||
| run: | | ||
| rustup update |
There was a problem hiding this comment.
Two suggestions here:
- Be explicit about the toolchain channel:
run: rustup update stablerustup update (without arguments) updates all installed toolchains. Specifying stable makes the intent clear.
2. Add Swatinem/rust-cache after this step to cache ~/.cargo and target/.
It automatically cleans unused build artifacts, resulting in smaller caches than manual actions/cache configuration:
- uses: Swatinem/rust-cache@779680da715d629ac1d338a641029a2f4372abb5 # v2.8.2| rustc --version | ||
| cargo --version | ||
|
|
||
| - name: cargo build |
There was a problem hiding this comment.
Consider reordering steps: move cargo fmt --check and cargo clippy before cargo build and cargo test.
Formatting and lint checks are much faster than a full build. If they fail, the developer gets feedback sooner without waiting for compilation and test execution.
| run: cargo fmt --check | ||
|
|
||
| - name: Run clippy | ||
| run: cargo clippy -- --deny warnings |
There was a problem hiding this comment.
Add --all-targets to also lint test code, examples, and benchmarks:
run: cargo clippy --all-targets -- --deny warnings
Without it, clippy only checks the library/binary targets and may miss issues in #[cfg(test)] modules.
|
Closing in favor of #28. |
Description
Add GitHub Action configuration to run the tests and certain verification on every push and every pull-request.
Type of Change