Cleanup ci.yaml matrix usage#7260
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces OS-specific test/skip/env handling in Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,230,255,0.5)
participant GH as GitHub Actions
participant Matrix as Job Matrix
participant Runner as OS Runner
participant Steps as Job Steps
end
GH->>Matrix: expand matrix.include (os, skips, env_polluting_tests, extra_test_args, timeout)
Matrix->>Runner: select runner for matrix.os
Runner->>Steps: execute unified job with matrix.* values
Steps->>Steps: run platform-independent tests (matrix.extra_test_args)
Steps->>Steps: run platform-dependent tests excluding matrix.skips
Steps->>Steps: run env-polluter checks using matrix.env_polluting_tests
Steps-->>Runner: return results
Runner-->>GH: report completion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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)
📝 Coding Plan
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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yaml (1)
343-367:⚠️ Potential issue | 🟠 Major
ENV_POLLUTING_TESTS_COMMONis referenced but not defined.Line 346 references
${{ env.ENV_POLLUTING_TESTS_COMMON }}, but this environment variable is not defined anywhere in the file. Combined withmatrix.job.env_polluting_testsbeing set to""for all matrix entries, the for-loop body will never execute, making this step a silent no-op.If there are currently no environment-polluting tests to check, consider either:
- Defining
ENV_POLLUTING_TESTS_COMMONin theenv:section (even if empty, for clarity), or- Removing this step entirely if it's no longer needed, or
- Adding a comment explaining that this step is intentionally dormant pending future polluting tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yaml around lines 343 - 367, The workflow step's for-loop references ENV_POLLUTING_TESTS_COMMON and matrix.job.env_polluting_tests but ENV_POLLUTING_TESTS_COMMON is not defined, so the loop will be a no-op; fix by defining ENV_POLLUTING_TESTS_COMMON in the workflow's env: block (even as an empty string) or remove/comment out this step if it's no longer needed — locate the references to ENV_POLLUTING_TESTS_COMMON and matrix.job.env_polluting_tests in the CI step that runs rustpython (the for thing in ... loop) and either add an env: ENV_POLLUTING_TESTS_COMMON: "" entry at the top-level env or delete/add a clear comment explaining the step is intentionally dormant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.github/workflows/ci.yaml:
- Around line 343-367: The workflow step's for-loop references
ENV_POLLUTING_TESTS_COMMON and matrix.job.env_polluting_tests but
ENV_POLLUTING_TESTS_COMMON is not defined, so the loop will be a no-op; fix by
defining ENV_POLLUTING_TESTS_COMMON in the workflow's env: block (even as an
empty string) or remove/comment out this step if it's no longer needed — locate
the references to ENV_POLLUTING_TESTS_COMMON and matrix.job.env_polluting_tests
in the CI step that runs rustpython (the for thing in ... loop) and either add
an env: ENV_POLLUTING_TESTS_COMMON: "" entry at the top-level env or delete/add
a clear comment explaining the step is intentionally dormant.
bd21154 to
36fed7d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/ci.yaml (2)
300-303: Use matrix args here too to avoid drift.Line 303 hardcodes
-u all, while Line 310 already pulls frommatrix.extra_test_args. Reusing matrix input here keeps a single source of truth.♻️ Proposed change
- target/release/rustpython -m test -j 1 -u all --slowest --fail-env-changed --timeout 600 -v ${{ env.PLATFORM_INDEPENDENT_TESTS }} + target/release/rustpython -m test -j 1 ${{ join(matrix.extra_test_args, ' ') }} --slowest --fail-env-changed --timeout 600 -v ${{ env.PLATFORM_INDEPENDENT_TESTS }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yaml around lines 300 - 303, The workflow step "run cpython platform-independent tests" uses a hardcoded "-u all" in the test command; replace that hardcoded arg with the matrix variable already used elsewhere (matrix.extra_test_args or env.PLATFORM_INDEPENDENT_TESTS) so the step consumes the same single source of truth as the other test step—update the run string that invokes "target/release/rustpython -m test ..." to include the matrix.extra_test_args (or the existing PLATFORM_INDEPENDENT_TESTS env var) instead of "-u all" and ensure the job matrix defines extra_test_args consistently.
315-332: Skip env-polluter verification step when list is empty.With current matrix values, this step is a no-op on every OS. Adding a step-level guard makes intent clearer and trims unnecessary CI work.
🧹 Proposed guard
- name: run cpython tests to check if env polluters have stopped polluting + if: ${{ join(matrix.env_polluting_tests, '') != '' }} shell: bash run: | for thing in ${{ join(matrix.env_polluting_tests, ' ') }}; do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yaml around lines 315 - 332, The CI step named "run cpython tests to check if env polluters have stopped polluting" currently runs even when matrix.env_polluting_tests is empty; add a step-level guard so the step only runs when matrix.env_polluting_tests is non-empty (i.e., check matrix.env_polluting_tests before executing the for-loop), thereby skipping the whole step when there are no env polluters to verify.
🤖 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/ci.yaml:
- Around line 300-303: The workflow step "run cpython platform-independent
tests" uses a hardcoded "-u all" in the test command; replace that hardcoded arg
with the matrix variable already used elsewhere (matrix.extra_test_args or
env.PLATFORM_INDEPENDENT_TESTS) so the step consumes the same single source of
truth as the other test step—update the run string that invokes
"target/release/rustpython -m test ..." to include the matrix.extra_test_args
(or the existing PLATFORM_INDEPENDENT_TESTS env var) instead of "-u all" and
ensure the job matrix defines extra_test_args consistently.
- Around line 315-332: The CI step named "run cpython tests to check if env
polluters have stopped polluting" currently runs even when
matrix.env_polluting_tests is empty; add a step-level guard so the step only
runs when matrix.env_polluting_tests is non-empty (i.e., check
matrix.env_polluting_tests before executing the for-loop), thereby skipping the
whole step when there are no env polluters to verify.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 934455a1-3965-4330-80dd-d4bd5b07da9a
📒 Files selected for processing (1)
.github/workflows/ci.yaml
* cleanup * Don't enable all resources on windows
Summary by CodeRabbit
Chores
Tests