preparing step to integrate fix_test into lib_updater #6698
Conversation
|
Warning Rate limit exceeded@youknowone has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 20 minutes and 2 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR refactors Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @scripts/fix_test.py:
- Around line 152-161: The current check uses a hardcoded string "/Lib/" against
str(args.quick_import) which fails on Windows backslashes; instead convert
args.quick_import to a pathlib.Path, inspect Path.parts to find the "Lib"
component (e.g., p = Path(args.quick_import); find index = p.parts.index("Lib")
if present), error if "Lib" not in p.parts, and construct lib_path as
Path(*p.parts[index:]) (so it works cross-platform); update references to
src_str, lib_marker, and lib_path accordingly and remove the string-based
"/Lib/" check.
🧹 Nitpick comments (2)
scripts/fix_test.py (2)
43-46: Unused--forceand--platformflags.These flags are parsed but not referenced anywhere in the refactored code. If they're no longer needed after removing the legacy
apply_modificationslogic, consider removing them to avoid confusion. Otherwise, document their intended use.
61-68: Mutable class attributetests = []is shared across instances.Class-level mutable defaults like
tests = []are shared across all instances. While this works in the current single-use context, it's a latent bug if the class is reused.♻️ Suggested fix
class TestResult: tests_result: str = "" - tests = [] + tests: list stdout = "" + + def __init__(self): + self.tests = []
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/fix_test.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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
**/*test*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*test*.py: NEVER comment out or delete any test code lines except for removing@unittest.expectedFailuredecorators and upper TODO comments
NEVER modify test assertions, test logic, or test data in test files
When a test cannot pass due to missing language features, keep it as expectedFailure and document the reason; do not comment it out
The only acceptable modifications to test files are: (1) Removing@unittest.expectedFailuredecorators and the upper TODO comments when tests actually pass, and (2) Adding@unittest.expectedFailuredecorators when tests cannot be fixed
Files:
scripts/fix_test.py
🧠 Learnings (7)
📓 Common learnings
Learnt from: ShaharNaveh
Repo: RustPython/RustPython PR: 6089
File: scripts/lib_updater.py:292-297
Timestamp: 2025-08-30T14:40:05.858Z
Learning: In scripts/lib_updater.py, the --inplace flag intentionally writes to orig_file (not remote_file) even though patches are applied to remote_file content. This workflow allows updating the original RustPython test file with patches applied to new upstream CPython content.
Learnt from: ShaharNaveh
Repo: RustPython/RustPython PR: 6089
File: scripts/lib_updater.py:198-202
Timestamp: 2025-09-07T05:38:31.690Z
Learning: In scripts/lib_updater.py, the iter_patches function intentionally does not handle SyntaxError from ast.parse(contents). The author confirmed this behavior is fine and intended - the tool should fail fast on unparseable files rather than silently skip processing.
📚 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 **/*test*.py : The only acceptable modifications to test files are: (1) Removing `unittest.expectedFailure` decorators and the upper TODO comments when tests actually pass, and (2) Adding `unittest.expectedFailure` decorators when tests cannot be fixed
Applied to files:
scripts/fix_test.py
📚 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 Lib/** : In the `Lib/` directory, when modifications are necessary, add a `# TODO: RUSTPYTHON` comment or use `unittest.skip('TODO: RustPython <reason>')` or `unittest.expectedFailure` with `# TODO: RUSTPYTHON <reason>` comment
Applied to files:
scripts/fix_test.py
📚 Learning: 2025-08-30T14:40:05.858Z
Learnt from: ShaharNaveh
Repo: RustPython/RustPython PR: 6089
File: scripts/lib_updater.py:292-297
Timestamp: 2025-08-30T14:40:05.858Z
Learning: In scripts/lib_updater.py, the --inplace flag intentionally writes to orig_file (not remote_file) even though patches are applied to remote_file content. This workflow allows updating the original RustPython test file with patches applied to new upstream CPython content.
Applied to files:
scripts/fix_test.py
📚 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 **/*test*.py : NEVER comment out or delete any test code lines except for removing `unittest.expectedFailure` decorators and upper TODO comments
Applied to files:
scripts/fix_test.py
📚 Learning: 2025-09-07T05:38:31.690Z
Learnt from: ShaharNaveh
Repo: RustPython/RustPython PR: 6089
File: scripts/lib_updater.py:198-202
Timestamp: 2025-09-07T05:38:31.690Z
Learning: In scripts/lib_updater.py, the iter_patches function intentionally does not handle SyntaxError from ast.parse(contents). The author confirmed this behavior is fine and intended - the tool should fail fast on unparseable files rather than silently skip processing.
Applied to files:
scripts/fix_test.py
📚 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: When testing Python code, always use RustPython instead of the standard `python` command; use `cargo run -- script.py` or `cargo run` for interactive REPL
Applied to files:
scripts/fix_test.py
🧬 Code graph analysis (1)
scripts/fix_test.py (1)
scripts/lib_updater.py (3)
apply_patches(273-283)PatchSpec(81-122)UtMethod(59-78)
⏰ 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 (5)
scripts/fix_test.py (5)
1-31: LGTM!Clear documentation for the new
--quick-importworkflow, and imports are appropriate for the refactored patch-based approach usinglib_updater.
70-117: LGTM!The result parsing and path extraction logic correctly identifies failing tests and extracts class/method names.
120-129: LGTM!Correctly builds the patch dictionary structure expected by
apply_patches. Based on learnings,lib_updaterhandles adding the TODO comment viaPatchSpec._reason.
140-145: LGTM!Adding
-u all --slowestflags provides more comprehensive test execution and timing information.
189-206: LGTM!The deduplication using a set correctly handles subtests that may produce multiple failures for the same test method. The integration with
apply_patchesfromlib_updateris correct.Note: The check at line 192 is slightly redundant since
parse_resultsalready filters to only "fail" or "error" results (line 91-92), but it's harmless and adds defensive clarity.
scripts/fix_test.py
Outdated
| if args.quick_import is not None: | ||
| src_str = str(args.quick_import) | ||
| lib_marker = "/Lib/" | ||
|
|
||
| if lib_marker not in src_str: | ||
| print(f"Error: --quick-import path must contain '/Lib/' (got: {src_str})") | ||
| sys.exit(1) | ||
|
|
||
| idx = src_str.index(lib_marker) | ||
| lib_path = Path(src_str[idx + 1:]) # Lib/test/foo.py |
There was a problem hiding this comment.
Cross-platform path separator issue.
The /Lib/ marker won't match on Windows where Path objects stringify with backslashes. Users passing cpython\Lib\test\test_foo.py would get the error despite having a valid path.
🔧 Suggested fix using platform-agnostic separator
if args.quick_import is not None:
- src_str = str(args.quick_import)
- lib_marker = "/Lib/"
+ # Normalize to forward slashes for consistent parsing
+ src_str = str(args.quick_import).replace("\\", "/")
+ lib_marker = "/Lib/"
if lib_marker not in src_str:🤖 Prompt for AI Agents
In @scripts/fix_test.py around lines 152 - 161, The current check uses a
hardcoded string "/Lib/" against str(args.quick_import) which fails on Windows
backslashes; instead convert args.quick_import to a pathlib.Path, inspect
Path.parts to find the "Lib" component (e.g., p = Path(args.quick_import); find
index = p.parts.index("Lib") if present), error if "Lib" not in p.parts, and
construct lib_path as Path(*p.parts[index:]) (so it works cross-platform);
update references to src_str, lib_marker, and lib_path accordingly and remove
the string-based "/Lib/" check.
b71ad81 to
f2d5594
Compare
also added --quick-import just like --quick-upgrade
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.