From 058abf410cbe78fa599c1b3316b62c700e497182 Mon Sep 17 00:00:00 2001 From: Sam Ruscica Date: Fri, 25 Sep 2026 16:35:59 +0000 Subject: [PATCH] AIRUNTIME-2743: Add unsupported skip field to Catch2 config parser Introduce an `unsupported:` skip field alongside the existing `disabled:` field in the Catch2 config YAML. Both share the same syntax (a flat target list, or a `{targets, reason}` mapping) and emit the same skip tags; they differ only in intent. `unsupported:` marks a genuine capability gap (a platform, architecture, or runtime mode that cannot run the case), while `disabled:` marks a temporary or tracked defect. Reason-field enforcement is intentionally deferred (AIRUNTIME-2744); the validator only guards that each skip field is a list or a mapping with a `targets` list, since a scalar breaks tag generation. Changes: - parse_config.py: emit skip tags for the `unsupported:` field - check_config.py: validate `disabled`/`unsupported` field shape - README.md, TESTING.md: document the unsupported field --- projects/hip-tests/TESTING.md | 2 +- projects/hip-tests/catch/README.md | 12 +++++-- .../hip-tests/catch/config/check_config.py | 27 ++++++++++++++++ .../hip-tests/catch/config/parse_config.py | 32 ++++++++++++++++--- 4 files changed, 64 insertions(+), 9 deletions(-) diff --git a/projects/hip-tests/TESTING.md b/projects/hip-tests/TESTING.md index a00f9c508a9e..8f2e9953a3bb 100644 --- a/projects/hip-tests/TESTING.md +++ b/projects/hip-tests/TESTING.md @@ -81,7 +81,7 @@ When you change public HIP API behavior, run the contract tests for the touched A flaky or quarantined case is treated as unstable signal: it does not block merge, but it must have an owner, a tracking issue, and a removal or fix plan before it can be promoted back to trusted signal. Use `HIP_SKIP_TEST` for a genuine capability gap such as an unsupported device or runtime path, too few GPUs, or no image support, so the case skips cleanly instead of failing. Never leave a test that crashes the process. Any test that intentionally triggers a HIP error must consume the sticky thread-local error with `(void)hipGetLastError()` before returning so it does not leak into later cases in the same process. -Disabling a test case should be rare and explicit. It is appropriate when a test cannot execute on a platform or architecture because a required capability, OS facility, graphics environment, or runtime mode is absent; when a known product or infrastructure defect has a tracking issue; or when keeping the test enabled would crash the process and hide later signal. Prefer runtime capability checks plus `HIP_SKIP_TEST` for environment-dependent behavior, and use YAML `disabled:` entries only when a case should be hidden for a specific platform, architecture, sanitizer mode, or tracked known issue. Do not disable a test only to make CI green. Every disabled or quarantined case should record why it is disabled, where it is tracked, and what condition allows it to be re-enabled. +Disabling a test case should be rare and explicit. It is appropriate when a test cannot execute on a platform or architecture because a required capability, OS facility, graphics environment, or runtime mode is absent; when a known product or infrastructure defect has a tracking issue; or when keeping the test enabled would crash the process and hide later signal. Prefer runtime capability checks plus `HIP_SKIP_TEST` for environment-dependent behavior, and use the YAML skip fields only when a case should be hidden for a specific platform, architecture, sanitizer mode, or tracked known issue. The `disabled:` and `unsupported:` fields share the same syntax and produce the same skip; they differ only in intent. Do not disable a test only to make CI green. Every skipped case should record why it is skipped, where it is tracked when a `disabled:` entry points at a bug. ## Coverage diff --git a/projects/hip-tests/catch/README.md b/projects/hip-tests/catch/README.md index 0b04351721d5..fb11e3da2499 100644 --- a/projects/hip-tests/catch/README.md +++ b/projects/hip-tests/catch/README.md @@ -69,7 +69,9 @@ All configuration for the test cases is done through the `hip_tests_config.yaml` Every test case has its own entry. Currently supported options are: - level : Specify to which level the case belongs to (e.g. Level_2 is a standard test) - tags : List all Catch2 tags that the case is associated with -- disabled : List all platforms where the case should be disabled +- disabled : Temporarily skip the case on the listed targets (a regression or tracked bug that is expected to be re-enabled) +- unsupported : Permanently skip the case on the listed targets, where the platform, architecture, or backend cannot support it +`disabled` and `unsupported` share the same syntax and produce the same skip; they differ only in intent (temporary regression versus permanent capability gap). The group name is automatically added as a tag for every case. Changing the configuration file will retrigger the build, so we have an up to date configuration every time. @@ -80,8 +82,12 @@ unit: Unit_atomicExch_system_Positive_Peer_GPUs: <<: *level_2 tags: [multigpu] - # SWDEV-435667: Below tests failing randomly in stress test on 01/12/23 - disabled: [amd_wsl] + disabled: + targets: [amd_wsl] + reason: "SWDEV-435667: fails randomly in stress test" + unsupported: + targets: [gfx900] + reason: requires a hardware feature not present on this architecture ``` will be generated (on an AMD linux machine) as: ```cpp diff --git a/projects/hip-tests/catch/config/check_config.py b/projects/hip-tests/catch/config/check_config.py index a3a5520baf34..838e859f650d 100644 --- a/projects/hip-tests/catch/config/check_config.py +++ b/projects/hip-tests/catch/config/check_config.py @@ -26,11 +26,29 @@ def main(): configs_path = args.configs_path missing = [] + invalid_skip_fields = [] for group, cases in iter_group_configs(configs_path): for case_name, case_config in cases.items(): if "level" not in case_config: missing.append(f" {group}/{case_name}") + for field in ("disabled", "unsupported"): + if field not in case_config: + continue + value = case_config[field] + # A skip field is either a flat list of targets, or a mapping + # with a 'targets' list plus an optional 'reason' scalar. The + # reason is metadata and is intentionally NOT enforced yet — + # deferred to a later phase (AIRUNTIME-2744): + # https://amd-hub.atlassian.net/browse/AIRUNTIME-2744 + # We only guard that the targets are a list, since a scalar + # breaks tag generation (a string is iterated char-by-char) and + # a malformed mapping fails the parser's list concatenation. + if isinstance(value, list): + continue + if isinstance(value, dict) and isinstance(value.get("targets"), list): + continue + invalid_skip_fields.append(f" {group}/{case_name}: '{field}'") if missing: print( @@ -41,6 +59,15 @@ def main(): print(f"[check_config] {ERROR}{entry}{RESET}", file=sys.stderr) sys.exit(1) + if invalid_skip_fields: + print( + f"[check_config] {ERROR}ERROR: The following test cases have a 'disabled'/'unsupported' field that is neither a YAML list nor a mapping with a 'targets' list:{RESET}", + file=sys.stderr, + ) + for entry in invalid_skip_fields: + print(f"[check_config] {ERROR}{entry}{RESET}", file=sys.stderr) + sys.exit(1) + if __name__ == "__main__": main() diff --git a/projects/hip-tests/catch/config/parse_config.py b/projects/hip-tests/catch/config/parse_config.py index 0651ba0940c7..7a2f1505934a 100644 --- a/projects/hip-tests/catch/config/parse_config.py +++ b/projects/hip-tests/catch/config/parse_config.py @@ -42,17 +42,39 @@ def parse_args(): "--asan", action="store_true", help="Target is an Address Sanitizer (ASAN) build. Test cases that " - "list 'asan' in their 'disabled' field are skipped.", + "list 'asan' in their 'disabled' or 'unsupported' field are skipped.", ) return parser.parse_args() +def _skip_targets(field): + """Return the platform/arch/config targets from a skip field. + + A skip field is either a flat list of targets ([amd_windows]) or a mapping + with a 'targets' list plus an optional 'reason' scalar + ({targets: [...], reason: ...}). Only the targets drive tag generation; the + reason is metadata for other tooling and is ignored here. + """ + if isinstance(field, dict): + return field.get("targets", []) + return field + + def create_test_definition( group, case_name, case_config, platform, os_name, arch, asan=False ): level = case_config.get("level", 2) tags = case_config.get("tags", []) disabled = case_config.get("disabled", []) + unsupported = case_config.get("unsupported", []) + # Extract the targets from each skip field (list form or {targets, reason} + # mapping form) and merge into one ordered local (disabled first) so the + # match and promotion logic below is identical for either field. Do not + # mutate the source lists. "disabled" is temporary/regressions, "unsupported" + # is permanent; both produce the same [disabled] skip tag and + # [exclude_] promotions that CI and the compute-utils/WSL runners + # depend on. + skip_targets = _skip_targets(disabled) + _skip_targets(unsupported) tags_str = "" @@ -62,9 +84,9 @@ def create_test_definition( tags_str += f"[{group}]" if ( - f"{platform}_{os_name}" in disabled - or arch in disabled - or (asan and "asan" in disabled) + f"{platform}_{os_name}" in skip_targets + or arch in skip_targets + or (asan and "asan" in skip_targets) ): # Disabled on this platform (e.g. amd_linux) or arch (e.g. gfx1260). # Use the [disabled] tag (no leading dot) so it is visible in --list-tests @@ -78,7 +100,7 @@ def create_test_definition( # specific labels a test is disabled for (incl. OS labels dropped above). # Prefix is "exclude_" (not "disabled_") so it does not substring-match a # ctest -LE disabled filter; consumers should match anchored ^exclude_$. - for entry in disabled: + for entry in skip_targets: tags_str += f"[exclude_{entry}]" return f'#define {case_name} "{case_name}", "{tags_str}"'