Cleanup ci.yaml matrix usage - #7260
Conversation
Sorry, something went wrong.
|
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 reviews.auto_review.auto_pause_after_reviewed_commits setting. 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: 6eb712f2-1f5e-4439-8235-a6c7206bd08a 📥 CommitsReviewing files that changed from the base of the PR and between b667837 and 8f5161f. 📒 Files selected for processing (1)
📝 Walkthrough WalkthroughReplaces OS-specific test/skip/env handling in .github/workflows/ci.yaml with a matrix-driven job configuration that embeds per-entry os, skips, env_polluting_tests, extra_test_args, and timeout, and consolidates test and env-polluter steps into unified, matrix-aware steps. 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)
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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
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_COMMON is referenced but not defined.
Line 346 references ${{ env.ENV_POLLUTING_TESTS_COMMON }}, but this environment variable is not defined anywhere in the file. Combined with matrix.job.env_polluting_tests being 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:
🤖 Prompt for AI Agents
- Defining ENV_POLLUTING_TESTS_COMMON in the env: 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.
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.
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.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between 5c726e1 and e333638.
📒 Files selected for processing (1)- .github/workflows/ci.yaml
Sorry, something went wrong.
There was a problem hiding this comment.
.github/workflows/ci.yaml (2)🤖 Prompt for all review comments with AI agents300-303: Use matrix args here too to avoid drift.
Line 303 hardcodes -u all, while Line 310 already pulls from matrix.extra_test_args. Reusing matrix input here keeps a single source of truth.
♻️ Proposed change🤖 Prompt for AI Agents- 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 }}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🤖 Prompt for AI Agents- 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, ' ') }}; doVerify 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.
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
📥 CommitsReviewing files that changed from the base of the PR and between 36fed7d and b667837.
📒 Files selected for processing (1)- .github/workflows/ci.yaml
Sorry, something went wrong.
* cleanup * Don't enable all resources on windows
* cleanup * Don't enable all resources on windows
* cleanup * Don't enable all resources on windows
Summary by CodeRabbit
Chores
Tests