| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
📝 Walkthrough
WalkthroughCI 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)
✏️ 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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
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 libregrtest itself)
Tests being skipped
Linux onlyTests 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
macOS onlyTests altering the execution environment when they're not supposed to (might be something that we need to change in libregrtest itself)
Tests being skipped
windows onlyTests being skipped
|
Sorry, something went wrong.
|
@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) |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agentsIn @.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.
.github/workflows/ci.yaml (1)📜 Review details322-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 scriptCreate 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
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between e181482 and 7445b27.
⛔ Files ignored due to path filters (46)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:
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:
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 (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.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2).github/workflows/ci.yaml (2)📜 Review details351-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 scriptCreate .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.
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between 7445b27 and 1dcded4.
📒 Files selected for processing (1)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:
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:
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 (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.
Sorry, something went wrong.
There was a problem hiding this comment.
Thank you so much!
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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
✏️ Tip: You can customize this high-level summary in your review settings.