Set python args to be like CPython in CI - #8181
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)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe CI workflow now sets ChangesCI workflow updates
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 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?
There was a problem hiding this comment.
So I've now debugged it, it seems that it's actually because of the FORCE_COLOR=1 environment variable 🤦♂️
IMO the fix should be done over CPython side to add @support.force_not_colorized on the test method
There was a problem hiding this comment.
python/cpython#152749 contains a bit more more information about why
| - os: macos-latest | ||
| extra_test_args: | ||
| - '-u all' | ||
| - '--timeout 600' |
There was a problem hiding this comment.
if everything requires same args, dose it still need to be extra_test_args?
There was a problem hiding this comment.
it's repeated multiple times, I think this is the easiest way to manage it for now.
we repeat this arg in 2/3 times that we run the test suite. I can "hardcode" it in all other places instead, not a big deal
There was a problem hiding this comment.
extra_test_args changes the test name. then merge queue settings also must be updated.
that's not a big deal when it occasionally changes. but it happens more than common these days. So i wish that's still an argument, but not being a part of job title.
also it makes the CI job name unnecessary longer, not a big deal itself too though.
There was a problem hiding this comment.
extra_test_args changes the test name. then merge queue settings also must be updated. that's not a big deal when it occasionally changes. but it happens more than common these days. So i wish that's still an argument, but not being a part of job title.
also it makes the CI job name unnecessary longer, not a big deal itself too though.
CPython uses an external file to manage their CI commands, we could do the same using just for example.
bevy wrote their own tool for that, we could do something similar.
I'm in favor of starting with the just approach (or similar), if you think it's a good idea.
There's another option which I'm less of a fan of, but still worth mentioning. cargo alias can also do that, but that's adding another layer without the full features that justgives us, i.e. reuse of variables. (we can't define a variable called common_args and use it everywhere).
lmk if that makes sense to you and/or worth implementing
There was a problem hiding this comment.
either way i am good as long as contributors still can copy the same CI command from web CI view to their terminal
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
Summary by CodeRabbit
-m test --slow-ci) for more consistent coverage.