Skip to content

Conversation

@fanninpm
Copy link
Contributor

@fanninpm fanninpm commented Dec 12, 2025

  • Update test/libregrtest to CPython 3.13.10

  • Re-add try block around platform and encodings info

  • Disable WindowsLoadTracker on Windows

  • Disable struct._clearcache()

  • Disable saved_test_environment context manager

  • Disable support.record_original_stdout()

  • faulthandler.dump_traceback_later()

  • inspect._shadowed_dict_from_weakref_mro_tuple() Update dataclasses, inspect from CPython 3.13 #5377

Summary by CodeRabbit

  • Chores
    • Improved CI to detect and verify post-test environment pollution with repeated, OS-specific checks (Linux/macOS/Windows).
    • Added environment-variable toggles to enable or skip pollution verification for relevant jobs.
    • Introduced retry loops (up to 10 attempts), per-test and overall timeouts, and clearer guidance when pollution persists.
    • Normalized CI command formatting and updated minor consistency/formatting details for stability.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 12, 2025

📝 Walkthrough

Walkthrough

CI 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

Cohort / File(s) Change Summary
CI workflow
\.github/workflows/ci.yaml
Added env vars ENV_POLLUTING_TESTS_COMMON, ENV_POLLUTING_TESTS_LINUX, ENV_POLLUTING_TESTS_MACOS, ENV_POLLUTING_TESTS_WINDOWS. Introduced per‑OS verification loops (Linux/macOS/Windows) that retry up to 10 times, treat exit code 3 as polluter detected, use 15‑minute overall timeouts and 600s per run, set RUSTPYTHON_SKIP_ENV_POLLUTERS: true for normal runs, and standardized command formatting/indentation.

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
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested reviewers

  • youknowone

Poem

🐰 I hopped through workflows with a twitch and a cheer,

I sniffed out stray envs that lingered near.
Ten little retries, I batted my ears,
I nudged at the polluters until skies were clear,
Now CI skips soot — carrots and cheer! 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Title check ⚠️ Warning The PR title claims to update test/libregrtest to CPython 3.13.10, but the raw_summary shows changes to .github/workflows/ci.yaml involving environment-polluter checks, not libregrtest updates. Update the title to accurately reflect the main changes, such as 'Add environment-polluter verification checks to CI pipeline' or similar.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ 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.

❤️ Share

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

@fanninpm
Copy link
Contributor Author

fanninpm commented Dec 30, 2025

OUTDATED!

I will write a new comment detailing the status over the weekend.

All platforms

Tests failing

Tests altering the execution environment when they're not supposed to (might be something that we need to change in libregrtest itself)

Tests being skipped

  • test.test_multiprocessing_fork.test_manager (needs a functioning sem_open implementation)
  • test.test_multiprocessing_fork.test_misc (needs a functioning sem_open implementation)
  • test.test_multiprocessing_fork.test_processes (needs a functioning sem_open implementation)
  • test.test_multiprocessing_fork.test_threads (needs a functioning sem_open implementation)
  • test.test_multiprocessing_forkserver.test_manager (needs a functioning sem_open implementation)
  • test.test_multiprocessing_forkserver.test_misc (needs a functioning sem_open implementation)
  • test.test_multiprocessing_forkserver.test_processes (needs a functioning sem_open implementation)
  • test.test_multiprocessing_forkserver.test_threads (needs a functioning sem_open implementation)
  • test.test_multiprocessing_spawn.test_manager (needs a functioning sem_open implementation)
  • test.test_multiprocessing_spawn.test_misc (needs a functioning sem_open implementation)
  • test.test_multiprocessing_spawn.test_processes (needs a functioning sem_open implementation)
  • test.test_multiprocessing_spawn.test_threads (needs a functioning sem_open implementation)
  • test_android (Android-only)
  • test_audit (needs sys.audit)
  • test_bytes (needs _testlimitedcapi)
  • test_devpoll (Solaris-exclusive)
  • test_kqueue (BSD-exclusive)
  • test_multiprocessing_main_handling (needs a functioning sem_open implementation)
  • test_opcache (cPython-specific)
  • test_wait3 (needs os.wait3)
  • test_wait4 (needs os.wait4)

Linux only

