Tiny deprecate the --range-begin in run_suite.py#13381
Conversation
Summary of ChangesHello @hnyls2002, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on cleaning up the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request deprecates and removes the --range-begin and --range-end arguments from run_suite.py, making test partitioning mandatory. This is a good simplification.
My review identifies two critical issues that arise from this change: one new bug and one pre-existing bug that is now easily triggered. Both can cause the script to crash. I've provided a code suggestion to fix both issues by adding necessary validation and data type conversion, making the script more robust.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request removes the unused command-line arguments --range-begin and --range-end from run_suite.py. The change is a good cleanup that improves code maintainability. However, I've identified a subtle change in behavior that could lead to potential issues. By removing the else block, the code no longer creates a shallow copy of the files list, which could lead to unintended modifications of the global suites dictionary. I've added a comment with a suggestion to address this.
| if args.auto_partition_size: | ||
| files = auto_partition(files, args.auto_partition_id, args.auto_partition_size) |
There was a problem hiding this comment.
By removing the else block that contained the slicing files[args.range_begin : args.range_end], the code no longer creates a shallow copy of the files list when args.auto_partition_size is not set. This means the original list from the suites dictionary is passed to run_unittest_files. If run_unittest_files were to modify the list in-place, it would cause an unintended side effect of modifying the global suites dictionary.
While the original intent of the else block was to handle range_begin and range_end, the slicing also provided the benefit of creating a shallow copy. To maintain this safe behavior, I suggest explicitly creating a shallow copy in an else branch.
| if args.auto_partition_size: | |
| files = auto_partition(files, args.auto_partition_id, args.auto_partition_size) | |
| if args.auto_partition_size: | |
| files = auto_partition(files, args.auto_partition_id, args.auto_partition_size) | |
| else: | |
| # Explicitly create a shallow copy to avoid modifying the original list in `suites`. | |
| files = files[:] |
This argument is no longer used.