ci: sync pr bot from TheRock and add hip-tests path pattern#8782
Closed
amd-chrissosa wants to merge 6 commits into
Closed
ci: sync pr bot from TheRock and add hip-tests path pattern#8782amd-chrissosa wants to merge 6 commits into
amd-chrissosa wants to merge 6 commits into
Conversation
Three changes to fix the unit test check incorrectly flagging CLR PRs: 1. Sync policy_check.py from ROCm/TheRock (canonical source). Key addition: _is_test_file() supports two matching modes — basename patterns (no '/') match the filename only; path patterns (contain '/') match the full repo-relative path, enabling test directories to be recognised by location rather than filename. 2. Add projects/hip-tests/** to test_file_patterns in policy.yml. CLR's tests live in projects/hip-tests/ and are named after the API they test (atomicAdd.cc, acquire_release.cc) with no test_* prefix. Also aligns basename patterns with TheRock (testing_*, *_tests.*, Test*). 3. Add test_policy_check_ut.py synced from TheRock with rocm-systems additions: test_hip_tests_path_recognised_by_shipped_policy verifies the real policy.yml correctly passes a CLR + hip-tests PR. Closes #8778 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
4 tasks
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the Systems PR Bot policy checker to better recognize unit test changes (including path-based test locations like projects/hip-tests/**), synchronizes policy_check.py behavior with TheRock, and adds a local unit/integration test suite to prevent regressions in rocm-systems.
Changes:
- Extend unit-test file recognition to support both basename-based and path-based glob patterns, and ship
projects/hip-tests/**as a recognized test location. - Add/ship a comprehensive
test_policy_check_ut.pysuite, including an integration test that loads the realpolicy.yml. - Update bot comment/FAQ behavior and ignore Python bytecode caches in
tools/systems_pr_bot/.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tools/systems_pr_bot/test_policy_check_ut.py | Adds unit + integration tests for policy checking (including shipped-policy integration coverage). |
| tools/systems_pr_bot/policy.yml | Updates unit test file patterns to include hip-tests and documents basename vs path matching modes. |
| tools/systems_pr_bot/policy_check.py | Syncs policy check logic (notably test-file detection) and modifies comment output/help link behavior. |
| tools/systems_pr_bot/.gitignore | Ignores __pycache__/ for the bot’s Python code. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+9
to
+12
| Run locally: | ||
| python -m unittest .github/therock_pr_bot/test_policy_check_ut.py -v | ||
| # or | ||
| pytest .github/therock_pr_bot/test_policy_check_ut.py |
Comment on lines
119
to
120
| # A changed file is treated as a unit test if its basename matches one of | ||
| # these glob patterns (e.g. test_parser.py, parser_test.cpp). |
Comment on lines
1005
to
1009
| pr_number, | ||
| token, # type: ignore[arg-type] | ||
| marker, | ||
| build_policy_table_comment( | ||
| build_bump_pr_results(policy), marker, ready=True, note=note | ||
| ), | ||
| note, | ||
| ) |
Comment on lines
319
to
+323
| def ensure_pr_title(policy: Policy, title: str, errors: List[str]) -> None: | ||
| """Validate the PR title (length, Conventional Commits style, forbidden words). | ||
| """Validate the PR title (length and forbidden words). | ||
|
|
||
| Appends a structured Error/Expected/Desired-format message to `errors` for | ||
| each rule that fails. | ||
| Appends a structured Error/Expected message to `errors` for each rule that | ||
| fails. |
| footer = "\n\n> 🎉 All policy checks passed!" | ||
|
|
||
| faq_url = "https://github.com/ROCm/rocm-systems/tree/develop/docs/SYSTEMS_PR_BOT_FAQ.md" | ||
| faq_url = "https://github.com/ROCm/TheRock/blob/main/skills/therock_pr_bot/FAQ.md" |
This was referenced Jul 17, 2026
therock_configure_ci.py fell back to evaluating all build subtrees when modified files didn't match any known subtree. For PRs that only touch tools/ (bot scripts, utilities) this triggered a full Linux+Windows build matrix with no benefit. Add CI_SKIP_PATH_PREFIXES (initially ['tools/']) and check_only_ci_skip_paths() so that PRs whose every changed file is under a skip prefix produce an empty subtree list and skip the build CI entirely. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
github-actions
Bot
added
the
github actions
…ch not develop) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
amd-chrissosa
added a commit
to ROCm/TheRock
that referenced
this pull request
Jul 20, 2026
## Summary The `test_file_patterns` config already supports full-path glob matching (patterns containing `/` are matched against the full file path via `_is_test_file`), but this was underdocumented and had no unit test coverage. This matters because some repos (notably rocm-systems) have test directories whose files use no `test_*` naming convention (e.g. `projects/hip-tests/catch/unit/memory/atomicAdd.cc`). - **`policy.yml`**: expand the `test_file_patterns` comment to clearly document both matching modes (basename vs full-path) with concrete examples for how downstream repos configure path-based patterns for their test directories - **`test_policy_check_ut.py`**: add two new tests covering path-based patterns (one pass, one fail), and fix the `make_policy()` fixture which referenced `Policy` fields that no longer exist (`max_files_changed`, `max_total_changes`, `max_single_file_changes`), causing 20 pre-existing test failures on `main` Closes #6654 ## Test plan - [x] `python -m pytest skills/therock_pr_bot/test_policy_check_ut.py` — 32 pass (up from 3), 2 pre-existing failures unrelated to this change - [x] `test_path_based_pattern_satisfies_requirement`: file under `projects/hip-tests/**` satisfies the unit test requirement alongside a CLR source change - [x] `test_path_based_pattern_code_only_still_fails`: source-only change with no matching test path still fails correctly ## Follow-up A separate PR to rocm-systems (ROCm/rocm-systems#8782) syncs `policy_check.py` and adds `projects/hip-tests/**` to its `test_file_patterns`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
4 tasks
Contributor
Author
|
Superseded by #8899 — clean single-commit rebase on develop with all Copilot feedback addressed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three changes to fix the unit test check incorrectly flagging CLR PRs:
1. Sync
policy_check.pyfrom ROCm/TheRock (canonical source)The previous version lacked
_is_test_file(). The updated version supports two matching modes:/): match the filename only/): match the full repo-relative path, allowing test directories to be recognised by location regardless of filename2. Add
projects/hip-tests/**totest_file_patternsinpolicy.ymlCLR's tests live in
projects/hip-tests/and are named after the API they test (atomicAdd.cc,acquire_release.cc) with notest_*prefix. Also aligns basename patterns with TheRock (testing_*,*_tests.*,Test*).3. Add
test_policy_check_ut.pySynced from TheRock with a rocm-systems-specific integration test:
test_hip_tests_path_recognised_by_shipped_policyloads the realpolicy.ymland verifies a CLR PR touchingprojects/hip-tests/correctly passes the unit test check.Closes #8778
Tracks: ROCm/TheRock#6654
Depends on: #8781 (for CI to skip on future
tools/PRs)Test plan
python -m pytest tools/systems_pr_bot/test_policy_check_ut.pytest_hip_tests_path_recognised_by_shipped_policy: CLR source +projects/hip-tests/catch/unit/memory/atomicAdd.cc→ passestest_path_based_pattern_code_only_still_fails: CLR source only → still fails correctly🤖 Generated with Claude Code