Add CI step for checking redundant test patches by ShaharNaveh · Pull Request #7126 · RustPython/RustPython · GitHub
Skip to content

Add CI step for checking redundant test patches - #7126

Merged
youknowone merged 6 commits into
RustPython:mainfrom
ShaharNaveh:lint-script-patches
Feb 15, 2026
Merged

Add CI step for checking redundant test patches#7126
youknowone merged 6 commits into
RustPython:mainfrom
ShaharNaveh:lint-script-patches

Conversation

@ShaharNaveh

@ShaharNaveh ShaharNaveh commented Feb 14, 2026

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

Chores

  • Improved continuous integration pipeline with enhanced Python code quality checks to automatically identify and report redundant test patches.

@coderabbitai

coderabbitai Bot commented Feb 14, 2026

Copy link
Copy Markdown
Contributor

@youknowone

Copy link
Copy Markdown
Member

@ShaharNaveh Can this be auto-fix using update_lib instead just lint?

@ShaharNaveh

Copy link
Copy Markdown
Contributor Author

@ShaharNaveh Can this be auto-fix using update_lib instead just lint?

Yes, but I'd rather not doing that, for multiple reasons:

  • The update_lib is not stable enough imo.
  • We would run into issues where someone fixes something else, but this will cause the entire test file to be updated and it will add more stuff to the PR that can be out of scope.

@ShaharNaveh
ShaharNaveh marked this pull request as ready for review February 14, 2026 14:26

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In @.github/workflows/ci.yaml:
- Around line 462-470: Remove the duplicate checkout step: the workflow contains
two identical steps using "uses: actions/checkout@v6.0.2"; keep the first
checkout that precedes the "Check for redundant test patches" step and delete
the second "uses: actions/checkout@v6.0.2" entry to avoid re-checking out the
repo unnecessarily.

In `@scripts/check_redundant_patches.py`:
- Around line 18-23: The except SyntaxError block after calling
ast.parse(contents) currently uses pass, which lets execution continue to
ast.walk(tree) and causes NameError or stale-tree reuse; change the handler so
the file is skipped (e.g., continue out of the loop) or explicitly set tree =
None and skip calling ast.walk for that file — update the block around
ast.parse(contents) / ast.walk(tree) to ensure that when ast.parse raises
SyntaxError you do not call ast.walk on an undefined or previous tree.
🧹 Nitpick comments (2)
scripts/check_redundant_patches.py (2)

12-12: rglob("**/*.py") double-recurses; use rglob("*.py") instead.

Path.rglob already applies the pattern recursively, so the leading **/ is redundant. It still works, but rglob("*.py") is the idiomatic form.

Proposed fix
-    for file in TEST_DIR.rglob("**/*.py"):
+    for file in TEST_DIR.rglob("*.py"):

51-52: Prefer sys.exit() over the builtin exit().

sys is already imported. The builtin exit() is intended for the interactive interpreter; sys.exit() is the standard way to exit from scripts.

Proposed fix
 if __name__ == "__main__":
-    exit(main())
+    sys.exit(main())

Comment thread .github/workflows/ci.yaml Outdated
Comment thread scripts/check_redundant_patches.py
@github-actions

Copy link
Copy Markdown
Contributor

Code has been automatically formatted

The code in this PR has been formatted using:

  • cargo fmt --all
    Please pull the latest changes before pushing again:
git pull origin lint-script-patches

@youknowone

Copy link
Copy Markdown
Member

@ShaharNaveh could you check coderabbitai reviews?

@youknowone

Copy link
Copy Markdown
Member

@youknowone
youknowone merged commit 64088bb into RustPython:main Feb 15, 2026
13 checks passed
youknowone pushed a commit to youknowone/RustPython that referenced this pull request Mar 22, 2026
@coderabbitai coderabbitai Bot mentioned this pull request Mar 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants