Skip to content

Set python args to be like CPython in CI#8181

Open
ShaharNaveh wants to merge 5 commits into
RustPython:mainfrom
ShaharNaveh:ci-cpython-like-args
Open

Set python args to be like CPython in CI#8181
ShaharNaveh wants to merge 5 commits into
RustPython:mainfrom
ShaharNaveh:ci-cpython-like-args

Conversation

@ShaharNaveh

@ShaharNaveh ShaharNaveh commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

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

  • Tests
    • Updated CI CPython and flaky retry runs to use standardized test execution flags (-m test --slow-ci) for more consistent coverage.
    • Centralized timeout and related runtime settings in the job matrix to improve reliability and reduce command-level variance.
    • Adjusted the environment “polluters” verification loop to follow the same standardized test behavior.
  • Chores
    • Enabled consistent colored output in CI logs via a workflow-level setting.

@ShaharNaveh ShaharNaveh marked this pull request as ready for review June 26, 2026 17:30
@coderabbitai

coderabbitai Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: ca747555-366e-47cd-895b-dc1657211b16

📥 Commits

Reviewing files that changed from the base of the PR and between 7b47348 and 674bfa3.

📒 Files selected for processing (1)
  • .github/workflows/ci.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
  • .github/workflows/ci.yaml

📝 Walkthrough

Walkthrough

The CI workflow now sets FORCE_COLOR=1, expands matrix test arguments, and updates three CPython test invocations to use --slow-ci with revised timeout and option handling.

Changes

CI workflow updates

Layer / File(s) Summary
Workflow env and matrix args
.github/workflows/ci.yaml
The workflow sets FORCE_COLOR globally and expands the snippets_cpython matrix extra_test_args with timeout and python-opts flags.
CPython test commands
.github/workflows/ci.yaml
The main CPython run, flaky MP retry loop, and env-polluter verification command switch to --slow-ci and updated argument handling.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: youknowone

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title matches the main change: CI test commands are adjusted to use CPython-like Python args and options.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Comment thread .github/workflows/ci.yaml Fixed

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Move 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 into env: (or otherwise removed from run:).

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: true

As 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

📥 Commits

Reviewing files that changed from the base of the PR and between 0009dd6 and 16899d2.

📒 Files selected for processing (1)
  • .github/workflows/ci.yaml

Comment thread .github/workflows/ci.yaml Fixed

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Keep -W error out of these special-purpose retry loops.

Line 399’s loop only recognizes exit code 3 as “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

📥 Commits

Reviewing files that changed from the base of the PR and between 16899d2 and ef40c7e.

📒 Files selected for processing (1)
  • .github/workflows/ci.yaml

@ShaharNaveh ShaharNaveh force-pushed the ci-cpython-like-args branch from ef40c7e to bd809ce Compare June 26, 2026 18:18
@ShaharNaveh ShaharNaveh marked this pull request as draft June 26, 2026 18:18
Comment thread .github/workflows/ci.yaml Fixed
Comment thread .github/workflows/ci.yaml
- 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, ' ') }}
@github-actions

Copy link
Copy Markdown
Contributor

📦 Library Dependencies

The following Lib/ modules were modified. Here are their dependencies:

[x] lib: cpython/Lib/future.py

dependencies:

  • future

dependent tests: (35 tests)

  • future: test_flufl test_future_stmt test_generator_stop test_pydoc test_zoneinfo
    • codeop: test_codeop
      • pdb: test_pdb
      • traceback: test_asyncio test_builtin test_code_module test_contextlib test_contextlib_async test_coroutines test_dictcomps test_exceptions test_http_cookiejar test_importlib test_iter test_listcomps test_pyexpat test_setcomps test_socket test_ssl test_subprocess test_sys test_threadedtempfile test_threading test_traceback test_unittest test_with test_zipimport
    • importlib.metadata: test_importlib
    • pydoc: test_enum
      • xmlrpc.server: test_docxmlrpc test_xmlrpc

Legend:

  • [+] path exists in CPython
  • [x] up-to-date, [ ] outdated

exec("from __future__ import unicode_literals; x = ''", {}, scope)
self.assertIsInstance(scope["x"], str)

@unittest.expectedFailure # TODO: RUSTPYTHON; barry_as_FLUFL (<> operator) not supported

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that adding --dont-add-python-opts makes this pass 😟

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that does seem a bit suspicious. Any idea why?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ShaharNaveh ShaharNaveh marked this pull request as ready for review June 27, 2026 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants