Open
Conversation
Contributor
📝 WalkthroughWalkthroughRefactors 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 Shell as Job Steps
end
GH->>Matrix: expand matrix.include (os, skips, PLATFORM_INDEPENDENT_TESTS, env_polluting_tests, timeout)
Matrix->>Runner: select runner for entry (linux/macos/windows)
Runner->>Shell: execute unified job steps with matrix.* args
Shell->>Shell: run platform-independent tests (matrix.PLATFORM_INDEPENDENT_TESTS)
Shell->>Shell: run platform-dependent tests excluding matrix.skips
Shell->>Shell: run env-polluter checks using matrix.env_polluting_tests
Shell-->>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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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.
ShaharNaveh
force-pushed
the
CLN-ci-matrix
branch
from
bd21154 to
36fed7d
Compare
Contributor
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Chores
Tests