Tests failing

  • test_eintr (used to be skipped due to the lack of the walltime resource)
  • test_io
  • [ ] test_socket (likely only on my machine 😕: TypeError: AF_CAN address must be a tuple (interface,) or (interface, addr))
  • [ ] test_sysconfig (likely only on my machine 😕: platlib doesn't match expected value)
  • test_urllib2net
  • test_xmlrpc

Tests altering the execution environment when they're not supposed to (might be something that we need to change in libregrtest itself)

  • test_file_eintr
  • test_subprocess (also on macOS)

Tests being skipped

  • [ ] test_sqlite3 (i wasn't giving cargo the sqlite feature flag)
  • test_winapi (windows-only)
  • test_winreg (windows-only)

macOS only

Tests altering the execution environment when they're not supposed to (might be something that we need to change in libregrtest itself)

  • test_subprocess (also on Linux)

Tests being skipped

  • test_epoll (Linux-only)
  • test_ioctl (needs /dev/tty access)
  • test_winapi (windows-only)
  • test_winreg (windows-only)

windows only

Tests being skipped

  • test_epoll (Linux-only)
  • test_fcntl (Unix-only)
  • test_grp (Unix-only except WASI/Android/iOS)
  • test_ioctl (Unix-only)
  • test_openpty (Unix-only except WASI)
  • test_poll (likely not supported)
  • test_pty (Unix-only)
  • test_pwd (Unix-only except WASI/iOS)
  • test_resource (Unix-only)
  • test_syslog (Unix-only except WASI/iOS)

@fanninpm fanninpm force-pushed the update-libregrtest branch 4 times, most recently from e629b6b to 014cb12 Compare January 1, 2026 16:48
@youknowone
Copy link
Member

@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)

@fanninpm fanninpm force-pushed the update-libregrtest branch from 9d7d65a to 4c76eef Compare January 3, 2026 02:31
@fanninpm
Copy link
Contributor Author

fanninpm commented Jan 3, 2026

@fanninpm awesome to see this is green.

Not anymore 😕

is this done?

At the time you wrote your comment, it needed a rebase due to #6506.

Do you have idea what affects fail_env_changed ? (though we can fix it later)

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.

@fanninpm fanninpm force-pushed the update-libregrtest branch from b435f6d to 7bf9c34 Compare January 3, 2026 16:29
@fanninpm fanninpm marked this pull request as ready for review January 11, 2026 03:43
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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
done

Then 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

📥 Commits

Reviewing files that changed from the base of the PR and between e181482 and 7445b27.

⛔ Files ignored due to path filters (46)
  • Lib/test/__main__.py is excluded by !Lib/**
  • Lib/test/_test_eintr.py is excluded by !Lib/**
  • Lib/test/autotest.py is excluded by !Lib/**
  • Lib/test/libregrtest/__init__.py is excluded by !Lib/**
  • Lib/test/libregrtest/cmdline.py is excluded by !Lib/**
  • Lib/test/libregrtest/filter.py is excluded by !Lib/**
  • Lib/test/libregrtest/findtests.py is excluded by !Lib/**
  • Lib/test/libregrtest/logger.py is excluded by !Lib/**
  • Lib/test/libregrtest/main.py is excluded by !Lib/**
  • Lib/test/libregrtest/mypy.ini is excluded by !Lib/**
  • Lib/test/libregrtest/pgo.py is excluded by !Lib/**
  • Lib/test/libregrtest/refleak.py is excluded by !Lib/**
  • Lib/test/libregrtest/result.py is excluded by !Lib/**
  • Lib/test/libregrtest/results.py is excluded by !Lib/**
  • Lib/test/libregrtest/run_workers.py is excluded by !Lib/**
  • Lib/test/libregrtest/runtest.py is excluded by !Lib/**
  • Lib/test/libregrtest/runtest_mp.py is excluded by !Lib/**
  • Lib/test/libregrtest/runtests.py is excluded by !Lib/**
  • Lib/test/libregrtest/save_env.py is excluded by !Lib/**
  • Lib/test/libregrtest/setup.py is excluded by !Lib/**
  • Lib/test/libregrtest/single.py is excluded by !Lib/**
  • Lib/test/libregrtest/testresult.py is excluded by !Lib/**
  • Lib/test/libregrtest/tsan.py is excluded by !Lib/**
  • Lib/test/libregrtest/utils.py is excluded by !Lib/**
  • Lib/test/libregrtest/win_utils.py is excluded by !Lib/**
  • Lib/test/libregrtest/worker.py is excluded by !Lib/**
  • Lib/test/test_builtin.py is excluded by !Lib/**
  • Lib/test/test_concurrent_futures/test_process_pool.py is excluded by !Lib/**
  • Lib/test/test_concurrent_futures/test_thread_pool.py is excluded by !Lib/**
  • Lib/test/test_concurrent_futures/test_wait.py is excluded by !Lib/**
  • Lib/test/test_ftplib.py is excluded by !Lib/**
  • Lib/test/test_importlib/resources/test_resource.py is excluded by !Lib/**
  • Lib/test/test_io.py is excluded by !Lib/**
  • Lib/test/test_logging.py is excluded by !Lib/**
  • Lib/test/test_multiprocessing_fork/test_manager.py is excluded by !Lib/**
  • Lib/test/test_multiprocessing_fork/test_misc.py is excluded by !Lib/**
  • Lib/test/test_multiprocessing_fork/test_processes.py is excluded by !Lib/**
  • Lib/test/test_multiprocessing_fork/test_threads.py is excluded by !Lib/**
  • Lib/test/test_multiprocessing_forkserver/test_processes.py is excluded by !Lib/**
  • Lib/test/test_multiprocessing_forkserver/test_threads.py is excluded by !Lib/**
  • Lib/test/test_multiprocessing_spawn/test_processes.py is excluded by !Lib/**
  • Lib/test/test_multiprocessing_spawn/test_threads.py is excluded by !Lib/**
  • Lib/test/test_regrtest.py is excluded by !Lib/**
  • Lib/test/test_ssl.py is excluded by !Lib/**
  • Lib/test/test_threading.py is excluded by !Lib/**
  • Lib/test/test_urllib2net.py is 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_WINDOWS being 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_POLLUTERS environment 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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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
done

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

  1. Current approach (every run): Ensures prompt detection but adds consistent overhead
  2. 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

📥 Commits

Reviewing files that changed from the base of the PR and between 7445b27 and 1dcded4.

📒 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_WINDOWS list 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_POLLUTERS flag 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-changed detects environment pollution. This is confirmed in CPython's /usr/lib/python3.11/test/libregrtest/main.py where the code explicitly returns sys.exit(3) when both the --fail-env-changed flag 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.

Copy link
Member

@youknowone youknowone left a 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!

@youknowone youknowone merged commit 1d95a04 into RustPython:main Jan 11, 2026
13 checks passed
@fanninpm fanninpm deleted the update-libregrtest branch January 11, 2026 18:43
This was referenced Jan 11, 2026
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