fix(ci): set opt-level = 1 for ci codecov#6847
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughUpdate CI workflows and local task runner to split coverage collection: add a Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant SCC as sccache
participant Install as Install actions
participant Mise as mise action
participant Nextest as nextest (tests)
participant Cargo as cargo (tests)
participant Codecov as Codecov
GH->>SCC: export SCCACHE_* env & run mozilla-actions/sccache-action
GH->>Install: install cargo-llvm-cov and nextest tooling
GH->>Mise: run `mise codecov`
Mise->>Nextest: run `cargo llvm-cov --no-report nextest --cargo-profile codecov`
Nextest->>Mise: produce Nextest JUnit & coverage artifacts
Mise->>Cargo: run `cargo llvm-cov test --profile codecov` for lib/package
Cargo->>Mise: produce cargo coverage artifacts
Mise->>Mise: merge coverage -> `lcov.info`
GH->>Codecov: upload `target/nextest/default/junit.xml` (report_type: test_results)
GH->>Codecov: upload merged `lcov.info`
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
44c0366 to
b5f5e25
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted filessee 17 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
b88a4d2 to
34b273b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
mise.toml (1)
221-227: Minor inconsistency:codecovtask duplicates the report command instead of callingcodecov:report.The
codecovtask directly runscargo llvm-cov report(line 226) instead of invoking thecodecov:reporttask (lines 215-219). For consistency and DRY principles, consider usingmise task run codecov:report.♻️ Proposed refactor
[tasks.codecov] description = "Generate codecov report" run = ''' mise task run codecov:nextest mise task run codecov:cargo -cargo llvm-cov report --profile codecov --codecov --output-path lcov.info +mise task run codecov:report '''🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mise.toml` around lines 221 - 227, The tasks.codecov task duplicates the cargo llvm-cov report step instead of reusing the existing codecov:report task; update tasks.codecov to run the existing codecov:report task (i.e., replace the direct "cargo llvm-cov report ..." invocation with "mise task run codecov:report") while keeping the existing mise task runs for codecov:nextest and codecov:cargo so the report generation logic is centralized in the codecov:report task..github/workflows/unit-tests.yml (1)
44-49: Consider adding a comment explaining why this job is disabled.The
if: falsepattern works but lacks context for future maintainers. If this is a permanent change (since coverage is now handled incoverage.yml), consider removing the job entirely. If temporary, add a comment explaining the reason and when it should be re-enabled.tests-release: runs-on: ubuntu-24.04-arm timeout-minutes: 45 - # Run the job only if the PR is not a draft. - # This is done to limit the runner cost. - if: false + # Job disabled: unit tests coverage is now handled in coverage.yml workflow + # using the dedicated codecov profile. Remove this job if no longer needed. + if: false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/unit-tests.yml around lines 44 - 49, The tests-release job is disabled via "if: false" without context; either remove the tests-release job entirely if coverage is now permanently handled in coverage.yml, or leave it and replace the bare "if: false" with a clear comment above the job explaining why it’s disabled (reference coverage.yml), whether this is temporary, and the criteria or date for re-enabling; ensure you update the tests-release job header so future maintainers understand the intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/unit-tests.yml:
- Around line 44-49: The tests-release job is disabled via "if: false" without
context; either remove the tests-release job entirely if coverage is now
permanently handled in coverage.yml, or leave it and replace the bare "if:
false" with a clear comment above the job explaining why it’s disabled
(reference coverage.yml), whether this is temporary, and the criteria or date
for re-enabling; ensure you update the tests-release job header so future
maintainers understand the intent.
In `@mise.toml`:
- Around line 221-227: The tasks.codecov task duplicates the cargo llvm-cov
report step instead of reusing the existing codecov:report task; update
tasks.codecov to run the existing codecov:report task (i.e., replace the direct
"cargo llvm-cov report ..." invocation with "mise task run codecov:report")
while keeping the existing mise task runs for codecov:nextest and codecov:cargo
so the report generation logic is centralized in the codecov:report task.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5db4f342-1c11-4661-bb73-5c931bb16477
📒 Files selected for processing (5)
.github/workflows/coverage.yml.github/workflows/forest.yml.github/workflows/unit-tests.ymlCargo.tomlmise.toml
34b273b to
5fe1887
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/unit-tests.yml (1)
47-49: Consider removing the disabled job instead of usingif: false.The static analysis tool (actionlint) correctly flags this as dead code. Since unit tests are now handled in
coverage.yml, keeping this job disabled viaif: falsecreates workflow clutter. Two options:
- If the job is obsolete: Remove the entire
tests-releasejob (or the workflow file if no other jobs exist)- If keeping for future re-enablement: Add a more detailed comment explaining under what conditions it would be re-enabled
Given the comment already suggests removal, option 1 seems appropriate.
🗑️ Suggested removal
- tests-release: - runs-on: ubuntu-24.04-arm - timeout-minutes: 45 - # Job disabled: unit tests coverage is now handled in coverage.yml workflow - # using the dedicated codecov profile. Remove this job if no longer needed. - if: false - steps: - - name: Configure SCCache variables - run: | - # External PRs do not have access to 'vars' or 'secrets'. - if [[ "${{secrets.AWS_ACCESS_KEY_ID}}" != "" ]]; then - echo "SCCACHE_ENDPOINT=${{ vars.SCCACHE_ENDPOINT}}" >> $GITHUB_ENV - echo "SCCACHE_BUCKET=${{ vars.SCCACHE_BUCKET}}" >> $GITHUB_ENV - echo "SCCACHE_REGION=${{ vars.SCCACHE_REGION}}" >> $GITHUB_ENV - fi - - name: Checkout Sources - uses: actions/checkout@v6 - - name: Setup sccache - uses: mozilla-actions/sccache-action@v0.0.9 - timeout-minutes: ${{ fromJSON(env.CACHE_TIMEOUT_MINUTES) }} - continue-on-error: true - - uses: actions/setup-go@v6 - with: - go-version-file: "go.work" - cache-dependency-path: "**/go.sum" - - name: install nextest - uses: taiki-e/install-action@nextest - - uses: actions/cache@v5 - id: cache-proof-params - with: - path: ${{ env.FIL_PROOFS_PARAMETER_CACHE }} - key: proof-params-keys - - uses: actions/cache@v5 - id: cache-actor-bundle - with: - path: ${{ env.FOREST_ACTOR_BUNDLE_PATH }} - key: actor-bundle - - name: Fetch proof params and RPC test snapshots - run: | - cargo run --bin forest-dev --no-default-features --profile quick-test -- fetch-test-snapshots --actor-bundle $FOREST_ACTOR_BUNDLE_PATH - ls -ahl $FIL_PROOFS_PARAMETER_CACHE - - uses: jdx/mise-action@v4 - - run: | - mise test - env: - FOREST_TEST_SKIP_PROOF_PARAM_CHECK: 1 - # https://docs.github.com/en/actions/reference/runners/github-hosted-runners#standard-github-hosted-runners-for-public-repositories - CARGO_BUILD_JOBS: 3 - - name: Upload Test Results to CodeCov - uses: codecov/codecov-action@v6 - if: ${{ !cancelled() }} - with: - token: ${{ secrets.CODECOV_TOKEN }} - report_type: "test_results" - files: target/nextest/default/junit.xml - fail_ci_if_error: false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/unit-tests.yml around lines 47 - 49, The disabled CI job "tests-release" is left in the workflow behind an `if: false` guard; remove the entire tests-release job block from the workflow (delete the job definition that contains `if: false` and its steps) to eliminate dead code and workflow clutter, or if you prefer to keep it for future use, replace the `if: false` with a clear explanatory comment above the tests-release job that documents exactly when/how it should be re-enabled; ensure the change targets the job named "tests-release" and removes or documents all associated steps and metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mise.toml`:
- Around line 199-201: The tasks.test entry no longer runs doctests but its
existing description still says “Run all tests”; update the tasks.test
description (the description field under the tasks.test/TOML table) to
accurately state that it runs nextest/cargo unit and integration tests and
excludes doctests (or alternatively re-enable doctest execution), so
contributors aren’t misled—either change the description text to something like
“Run unit and integration tests (excludes doctests)” or restore doctest
invocation in the tasks.test task.
---
Nitpick comments:
In @.github/workflows/unit-tests.yml:
- Around line 47-49: The disabled CI job "tests-release" is left in the workflow
behind an `if: false` guard; remove the entire tests-release job block from the
workflow (delete the job definition that contains `if: false` and its steps) to
eliminate dead code and workflow clutter, or if you prefer to keep it for future
use, replace the `if: false` with a clear explanatory comment above the
tests-release job that documents exactly when/how it should be re-enabled;
ensure the change targets the job named "tests-release" and removes or documents
all associated steps and metadata.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2b263de3-93d0-4686-a7ba-dfcdc00062ac
📒 Files selected for processing (5)
.github/workflows/coverage.yml.github/workflows/forest.yml.github/workflows/unit-tests.ymlCargo.tomlmise.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- Cargo.toml
- .github/workflows/forest.yml
- .github/workflows/coverage.yml
5fe1887 to
3d43c15
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/unit-tests.yml (1)
47-49: Prefer retiring this workflow over a permanent no-op job.
if: falsedisables the only job, but theUnit testsworkflow still triggers on every PR/push and sticks around as dead config. If this disable is meant to stay, deleting the file or narrowingon:toworkflow_dispatchonly will keep Actions and required checks cleaner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/unit-tests.yml around lines 47 - 49, The Unit tests workflow currently contains an unconditional no-op gate (`if: false`) which leaves a dead Actions workflow; either delete this workflow file to retire the workflow entirely, or replace the broad `on:` triggers with a minimal trigger (e.g., `workflow_dispatch`) so it only runs manually. Locate the `Unit tests` workflow definition and remove the file or update the `on:` block to narrow triggers, and also remove the `if: false` no-op condition if you choose to retain the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/unit-tests.yml:
- Around line 47-49: The Unit tests workflow currently contains an unconditional
no-op gate (`if: false`) which leaves a dead Actions workflow; either delete
this workflow file to retire the workflow entirely, or replace the broad `on:`
triggers with a minimal trigger (e.g., `workflow_dispatch`) so it only runs
manually. Locate the `Unit tests` workflow definition and remove the file or
update the `on:` block to narrow triggers, and also remove the `if: false` no-op
condition if you choose to retain the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d9d9015e-5669-4f71-8499-a9775237ecd3
📒 Files selected for processing (5)
.github/workflows/coverage.yml.github/workflows/forest.yml.github/workflows/unit-tests.ymlCargo.tomlmise.toml
✅ Files skipped from review due to trivial changes (1)
- Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/coverage.yml
48694a3 to
4cc818f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/coverage.yml:
- Line 47: Change the unconditional workflow conditional that directly
references github.event.pull_request.draft to explicitly verify the event type
first; update the if condition (the existing "if" that uses
github.event.pull_request.draft) so it only checks draft when github.event_name
equals "pull_request" (i.e., combine an explicit event_name check with the
current draft check) to avoid accessing undefined pull_request context on non-PR
triggers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 53834f99-2b64-4648-845d-35c187cdcdd0
📒 Files selected for processing (5)
.github/workflows/coverage.yml.github/workflows/forest.yml.github/workflows/unit-tests.ymlCargo.tomlmise.toml
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/forest.yml
- Cargo.toml
4cc818f to
83f780f
Compare
Summary of changes
Changes introduced in this pull request:
opt-level=1unit-testCI workflowReference issue to close (if applicable)
Closes
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
Chores
Tests