Set python args to be like CPython in CI#8181
Conversation
📝 WalkthroughWalkthroughThe CI workflow now sets ChangesCI workflow updates
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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. 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)
361-374: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winMove workflow expressions out of
run:to satisfy zizmor.The new flags are fine, but Lines 363 and 374 still interpolate
${{ ... }}directly into the shell body. The matrix values shown here are hardcoded, so this is not an exploitable injection path today, but zizmor will keep flagging the step until those expressions are moved intoenv:(or otherwise removed fromrun:).Suggested shape
- name: Run CPython tests + env: + RUSTPYTHON_SKIP_ENV_POLLUTERS: true + EXTRA_TEST_ARGS: ${{ join(matrix.extra_test_args, ' ') }} + SKIPS: ${{ join(matrix.skips, ' ') }} run: | - target/release/rustpython -u -W error -bb -E -m test -j ${{ steps.cores.outputs.cores }} ${{ join(matrix.extra_test_args, ' ') }} --slowest --fail-env-changed --timeout 600 -x ${{ env.FLAKY_MP_TESTS }} ${{ join(matrix.skips, ' ') }} + read -r -a extra_test_args <<< "$EXTRA_TEST_ARGS" + read -r -a skips <<< "$SKIPS" + target/release/rustpython -u -W error -bb -E -m test -j ${{ steps.cores.outputs.cores }} "${extra_test_args[@]}" --slowest --fail-env-changed --timeout 600 -x ${{ env.FLAKY_MP_TESTS }} "${skips[@]}" timeout-minutes: ${{ matrix.timeout }} - env: - RUSTPYTHON_SKIP_ENV_POLLUTERS: trueAs per coding guidelines, "If you modify any file under
.github/workflows/, the change must pass a zizmor scan in CI."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yaml around lines 361 - 374, Move the workflow expressions out of the shell bodies in the CPython test steps so zizmor stops flagging them. In the “Run CPython tests” and “Run flaky MP CPython tests” steps, take the `${{ ... }}` values currently embedded in `run:` (for example the matrix args and flaky test flags) and pass them through `env:` instead, then reference the environment variables inside the commands. Keep the existing step names and test commands in `.github/workflows/ci.yaml`, but ensure `run:` contains no direct GitHub Actions expressions.Sources: Coding guidelines, Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/ci.yaml:
- Around line 361-374: Move the workflow expressions out of the shell bodies in
the CPython test steps so zizmor stops flagging them. In the “Run CPython tests”
and “Run flaky MP CPython tests” steps, take the `${{ ... }}` values currently
embedded in `run:` (for example the matrix args and flaky test flags) and pass
them through `env:` instead, then reference the environment variables inside the
commands. Keep the existing step names and test commands in
`.github/workflows/ci.yaml`, but ensure `run:` contains no direct GitHub Actions
expressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 0d89bebf-1572-4dd8-99a2-ba243a324d40
📒 Files selected for processing (1)
.github/workflows/ci.yaml
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)
374-399: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep
-W errorout of these special-purpose retry loops.Line 399’s loop only recognizes exit code
3as “still polluting”. With-W error, a warning now fails with a different status, so the job can incorrectly report “no longer polluting” even though it never saw a clean run. The same flag on Line 374 also turns deterministic warning failures into five pointless retries.Suggested fix
- target/release/rustpython -u -W error -bb -E -m test -j 1 ${{ join(matrix.extra_test_args, ' ') }} --slowest --fail-env-changed --timeout 600 ${{ env.FLAKY_MP_TESTS }} + target/release/rustpython -u -bb -E -m test -j 1 ${{ join(matrix.extra_test_args, ' ') }} --slowest --fail-env-changed --timeout 600 ${{ env.FLAKY_MP_TESTS }}- target/release/rustpython -u -W error -bb -E -m test -j 1 --slowest --fail-env-changed --timeout 600 "${thing}" + target/release/rustpython -u -bb -E -m test -j 1 --slowest --fail-env-changed --timeout 600 "${thing}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yaml around lines 374 - 399, Remove the `-W error` flag from the retry-based test invocations in the CI workflow so these loops don’t turn warnings into different failure modes; update the `rustpython` command used in the retry loop and the cpython env-polluter check to rely on the existing exit-code logic instead. Keep the special handling in the retry loops (`status`, `--fail-env-changed`, and the `thing`/`TARGETS` iterations) unchanged so exit code `3` remains the only signal for continued pollution.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/ci.yaml:
- Around line 374-399: Remove the `-W error` flag from the retry-based test
invocations in the CI workflow so these loops don’t turn warnings into different
failure modes; update the `rustpython` command used in the retry loop and the
cpython env-polluter check to rely on the existing exit-code logic instead. Keep
the special handling in the retry loops (`status`, `--fail-env-changed`, and the
`thing`/`TARGETS` iterations) unchanged so exit code `3` remains the only signal
for continued pollution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 8eda90b1-ad3c-4957-81cc-a08d767a915f
📒 Files selected for processing (1)
.github/workflows/ci.yaml
ef40c7e to
bd809ce
Compare
| - name: Run CPython tests | ||
| run: | | ||
| target/release/rustpython -m test -j ${{ steps.cores.outputs.cores }} ${{ join(matrix.extra_test_args, ' ') }} --slowest --fail-env-changed --timeout 600 -v -x ${{ env.FLAKY_MP_TESTS }} ${{ join(matrix.skips, ' ') }} | ||
| target/release/rustpython -u -m test --slow-ci -j ${{ steps.cores.outputs.cores }} ${{ join(matrix.extra_test_args, ' ') }} -x ${{ env.FLAKY_MP_TESTS }} ${{ join(matrix.skips, ' ') }} |
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] lib: cpython/Lib/future.py dependencies:
dependent tests: (35 tests)
Legend:
|
| exec("from __future__ import unicode_literals; x = ''", {}, scope) | ||
| self.assertIsInstance(scope["x"], str) | ||
|
|
||
| @unittest.expectedFailure # TODO: RUSTPYTHON; barry_as_FLUFL (<> operator) not supported |
There was a problem hiding this comment.
It seems that adding --dont-add-python-opts makes this pass 😟
There was a problem hiding this comment.
Yeah, that does seem a bit suspicious. Any idea why?
Summary
I've recently made some patches to CPython, and noticed that their CI output is less noisy. made it easier to find the failing tests
lmk what you think
Summary by CodeRabbit