Fix fix_test.py#6415
Conversation
Sorry, something went wrong.
WalkthroughThe test-fixing script undergoes substantial refactoring to replace in-place line-wise modifications with a locate-modify-apply sequence. Results parsing is updated to extract failing tests from targeted patterns, new helper functions locate test definitions and apply modifications in reverse order, and file I/O transitions to Path-based methods with Windows executable support. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
scripts/fix_test.py (3)
105-124:find_test_lineno()should iteratea.bodydirectly and avoid over-restrictive structure assumptions.
Currentast.iter_fields()+key == "body"works but is indirect; also consider catchingSyntaxErrorfromast.parse(script should warn and skip instead of crashing on a bad file).def find_test_lineno(file: str, test: list[str]) -> tuple[int, int] | None: @@ - a = ast.parse(file) - for key, node in ast.iter_fields(a): - if key == "body": - for n in node: + try: + a = ast.parse(file) + except SyntaxError: + return None + + for n in a.body: match n: case ast.ClassDef(): if len(test) == 2 and test[0] == n.name: for fn in n.body: match fn: case ast.FunctionDef() | ast.AsyncFunctionDef(): if fn.name == test[-1]: return (fn.lineno, fn.col_offset) case ast.FunctionDef() | ast.AsyncFunctionDef(): if n.name == test[0] and len(test) == 1: return (n.lineno, n.col_offset) return None
140-153: Add a timeout (and surface stderr) to avoid hanging forever on known-hanging tests.
Docs say “skip the tests that hang” but the script itself can still hang insubprocess.run(...).- result = subprocess.run( + result = subprocess.run( [rustpython_location, "-m", "test", "-v", test_name], capture_output=True, text=True, + timeout=60 * 20, )Optionally catch
subprocess.TimeoutExpiredand print a clear message with next steps.
156-188: Batch “locate → apply in reverse” flow is a good direction; just ensure the “platform/force” flags aren’t dead.
The new Path-based I/O + reverse-apply avoids offset bugs—nice. But--forceand--platformare currently unused in this file (at least in the shown code); either wire them in (e.g., allow re-marking / platform-gated TODO text) or remove them to avoid misleading CLI. Based on repo guidance, ensure the inserted markers match the project’s required patterns forLib/test/**edits (unittest.expectedFailure/unittest.skip("TODO: RustPython ...")+# TODO: RUSTPYTHON ...). Based on learnings, apply only those modifications to tests.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/fix_test.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: In most cases, Python code should not be edited; bug fixes should be made through Rust code modifications only
Follow PEP 8 style for custom Python code
Use ruff for linting Python code
Files:
scripts/fix_test.py
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to Lib/test/**/*.py : Only acceptable modifications to test files are: (1) Removing `unittest.expectedFailure` decorators and upper TODO comments when tests actually pass, (2) Adding `unittest.expectedFailure` decorators when tests cannot be fixed
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to Lib/test/**/*.py : Only acceptable modifications to test files are: (1) Removing `unittest.expectedFailure` decorators and upper TODO comments when tests actually pass, (2) Adding `unittest.expectedFailure` decorators when tests cannot be fixed
Applied to files:
scripts/fix_test.py
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to Lib/test/**/*.py : When modifying test files in `Lib/test/`, use `unittest.skip("TODO: RustPython <reason>")` or `unittest.expectedFailure` with `# TODO: RUSTPYTHON <reason>` comment to mark unsupported features
Applied to files:
scripts/fix_test.py
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to Lib/test/**/*.py : NEVER comment out or delete test code lines except for removing `unittest.expectedFailure` decorators and upper TODO comments
Applied to files:
scripts/fix_test.py
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to Lib/test/**/*.py : When a test cannot pass due to missing language features, keep it as expectedFailure and document the reason instead of modifying test code
Applied to files:
scripts/fix_test.py
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Use `cargo run -- script.py` instead of the standard `python` command to run and test Python code with RustPython
Applied to files:
scripts/fix_test.py
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Run `cargo test --workspace --exclude rustpython_wasm` to execute Rust unit tests
Applied to files:
scripts/fix_test.py
📚 Learning: 2025-09-28T22:22:55.921Z
Learnt from: reactive-firewall
Repo: RustPython/RustPython PR: 6176
File: .github/workflows/Check_Tests.yml:133-141
Timestamp: 2025-09-28T22:22:55.921Z
Learning: In the RustPython project's CI-5974-Test-RustPython-Integration action, the override-rustpython-path input is marked as required but has runtime fallback logic that defaults to RUSTPYTHONPATH environment variable or "Lib" if neither is provided, making explicit specification unnecessary in most cases.
Applied to files:
scripts/fix_test.py
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to Lib/**/*.py : Minimize modifications to CPython standard library files in the `Lib/` directory; modifications should be minimal and only to work around RustPython limitations
Applied to files:
scripts/fix_test.py
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to Lib/**/*.py : Add a `# TODO: RUSTPYTHON` comment when modifications are made to files in the `Lib/` directory
Applied to files:
scripts/fix_test.py
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to **/*.py : In most cases, Python code should not be edited; bug fixes should be made through Rust code modifications only
Applied to files:
scripts/fix_test.py
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Use the `python` command explicitly when comparing behavior with CPython or running test suites against CPython
Applied to files:
scripts/fix_test.py
🪛 Flake8 (7.3.0)
scripts/fix_test.py
[error] 19-19: 'itertools' imported but unused
(F401)
[error] 20-20: 'platform' imported but unused
(F401)
⏰ 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). (1)
- GitHub Check: Check Rust code with clippy
🔇 Additional comments (1)
scripts/fix_test.py (1)
55-95: Python code modifications not appropriate per project guidelines.While the parsing logic concern has merit (the code relies on specific output markers like "Run tests sequentially" that could be fragile), the coding guidelines explicitly state: "In most cases, Python code should not be edited; bug fixes should be made through Rust code modifications only." If the underlying test runner or output format is problematic, address the root cause in Rust rather than modifying the Python parsing code.
Sorry, something went wrong.
youknowone
merged commit ab1105a
into
RustPython:main
Dec 20, 2025
Reviewers
3 participants
with better collecting, async support and win32
I'd like to rename the script to
mark_test_failuresor something like that to shows better what it isSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.