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 — preparing step to integrate fix_test into lib_updater by youknowone · Pull Request #6698 · RustPython/RustPython 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 |
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: 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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
b71ad81 to
f2d5594
Compare
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.
1 participant
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.