Add CI step for checking redundant test patches#7126
Add CI step for checking redundant test patches#7126youknowone merged 6 commits intoRustPython:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds a Python script to identify and flag redundant test patches in the codebase—test functions that solely invoke Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 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 |
|
@ShaharNaveh Can this be auto-fix using update_lib instead just lint? |
Yes, but I'd rather not doing that, for multiple reasons:
|
There was a problem hiding this comment.
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; userglob("*.py")instead.
Path.rglobalready applies the pattern recursively, so the leading**/is redundant. It still works, butrglob("*.py")is the idiomatic form.Proposed fix
- for file in TEST_DIR.rglob("**/*.py"): + for file in TEST_DIR.rglob("*.py"):
51-52: Prefersys.exit()over the builtinexit().
sysis already imported. The builtinexit()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())
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin lint-script-patches |
|
@ShaharNaveh could you check coderabbitai reviews? |
|
What i feel is this is a harmless glitch and automatically fixed when upgrading to next version. I'd rather not to bother contributors by that. Let's try how this work. We can disable it again if it reveals actually annoying. |
800bf35 to
0da173a
Compare
Summary by CodeRabbit
Chores