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 — Fix fix_test.py by youknowone · Pull Request #6415 · RustPython/RustPython Conversation
|
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 |
All reactions
Sorry, something went wrong.
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: 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.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| import itertools | ||
| import platform | ||
| import sys |
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.
Remove unused imports (flake8 F401) to keep the script clean.
itertools (Line 19) and platform (Line 20) are unused per static analysis; sys (Line 21) is used.
import argparse import ast -import itertools -import platform import sys from pathlib import Path
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import itertools | |
| import platform | |
| import sys | |
| import argparse | |
| import ast | |
| import sys | |
| from pathlib import Path |
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 19-19: 'itertools' imported but unused
(F401)
[error] 20-20: 'platform' imported but unused
(F401)
🤖 Prompt for AI Agents
In scripts/fix_test.py around lines 19 to 21, the imports include unused modules
causing flake8 F401; remove the unused imports itertools and platform and keep
only the used import sys (i.e., replace the three-import block with a single
import for sys), ensuring any references to those removed modules are also
removed or updated if present elsewhere in the file.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| def path_to_test(path) -> list[str]: | ||
| return path.split(".")[2:] | ||
| # path format: test.module_name[.submodule].ClassName.test_method | ||
| # We need [ClassName, test_method] - always the last 2 elements | ||
| parts = path.split(".") | ||
| return parts[-2:] # Get class name and method name | ||
|
|
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.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the structure of the repository and locate the file find . -name "fix_test.py" -type f
Repository: RustPython/RustPython
Length of output: 86
🏁 Script executed:
# Get the file size to see if we can read it entirely
wc -l scripts/fix_test.pyRepository: RustPython/RustPython
Length of output: 88
🏁 Script executed:
# Read the file around the mentioned lines 98-103 and 169-182 cat -n scripts/fix_test.py | head -200
Repository: RustPython/RustPython
Length of output: 8340
🏁 Script executed:
# Search for all calls to path_to_test in the file rg "path_to_test" scripts/fix_test.py -n
Repository: RustPython/RustPython
Length of output: 157
🏁 Script executed:
# Let's also verify the unittest output format assumption by checking if there are any tests or documentation find . -name "*.py" -type f -exec grep -l "Run tests sequentially" {} \; 2>/dev/null | head -5
Repository: RustPython/RustPython
Length of output: 86
Bug confirmed: path_to_test() misidentifies test class and method names.
The function discards test.name (the actual method name) and returns the wrong class name. Given unittest's output format test_method (package.module.ClassName) ... RESULT, the parsing at lines 78-88 correctly extracts test.name = "test_method" and test.path = "package.module.ClassName". However, path_to_test(test.path) splits the path and returns parts[-2:] (e.g., ["module", "ClassName"] instead of ["ClassName", "test_method"]), causing find_test_lineno() to search for the wrong class name and use the class name as the method name. This prevents test discovery and causes marked tests to be skipped.
The suggested fix—passing both test.path and test.name to derive [class_name, method_name]—correctly addresses the root cause.
🤖 Prompt for AI Agents
In scripts/fix_test.py around lines 98 to 103, path_to_test currently only takes
the dotted path and returns parts[-2:], which misidentifies class and method;
update path_to_test to accept both the dotted path and the test.name (method
name) and return [ClassName, method_name] (extract the last element of the path
as ClassName and use the provided test.name for method_name), then update all
callers to pass both values so find_test_lineno searches for the correct class
and method.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| def apply_modifications(file: str, modifications: list[tuple[int, int]]) -> str: | ||
| """Apply all modifications in reverse order to avoid line number offset issues.""" | ||
| lines = file.splitlines() | ||
| fixture = "@unittest.expectedFailure" | ||
| # Sort by line number in descending order | ||
| modifications.sort(key=lambda x: x[0], reverse=True) | ||
| for lineno, col_offset in modifications: | ||
| indent = " " * col_offset | ||
| lines.insert(lineno - 1, indent + fixture) | ||
| lines.insert(lineno - 1, indent + "# TODO: RUSTPYTHON") | ||
| return "\n".join(lines) |
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.
apply_modifications() should avoid double-inserting when a test is already marked.
Re-running the script will currently stack # TODO: RUSTPYTHON and @unittest.expectedFailure repeatedly. Consider checking the preceding lines before inserting.
def apply_modifications(file: str, modifications: list[tuple[int, int]]) -> str:
@@
for lineno, col_offset in modifications:
indent = " " * col_offset
- lines.insert(lineno - 1, indent + fixture)
- lines.insert(lineno - 1, indent + "# TODO: RUSTPYTHON")
+ insert_at = lineno - 1
+ prev = lines[max(0, insert_at - 3):insert_at]
+ if any(l.strip() == fixture for l in prev):
+ continue
+ lines.insert(insert_at, indent + fixture)
+ lines.insert(insert_at, indent + "# TODO: RUSTPYTHON")
return "\n".join(lines)🤖 Prompt for AI Agents
In scripts/fix_test.py around lines 127 to 137, apply_modifications currently
inserts the fixture and TODO unconditionally which causes duplicate lines on
repeated runs; modify the loop to check the existing surrounding lines before
inserting (for example examine lines[lineno-2] and lines[lineno-3] safely) and
only insert the "@unittest.expectedFailure" and the "# TODO: RUSTPYTHON" if they
are not already present (use trimmed comparisons and guard against index
errors), preserving indentation and original ordering so rerunning the script is
idempotent.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| for fn in n.body: | ||
| match fn: | ||
| case ast.FunctionDef(): | ||
| case ast.FunctionDef() | ast.AsyncFunctionDef(): |
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.
We should consider to make this script to generate the json payload lib_updater is expecting, and (and use its python API to patch the files). both are doing the same thing (ish)
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
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.
Good idea
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
No one assignedLabels
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.