Repository navigation
[hip-tests] Standardize test level discovery - #8932
Conversation
❌ PR Check — Action Required
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
There was a problem hiding this comment.
Pull request overview
This PR standardizes how hip-tests resolves a single “test level” for an entire Catch2 run, making level selection deterministic (CLI tag filter → HIP_TEST_LEVEL → hardcoded default) and aligning fallback/default parameters with level_2 from definitions.yaml.
Changes:
- Updated
TestParameterStorefallback parameters to mirrorlevel_2(sizes, block sizes, iterations, warmups, max memory). - Removed prior “level_0-or-hardcoded” default initialization logic in
TestParameterStore::initialize(). - Reworked the Catch2 event listener to resolve the run’s level once at startup with strict precedence, validate it against a supported set, and apply it globally.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
projects/hip-tests/catch/include/hip_test_params.hh |
Updates hardcoded fallback parameters to match level_2 defaults. |
projects/hip-tests/catch/hipTestMain/hip_test_params.cc |
Removes prior fallback-default selection logic in initialization. |
projects/hip-tests/catch/hipTestMain/hip_test_listener.cc |
Implements strict, single-pass level resolution (CLI → env → default) and global application at run start. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
27907fa to
fe98b75
Compare
26e305e to
86d831e
Compare
|
This pull request has been inactive for 25 days and will be marked as stale. If you would like to keep this PR open, please:
This PR will be automatically closed in 5 days if no further activity occurs. |
86d831e to
d49c43d
Compare
mangupta
left a comment
There was a problem hiding this comment.
🤖 AI-assisted pre-review (for @mangupta)
[IMPORTANT] New level-resolution grammar and fatal misconfiguration path have no test coverage and are not testable as written
This change replaces a one-line regex extraction with a multi-rule parser (comma-split OR filters, AND'd patterns, ~ negation, exclusion-only filters expanding to the full supported set, bare-tag mode only for the environment variable, strict level_N matching, highest-wins reduction) plus a new fatal std::exit(EXIT_FAILURE) path, and no test exercises any of it. The helpers are private statics inside the .cc with no header, so they cannot be unit-tested without a refactor, and a silent mis-resolution changes the parameters of every parameterized test in the suite rather than failing loudly.
Suggested fix: Move collectLevels()/highestLevel()/isSupportedLevel() into a small internal header (or an anonymous-namespace-free helper compiled into the test main) and add a Catch2 test case covering the four scenarios above, asserting the resolved level string for each input rather than only the final run parameters. Alternatively, if the fatal path cannot be unit-tested in-process, cover it with a ctest WILL_FAIL/expected-exit-code test that runs the binary with HIP_TEST_LEVEL=level_9.
bd48524 to
c9d3a30
Compare
TheRock Submodule Bump ActivityNewest first
|
Motivation
Currently, it is not strictly clear with which level hip-tests will be executed with, due to a lot of ways they can be actually executed.
Technical Details
Standardize to three possible ways:
Remove all other invisible, non-deterministic, fallbacks.
Define level_2 as default level, using its parameters for test execution.
Define a strategy to determine level when multiple are passed - highest order wins (used for test parameters).
Issue Tracking
Test Plan
Ensure build passes. All tests pass
Test Result
Local build passed, local tests pass
Submission Checklist