ci: document and test path-based unit test patterns in pr bot#6654
Conversation
The test_file_patterns config already supports full-path glob matching (patterns containing '/') via _is_test_file, but this was underdocumented and the unit tests did not cover it. - policy.yml: expand the test_file_patterns comment to clearly explain both matching modes (basename vs full-path) and give a concrete example of using a path pattern for a test directory whose files use no special naming convention (e.g. projects/hip-tests/). - test_policy_check_ut.py: add two new tests covering path-based patterns: one confirming a file under a configured test path satisfies the requirement, and one confirming source-only changes still fail. Also fix the make_policy() fixture which referenced Policy fields (max_files_changed, max_total_changes, max_single_file_changes) that no longer exist, causing 20 pre-existing test failures. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
| policy = make_policy( | ||
| unit_test_patterns=[ | ||
| "test_*", | ||
| "*_test.*", | ||
| "**/test/gtest/**", | ||
| "projects/hip-tests/**", | ||
| ] | ||
| ) |
There was a problem hiding this comment.
The policy.yml in this repository currently only includes
test_file_patterns:
- 'test_*'
- 'testing_*'
- '*_test.*'
- '*_tests.*'
- 'Test*'
# Path-based: any file under a 'test/gtest/' directory is a unit test
# (e.g. projects/miopen/test/gtest/unit_conv_solver_ConvWinoRageRxS.cpp).
- '**/test/gtest/**'so projects/hip-tests/catch/unit/memory/atomicAdd.cpp would not match without also adding this projects/hip-tests/** pattern. We can apply that pattern in rocm-systems, but what about always adding a simpler regex pattern like *test*? There would be false positives if we don't include a separator character like _, -, /, .
I ran a few audits and found only five source files that would match *test* as part of the file name without being an actual test file:
generateStats.cpp(genera teSt ats.cpp)generateStats.hpp(genera teSt ats.hpp)filewritestream.h(filewri test ream.h- Two copies of
ComputeStoreVgprs.py(Compu teSt oreVgprs.py)
The larger concern with using an overly broad filter pattern like *test* for the entire file path is that it would match any file under a test directory, including "non-source" files like like .md, .txt
Here's my full conversation with Codex running some analysis: https://gist.github.com/ScottTodd/49e4990c3bb05092f6d51690713acd77
Based on that analysis, I think standardizing test file names and encoding into enough policy patterns is probably better than a broad case-insensitive *test* regex. It shouldn't be too difficult to run a more focused audit on each subproject, find the style used, and encode that style into the policy while watching for false positives / false negatives.
There was a problem hiding this comment.
Good analysis — the \ false positive rate is low but the broader path concern is real. Agree that the right long-term approach is a per-subproject audit rather than a broad catch-all, with each team encoding their actual conventions into the policy (as we are doing for rocm-systems with the path pattern for hip-tests). Will track that as follow-up work. Thanks for running the audit.
Summary
The
test_file_patternsconfig 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 thetest_file_patternscomment to clearly document both matching modes (basename vs full-path) with concrete examples for how downstream repos configure path-based patterns for their test directoriestest_policy_check_ut.py: add two new tests covering path-based patterns (one pass, one fail), and fix themake_policy()fixture which referencedPolicyfields that no longer exist (max_files_changed,max_total_changes,max_single_file_changes), causing 20 pre-existing test failures onmainCloses #6654
Test plan
python -m pytest skills/therock_pr_bot/test_policy_check_ut.py— 32 pass (up from 3), 2 pre-existing failures unrelated to this changetest_path_based_pattern_satisfies_requirement: file underprojects/hip-tests/**satisfies the unit test requirement alongside a CLR source changetest_path_based_pattern_code_only_still_fails: source-only change with no matching test path still fails correctlyFollow-up
A separate PR to rocm-systems (ROCm/rocm-systems#8782) syncs
policy_check.pyand addsprojects/hip-tests/**to itstest_file_patterns.🤖 Generated with Claude Code