Add sanity checks when a test file is not added to CI #6947
Conversation
There was a problem hiding this comment.
Hello @fzyzcjy, 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!
Summary of Changes
Hello! Gemini here, providing a summary of this pull request. This PR introduces a valuable sanity check into the test suite runner (test/srt/run_suite.py). The primary goal is to ensure that any new test files added to the repository are explicitly included in one of the defined test suites that run in CI, or are intentionally marked as excluded. This prevents test files from being added and then forgotten, ensuring better test coverage awareness.
Highlights
- Test Suite Sanity Check: A new function
_sanity_check_suitesis added to compare the list of.pyfiles in the test directory with the files explicitly listed in thesuitesdictionary. If any file on disk is not listed in any suite, the check fails with an informative error message. - Explicit Exclusion List: A new suite entry
__not_in_ci__is added to thesuitesdictionary. This list contains test files that are intentionally not run as part of the standard CI suites. This allows the sanity check to pass while still documenting which files are excluded. - Integration into Runner: The
_sanity_check_suitesfunction is called early in themainfunction ofrun_suite.py, ensuring this check is performed every time the test suite runner is executed.
Changelog
- test/srt/run_suite.py
- Added import for
pathlib.Path(L4). - Added a new suite entry
__not_in_ci__containing a list of test files intentionally excluded from CI (L154-L214). - Implemented the
_sanity_check_suitesfunction to verify all test files are listed in suites or__not_in_ci__(L218-L231). - Called
_sanity_check_suitesin themainfunction (L318). - Minor whitespace adjustment in list slicing (L328).
- Added import for
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable sanity check to ensure that all Python files within the test directory are explicitly accounted for in the defined test suites or an exclusion list. This will help prevent accidental omission of new test files from CI. The implementation is clear and uses appropriate tools like pathlib and set operations.
One area for potential improvement is to add a docstring to the new _sanity_check_suites function to clarify its scope and requirements for future maintainers. Overall, this is a good enhancement for test suite management.
Summary of Findings
- Clarity of
_sanity_check_suitesscope: The_sanity_check_suitesfunction checks all*.pyfiles. It would be beneficial to add a docstring to clarify that this includes files like__init__.pyand utility scripts, which must then be listed insuites(often in__not_in_ci__). This was raised as a review comment withmediumseverity. - Naming of
__not_in_ci__key: The dictionary key__not_in_ci__uses a dunder naming convention, which is typically reserved for special Python attributes/methods. While functional, a more standard constant name (e.g.,EXCLUDED_FROM_CI_FILES) might improve clarity. This is a minor stylistic point and was not commented on directly due to review settings (low severity). - Assertion message wording: The assertion message in
_sanity_check_suitesrefers to "Some test files are not in test suite." Since the check includes all.pyfiles (not strictly test files), a more general wording like "Some Python files are not accounted for..." might be slightly more accurate. This is a minor wording suggestion and was not commented on directly due to review settings (low severity).
Merge Readiness
The core functionality added in this PR is beneficial. I've made one suggestion regarding adding a docstring for clarity, which I believe would improve maintainability. Addressing this point would make the PR ready for merging. As a language model, I am not authorized to approve pull requests; please ensure further review and approval from project maintainers.
| def _sanity_check_suites(suites): | ||
| dir_base = Path(__file__).parent | ||
| disk_files = set([str(x.relative_to(dir_base)) for x in dir_base.glob("**/*.py")]) | ||
|
|
||
| suite_files = set( | ||
| [test_file.name for _, suite in suites.items() for test_file in suite] | ||
| ) | ||
|
|
||
| missing_files = sorted(list(disk_files - suite_files)) | ||
| assert len(missing_files) == 0, ( | ||
| f"Some test files are not in test suite. " | ||
| f"If this is intentional, please add to `not_in_ci` section. " | ||
| f"{missing_files=}" | ||
| ) |
There was a problem hiding this comment.
This new sanity check is a great addition for ensuring all Python files are accounted for!
Could you clarify the intended scope of dir_base.glob('**/*.py')?
Specifically:
- Does this intentionally include
__init__.pyfiles and other non-test utility.pyfiles (likerun_suite.pyitself, which is correctly in__not_in_ci__)? The current implementation implies 'yes', meaning they must also be listed in one of thesuites(typically__not_in_ci__) to pass this check. - If this is the case, it might be helpful to add a docstring to
_sanity_check_suitesexplaining that all.pyfiles withintest/srt/and its subdirectories must be explicitly listed in thesuitesdictionary (either as runnable tests or in__not_in_ci__). This would help future maintainers understand why they might need to add, for example, a new__init__.pyor a utility script to the__not_in_ci__list.
This clarification would enhance the long-term maintainability of this check. What are your thoughts on adding such a docstring?
Motivation
Modifications
Checklist