rename fix_test and support removing unexpected success by youknowone · Pull Request #6748 · RustPython/RustPython

youknowone

📝 Walkthrough

Walkthrough

Added AST-based utilities to detect super-only methods and remove expected failure decorators. Enhanced test parsing to collect unexpected successes alongside failures. Modified the test runner to track failing tests separately and automatically patch expected failures for tests that now pass.

Changes

Cohort / File(s)	Summary
Test automation enhancement `scripts/auto_mark_test.py`	Added `is_super_call_only()` and `remove_expected_failures()` helper functions for AST-based decorator removal. Extended `TestResult` with `unexpected_successes` field and updated `__str__()`. Modified `parse_results()` to detect and collect UNEXPECTED SUCCESS entries. Refactored `run_test()` to distinguish failing vs. unexpected success tests, apply patches selectively, and remove expected failure decorators for passing tests.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested reviewers

reactive-firewall

Poem

🐰 Tests once failing now shine bright,
Expected failures fade from sight,
AST paths trim the decorators away,
Hopping forward to a cleaner day! ✨

📜 Recent review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 380fa39 and 6120f6a.

📒 Files selected for processing (1)

scripts/auto_mark_test.py

🧰 Additional context used

📓 Path-based instructions (1)

**/*.py

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.py: Follow PEP 8 style for custom Python code
Use ruff for linting Python code

Files:

scripts/auto_mark_test.py

🧠 Learnings (4)

📓 Common learnings

Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-14T14:52:10.778Z
Learning: Applies to Lib/test/**/*.py : Use `unittest.skip("TODO: RustPython <reason>")` or `unittest.expectedFailure` with `# TODO: RUSTPYTHON <reason>` comment when marking tests in Lib/ that cannot run

Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-14T14:52:10.778Z
Learning: Applies to **/test*.py : Only remove `unittest.expectedFailure` decorators and upper TODO comments from tests when tests actually pass, or add these decorators when tests cannot be fixed

Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-14T14:52:10.778Z
Learning: Applies to **/test*.py : When a test cannot pass due to missing language features, keep it as expectedFailure and document the reason instead of modifying the test

📚 Learning: 2026-01-14T14:52:10.778Z

Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-14T14:52:10.778Z
Learning: Applies to **/test*.py : Only remove `unittest.expectedFailure` decorators and upper TODO comments from tests when tests actually pass, or add these decorators when tests cannot be fixed

Applied to files:

scripts/auto_mark_test.py

📚 Learning: 2026-01-14T14:52:10.778Z

Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-14T14:52:10.778Z
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/auto_mark_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/auto_mark_test.py

🧬 Code graph analysis (1)

scripts/auto_mark_test.py (1)

scripts/lib_updater.py (3)

PatchSpec (81-122)

UtMethod (59-78)

apply_patches (273-283)

⏰ 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 (4)

scripts/auto_mark_test.py (4)
26-36: LGTM!

Imports are appropriate for the new AST-based functionality.

119-131: LGTM!

The parsing logic for unexpected successes correctly follows the existing pattern for failures and properly validates the path format.

142-160: LGTM!

The AST pattern matching logic is well-structured and correctly identifies methods that only delegate to super().

203-222: String check may match conditional variants like expectedFailureIf.

The check "expectedFailure" in line_content at line 208 would also match @unittest.expectedFailureIf(...) or similar conditional decorators. If a test has a conditional expected failure (platform-specific), this might incorrectly remove it when only the unconditional decorator should be targeted.

Consider using AST-based checking for the decorator name, or explicitly excluding conditional variants:
♻️ Optional: More precise check
-                    # Check if it's `@unittest.expectedFailure`
-                    if "expectedFailure" not in line_content:
+                    # Check if it's `@unittest.expectedFailure` (exact, not conditional variants)
+                    if "expectedFailure" not in line_content or "expectedFailureIf" in line_content:
                         continue

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review 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.