GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||
GeezerVison is showing github.com — Update `test/libregrtest` to CPython 3.13.10 by fanninpm · Pull Request #6410 · RustPython/RustPython Conversation
|
📝 WalkthroughWalkthroughCI workflow enhanced to detect and mitigate environment‑polluting tests by adding per‑OS polluter groups, setting skip flags for standard runs, and introducing per‑OS verification loops that retry suspect polluter tests up to 10 times and fail with remediation guidance if pollution persists. (33 words) Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant Runner as Job Runner
participant Test as Test Executor
participant Verifier as Pollution Verifier
GH->>Runner: start job (env includes ENV_POLLUTING_* groups)
Runner->>Test: run tests (env: RUSTPYTHON_SKIP_ENV_POLLUTERS=true, per-test timeout)
Test-->>Runner: exit code (0 / 1 / 3)
note right of Runner: exit code 3 = polluter detected
alt polluter detected (exit 3)
Runner->>Verifier: run ENV_POLLUTING_COMMON + ENV_POLLUTING_OS tests (retry up to 10, 15min overall)
loop up to 10 attempts
Verifier->>Test: run specific polluter test (600s timeout)
Test-->>Verifier: exit code (0 / 1 / 3)
alt exit code 3
Verifier-->Verifier: still polluting → retry
else
Verifier-->Runner: polluter cleared for that test
end
end
alt any still polluting after retries
Verifier-->>GH: fail job and print remediation guidance
else
Verifier-->>GH: continue pipeline
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
a5a599f to
2a0a591
Compare
2a0a591 to
80ddfb6
Compare
d59be8b to
8c74805
Compare
8c74805 to
bff54a5
Compare
bff54a5 to
a4cf244
Compare
fanninpm
commented
Dec 30, 2025
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
OUTDATED!I will write a new comment detailing the status over the weekend. All platformsTests failing
Tests altering the execution environment when they're not supposed to (might be something that we need to change in
|
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
e629b6b to
014cb12
Compare
|
@fanninpm awesome to see this is green. is this done? Do you have idea what affects fail_env_changed ? (though we can fix it later) |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
9d7d65a to
4c76eef
Compare
Not anymore 😕
At the time you wrote your comment, it needed a rebase due to #6506.
I currently have no idea, other than possibly the "Disable saved_test_environment context manager" and the "Disable support.record_original_stdout()" commits. I'm rewriting the skip logic so that one can detect if a test is no longer polluting the environment. |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
b435f6d to
7bf9c34
Compare
7540fe2 to
7445b27
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/ci.yaml:
- Around line 178-181: The "Test example projects" step's multi-line run value
is missing a block scalar indicator causing YAML to join the two cargo commands
into one invalid line; change the step to use a block scalar (e.g., replace the
single-line run with a block scalar like `run: |`) and place each command on its
own line so both `cargo run --manifest-path
example_projects/barebone/Cargo.toml` and `cargo run --manifest-path
example_projects/frozen_stdlib/Cargo.toml` execute separately under the "Test
example projects" step.
🧹 Nitpick comments (1)
.github/workflows/ci.yaml (1)
322-404: Consider extracting repeated pollution-check logic to reduce duplication.These three blocks (Linux, macOS, Windows) are nearly identical—only the OS condition and env var names differ. This ~80 lines of duplication could be consolidated into a reusable composite action or a parameterized shell script that accepts the OS-specific env var as input.
♻️ Example approach: extract to a script
Create a script
scripts/check_env_polluters.sh:#!/bin/bash # Usage: check_env_polluters.sh <os_name> <test_list> OS_NAME="$1" shift for thing in "$@"; do for i in $(seq 1 10); do set +e target/release/rustpython -m test -j 1 --slowest --fail-env-changed --timeout 600 -v "${thing}" exit_code=$? set -e if [ ${exit_code} -eq 3 ]; then echo "Test ${thing} polluted the environment on attempt ${i}." break fi done if [ ${exit_code} -ne 3 ]; then echo "Test ${thing} is no longer polluting the environment after ${i} attempts!" echo "Please remove ${thing} from ENV_POLLUTING_TESTS in '.github/workflows/ci.yaml'." if [ ${exit_code} -ne 0 ]; then echo "Test ${thing} failed with exit code ${exit_code}." fi exit 1 fi doneThen in the workflow:
- if: runner.os == 'Linux' name: run cpython tests to check if env polluters have stopped polluting run: bash scripts/check_env_polluters.sh Linux ${{ env.ENV_POLLUTING_TESTS_COMMON }} ${{ env.ENV_POLLUTING_TESTS_LINUX }} timeout-minutes: 15
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (46)
Lib/test/__main__.pyis excluded by!Lib/**Lib/test/_test_eintr.pyis excluded by!Lib/**Lib/test/autotest.pyis excluded by!Lib/**Lib/test/libregrtest/__init__.pyis excluded by!Lib/**Lib/test/libregrtest/cmdline.pyis excluded by!Lib/**Lib/test/libregrtest/filter.pyis excluded by!Lib/**Lib/test/libregrtest/findtests.pyis excluded by!Lib/**Lib/test/libregrtest/logger.pyis excluded by!Lib/**Lib/test/libregrtest/main.pyis excluded by!Lib/**Lib/test/libregrtest/mypy.iniis excluded by!Lib/**Lib/test/libregrtest/pgo.pyis excluded by!Lib/**Lib/test/libregrtest/refleak.pyis excluded by!Lib/**Lib/test/libregrtest/result.pyis excluded by!Lib/**Lib/test/libregrtest/results.pyis excluded by!Lib/**Lib/test/libregrtest/run_workers.pyis excluded by!Lib/**Lib/test/libregrtest/runtest.pyis excluded by!Lib/**Lib/test/libregrtest/runtest_mp.pyis excluded by!Lib/**Lib/test/libregrtest/runtests.pyis excluded by!Lib/**Lib/test/libregrtest/save_env.pyis excluded by!Lib/**Lib/test/libregrtest/setup.pyis excluded by!Lib/**Lib/test/libregrtest/single.pyis excluded by!Lib/**Lib/test/libregrtest/testresult.pyis excluded by!Lib/**Lib/test/libregrtest/tsan.pyis excluded by!Lib/**Lib/test/libregrtest/utils.pyis excluded by!Lib/**Lib/test/libregrtest/win_utils.pyis excluded by!Lib/**Lib/test/libregrtest/worker.pyis excluded by!Lib/**Lib/test/test_builtin.pyis excluded by!Lib/**Lib/test/test_concurrent_futures/test_process_pool.pyis excluded by!Lib/**Lib/test/test_concurrent_futures/test_thread_pool.pyis excluded by!Lib/**Lib/test/test_concurrent_futures/test_wait.pyis excluded by!Lib/**Lib/test/test_ftplib.pyis excluded by!Lib/**Lib/test/test_importlib/resources/test_resource.pyis excluded by!Lib/**Lib/test/test_io.pyis excluded by!Lib/**Lib/test/test_logging.pyis excluded by!Lib/**Lib/test/test_multiprocessing_fork/test_manager.pyis excluded by!Lib/**Lib/test/test_multiprocessing_fork/test_misc.pyis excluded by!Lib/**Lib/test/test_multiprocessing_fork/test_processes.pyis excluded by!Lib/**Lib/test/test_multiprocessing_fork/test_threads.pyis excluded by!Lib/**Lib/test/test_multiprocessing_forkserver/test_processes.pyis excluded by!Lib/**Lib/test/test_multiprocessing_forkserver/test_threads.pyis excluded by!Lib/**Lib/test/test_multiprocessing_spawn/test_processes.pyis excluded by!Lib/**Lib/test/test_multiprocessing_spawn/test_threads.pyis excluded by!Lib/**Lib/test/test_regrtest.pyis excluded by!Lib/**Lib/test/test_ssl.pyis excluded by!Lib/**Lib/test/test_threading.pyis excluded by!Lib/**Lib/test/test_urllib2net.pyis excluded by!Lib/**
📒 Files selected for processing (1)
.github/workflows/ci.yaml
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-11T09:35:43.571Z
Learnt from: ShaharNaveh
Repo: RustPython/RustPython PR: 5947
File: .github/workflows/ci.yaml:116-116
Timestamp: 2025-07-11T09:35:43.571Z
Learning: GitHub Actions timeout-minutes field supports expressions that evaluate to integers, such as `${{ contains(matrix.os, 'windows') && 40 || 30 }}`, which will conditionally set different timeout values based on the runner OS.
Applied to files:
.github/workflows/ci.yaml
📚 Learning: 2025-12-27T14:03:49.034Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: Applies to **/*.rs : Always run clippy to lint code (`cargo clippy`) before completing tasks and fix any warnings or lints introduced by changes
Applied to files:
.github/workflows/ci.yaml
📚 Learning: 2025-12-27T14:03:49.034Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: Applies to **/*.rs : Follow the default rustfmt code style by running `cargo fmt` to format Rust code
Applied to files:
.github/workflows/ci.yaml
🔇 Additional comments (4)
.github/workflows/ci.yaml (4)
108-125: LGTM - Good separation of OS-specific polluting tests.The structure cleanly separates common polluters from platform-specific ones. Minor note:
ENV_POLLUTING_TESTS_WINDOWSbeing empty with>-followed by a comment works but could be clearer with an explicit empty value comment for future maintainers.
296-319: LGTM - Consistent environment variable usage across platforms.The
RUSTPYTHON_SKIP_ENV_POLLUTERSenvironment variable is applied uniformly to all platform test steps, providing a clean mechanism to skip polluting tests during standard runs.
424-469: LGTM - Formatting normalization.Minor formatting adjustments (indentation and quoting) improve consistency.
481-493: LGTM - Improved comment readability.Breaking the MIRIFLAGS comment across multiple lines improves clarity.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/ci.yaml (2)
351-405: Consider extracting duplicated verification logic (optional).The three verification steps (Linux, macOS, Windows) share nearly identical bash logic, differing only in the platform-specific environment variables. While this duplication is acceptable in CI YAML and keeps each step explicit, you could optionally extract this to a composite action or shell script to reduce maintenance burden.
Example: Extract to a script
Create
.github/scripts/verify-polluters.sh:#!/bin/bash # Usage: verify-polluters.sh "test1 test2..." "PLATFORM_NAME" POLLUTERS="$1" PLATFORM="$2" for thing in ${POLLUTERS}; do for i in $(seq 1 10); do set +e target/release/rustpython -m test -j 1 --slowest --fail-env-changed --timeout 600 -v ${thing} exit_code=$? set -e if [ ${exit_code} -eq 3 ]; then echo "Test ${thing} polluted the environment on attempt ${i}." break fi done if [ ${exit_code} -ne 3 ]; then echo "Test ${thing} is no longer polluting the environment after ${i} attempts!" echo "Please remove ${thing} from ENV_POLLUTING_TESTS_COMMON or ENV_POLLUTING_TESTS_${PLATFORM} in '.github/workflows/ci.yaml'." echo "Please also remove the skip decorators that include the word 'POLLUTERS' in ${thing}." if [ ${exit_code} -ne 0 ]; then echo "Test ${thing} failed with exit code ${exit_code}." echo "Please investigate which test item in ${thing} is failing and either mark it as an expected failure or a skip." fi exit 1 fi doneThen call it from each step:
run: bash .github/scripts/verify-polluters.sh "${{ env.ENV_POLLUTING_TESTS_COMMON }} ${{ env.ENV_POLLUTING_TESTS_LINUX }}" "LINUX"
323-405: Consider CI time impact of verification steps.The three verification steps add up to 45 minutes of cumulative testing time across platforms. While this overhead is necessary to automatically detect when polluter tests are fixed, consider:
- Current approach (every run): Ensures prompt detection but adds consistent overhead
- Alternative (scheduled/manual): Could run verification steps on a schedule (e.g., nightly) or manual trigger to reduce PR CI time
The current approach is valid if prompt detection is prioritized over CI speed. Given the active bisecting work mentioned in the PR description, running on every CI execution makes sense during this phase.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yaml
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-11T09:35:43.571Z
Learnt from: ShaharNaveh
Repo: RustPython/RustPython PR: 5947
File: .github/workflows/ci.yaml:116-116
Timestamp: 2025-07-11T09:35:43.571Z
Learning: GitHub Actions timeout-minutes field supports expressions that evaluate to integers, such as `${{ contains(matrix.os, 'windows') && 40 || 30 }}`, which will conditionally set different timeout values based on the runner OS.
Applied to files:
.github/workflows/ci.yaml
📚 Learning: 2025-12-27T14:03:49.034Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: Applies to **/*.rs : Always run clippy to lint code (`cargo clippy`) before completing tasks and fix any warnings or lints introduced by changes
Applied to files:
.github/workflows/ci.yaml
📚 Learning: 2025-12-27T14:03:49.034Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: Applies to **/*.rs : Follow the default rustfmt code style by running `cargo fmt` to format Rust code
Applied to files:
.github/workflows/ci.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run rust tests (windows-2025)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (windows-2025)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run tests under miri
- GitHub Check: Check the WASM package and demo
🔇 Additional comments (4)
.github/workflows/ci.yaml (4)
108-124: Verify Windows polluters list is intentionally empty.The
ENV_POLLUTING_TESTS_WINDOWSlist is empty while Linux and macOS have several entries. Based on the PR description mentioning ongoing Windows bisecting work, is this intentional or does it indicate work in progress?
297-320: LGTM: Clean separation between normal runs and polluter verification.The
RUSTPYTHON_SKIP_ENV_POLLUTERSflag correctly skips known polluters during regular test runs while the dedicated verification steps (added later) specifically test those polluters. This is a sound architectural choice.
136-136: LGTM: Formatting improvements enhance consistency.The formatting changes throughout the file improve readability and maintain consistency in the workflow structure without affecting functionality.
Also applies to: 167-167, 179-182, 432-432, 469-469, 484-485, 492-494
323-349: The exit code 3 assumption in the workflow is correct.Exit code 3 is indeed the correct signal from CPython's test framework when
--fail-env-changeddetects environment pollution. This is confirmed in CPython's/usr/lib/python3.11/test/libregrtest/main.pywhere the code explicitly returnssys.exit(3)when both the--fail-env-changedflag is set and environment changes are detected. The workflow logic correctly relies on this exit code to determine if a test is still polluting the environment.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
youknowone
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much!
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
Uh oh!
There was an error while loading. Please reload this page.
Assignees
Labels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
3 participants
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.