Skip to content

Conversation

@youknowone
Copy link
Member

@youknowone youknowone commented Jan 16, 2026

tested by #6749

Summary by CodeRabbit

  • Chores
    • Enhanced test automation script with improved failure tracking and result handling.
    • Added support for identifying tests with unexpected successes and automating their remediation.
    • Improved test patching logic to apply targeted updates based on test outcomes.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 16, 2026

📝 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! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'rename fix_test and support removing unexpected success' accurately describes the main changes: renaming functionality and adding support for handling unexpected test successes.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

🧹 Recent nitpick comments
scripts/auto_mark_test.py (2)

67-74: Class-level mutable defaults are a Python anti-pattern.

tests = [] and unexpected_successes = [] are shared across all instances. While parse_results correctly reinitializes these, the pattern is fragile. Consider using a dataclass or __init__ method.

♻️ Suggested refactor using dataclass
+from dataclasses import dataclass, field
+
+@dataclass
 class TestResult:
-    tests_result: str = ""
-    tests = []
-    unexpected_successes = []  # Tests that passed but were marked as expectedFailure
-    stdout = ""
+    tests_result: str = ""
+    tests: list = field(default_factory=list)
+    unexpected_successes: list = field(default_factory=list)
+    stdout: str = ""
 
     def __str__(self):
         return f"TestResult(tests_result={self.tests_result},tests={len(self.tests)},unexpected_successes={len(self.unexpected_successes)})"

If using dataclass, you can also remove the explicit initializations at lines 80-81 since the factory creates fresh lists.


304-338: LGTM!

The flow correctly handles both marking new failures and removing expected failures from tests that now pass. The ordering (remove first, then apply new patches) is sensible.

Minor style nit: Line 306's condition could use in for consistency with line 100:

if test.result in ("fail", "error"):

📜 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@youknowone youknowone added the skip:ci Skip running the ci label Jan 16, 2026
@youknowone youknowone marked this pull request as ready for review January 16, 2026 14:57
@github-actions
Copy link
Contributor

Code has been automatically formatted

The code in this PR has been formatted using:

  • ruff format
    Please pull the latest changes before pushing again:
git pull origin auto-mark-test

@youknowone youknowone merged commit c934265 into RustPython:main Jan 16, 2026
9 checks passed
@youknowone youknowone deleted the auto-mark-test branch January 16, 2026 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip:ci Skip running the ci

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant