feat: declare test_ids and labels in YAML files - #449
Merged
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
📝 WalkthroughWalkthroughThis PR moves validation labels and test IDs into YAML wiring, adds suite-wiring and test-plan guardrails, updates test-plan issue rendering to bare GitHub issue references, refreshes suite/provider configs and tests, and adjusts logging, network CRUD checks, GPU operator commands, and EKS node-pool deletion. ChangesWiring metadata and guardrails
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
|
abegnoche
changed the title
abegnoche
force-pushed
the
feat/move-labels-ids
branch
from
June 4, 2026 18:29
c9e6c15 to
caeadd6
Compare
abegnoche
changed the title
…tadata Establish test_id as the durable join key between docs/test-plan.yaml, the validation classes, and the release manifest, replacing brittle PR/commit archaeology. - Add `test_ids` ClassVar to BaseValidation (+ get_validation_test_ids helper) and surface it through the catalog. - Seed all 20 security.py validation classes with verified test_ids. - scripts/test_plan_coverage.py: offline join + report; `--check` fails when a class references a test_id absent from the plan (CI integrity guardrail). - scripts/sync_test_plan_status.py: refresh `#N (open/closed)` annotations from live GitHub (`--write`) or flag drift (`--check`). - make plan-coverage / make sync-plan targets; pytest guardrails under scripts/tests/. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Run the test-plan coverage integrity check (`make plan-coverage CHECK=1`) as a blocking gate so a validation class can never reference a test_id missing from docs/test-plan.yaml. Add the live GitHub issue-state drift check (`make sync-plan CHECK=1`) as an informational, non-blocking step since it depends on external issue state. Wire the new job into pipeline-status. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Stop storing GitHub issue state in docs/test-plan.yaml - it duplicated the source of truth (GitHub) and went stale. github_issues entries are now bare "#N" references; open/closed status is resolved live by the renderer. - Strip "(open)"/"(closed)" from 239 github_issues entries, keeping the issue number (the stable plan->issue link). - test_plan_yaml_to_adoc.py: best-effort fetch issue state via gh and render status icons; make plan still works offline (plain links, no icons). validate_test_plan now rejects stored state to keep the invariant. - Retire scripts/sync_test_plan_status.py, the make sync-plan target, and the CI drift step; the plan-coverage integrity gate stays. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Map high-confidence classes to their test-plan IDs (verified against plan summaries / introducing commits), extending the code<->plan link beyond the security domain: - K8s: CSI suite (K8S23-04..07), node-pool CRUD (K8S06-01..03), conformance, metrics, network ACL, OIDC, control-plane logs, network policy, autoscaler. - SDN: security-group CRUD + scoping (SDN02-*), fault/perf/audit logging (SDN09-*), network fabric (NET01-01, NET02-02). - Observability/telemetry (OBS-XX-17..19, TELEM-XX-04) and assorted compute, serial-console, access-key, and service-account checks. Generic/reusable checks and classes without an unambiguous mapping are left unset on purpose; the plan-coverage integrity gate guards every added value. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Map the provider-agnostic instance lifecycle checks (wired in both the bare_metal and vm suites) to their BM and VM test-plan IDs: - reboot CNP01-05/-13, stop CNP01-07/-14, start CNP01-08/-15, power-cycle CNP01-06, create CNP01-09, list CNP01-11, tags CNP05-01/-02, cloud-init BOOT02-01/-02, stable id CNP08-01/-03, specified key AUTH-XX-02. InstanceStateCheck is left unset: it is a generic post-op state assertion reused across many steps, not tied to a single plan item. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Verified via the introducing PR -> closed issue -> plan entry chain (with description disambiguation for multi-check PRs): - ByoipCheck NET03-01 (PR NVIDIA#144 / issue NVIDIA#115, Bring-Your-Own-IP) - DhcpIpManagementCheck CP-XX-01, VpcIpConfigCheck CP-XX-02 (PR NVIDIA#143 / NVIDIA#131,NVIDIA#132) - VpcCrudCheck SDN01-01..04, VpcIsolationCheck SDN04-02/-03, FloatingIpCheck SDN05-01, StablePrivateIpCheck SDN-XX-01, LocalizedDnsCheck SDN06-01, VpcPeeringCheck SDN07-01. SubnetConfigCheck, NetworkProvisionedCheck, NetworkConnectivityCheck, TrafficFlowCheck, and SecurityBlockingCheck are left unset: they originate from the foundational refactor commit with no issue link and no unambiguous plan entry. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
…tenant checks Mapped by reading each domain's plan section (no issue links exist for these foundational checks, which originate from bulk rename/refactor commits): - Slurm: SLURM-XX-02..09 (plan entries name the classes 1:1). - Tenant/IAM: TenantCreated/Listed/Info -> CP-XX-07/08/09; access-key authenticate/reject -> CP-XX-05/06. - Bare-metal NVIDIA hardware (Bm* cuda/driver/gpu) -> BMAAS-XX-08. - Host/SSH node checks -> BMAAS/VMAAS node entries (connectivity, OS, GPU, software/driver, vcpu/pci, ethernet/IB/NVLink, NCCL, GPU stress, training). - NIM health/model/inference -> BMAAS-XX-10, VMAAS-XX-12. CpuInfoCheck, ContainerRuntimeCheck, InstanceStateCheck and the foundational network helpers remain unset (no unambiguous plan entry). Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
- K8s workloads: NIM inference K8S-XX-03, NIM Helm K8S-XX-04, NCCL (single + multi-node) K8S-XX-05. - GPU Operator presence (namespace/pods + cluster-level installed check) -> K8S25-01. K8s GPU/node/pod-health checks (nvidia-smi, GPU pod access, capacity, labels, MIG, node-ready/count, expected-nodes, no-error/no-pending pods, pod health) are left unset: the plan's K8s section has no corresponding entry for them. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
The min_req label currently lives only in the test plan, not on the GitHub issues, so reporting coverage against it is premature. Replace the min_req-specific stats with generic coverage: plan items covered by >=1 class and by a released class. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
… IDs EthernetCheck, InfiniBandCheck, NvlinkCheck, NcclCheck, and GpuStressCheck are wired only in the bare_metal suite (no vm label), so their VMAAS-XX-* ids were wrong. Drop the VM ids, keeping the bare-metal ones. Surfaced by the new domain-consistency guardrail. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Extend `make plan-coverage CHECK=1` beyond referential integrity: - Completeness: every released class must declare test_ids unless explicitly allow-listed (ALLOWLIST_UNMAPPED) as generic/no-plan-entry. Catches forgotten classes. - Consistency: a class's labels must match the domain its test_ids imply (K8S->kubernetes, SEC->security, SDN/NET->network, BMAAS->bare_metal, VMAAS->vm, SLURM->slurm). Catches mis-assignments. Add `--review` to emit a class -> test_id -> plan-summary table for human correctness review, plus unit tests and a repo-wide guardrail test. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
…in test_ids Replace the central ALLOWLIST_UNMAPPED in the coverage tool with an explicit per-class marker so intent lives next to the code and an empty test_ids can never silently pass. - Add UNMAPPED sentinel to isvtest.core.validation; declare test_ids = (UNMAPPED,) on the 31 classes that intentionally map to no plan entry (generic helpers + K8s health/GPU + network/infra checks). - Completeness guardrail now fails for ANY class with an empty test_ids (real id or (UNMAPPED,) required), not just released ones - a new check must make an explicit choice. - Coverage/integrity/consistency/review ignore the sentinel via real_test_ids(). Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Prototype the YAML-only mapping model on the BM/VM host checks: each check declares a singular, context-correct test_id in bare_metal.yaml / vm.yaml instead of a class-level test_ids tuple. test_plan_coverage.py now reads test_id from the suite wiring and unions it with remaining class metadata so the integrity/completeness/consistency guardrails keep passing mid-migration. Per-context wiring surfaces a mapping the class model missed (GpuCheck covers BMAAS-XX-08 in BM, VMAAS-XX-06 in VM) and lets BM-only gaps be honest (N/A) where the BMaaS plan has no equivalent item. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Migrate every security.yaml check to a singular, context-correct test_id in the suite wiring and drop the corresponding class-level test_ids. This empties security.py of all test_id metadata; ServiceAccountCredentialCheck (defined in iam.py but wired only here) and the two vm.yaml-owned checks (ConsoleRbacCheck, VirtualDeviceHardeningCheck) move to their wiring suites too. Generic/UNMAPPED checks (e.g. StepSuccessCheck) are intentionally left as class-level metadata for now, pending a separate decision on how reusable checks should declare intentional gaps. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Migrate every slurm.yaml check (validations + workloads) to a singular test_id in the suite wiring and drop the class-level test_ids, emptying the slurm modules of test_id metadata. Slurm checks are wired only as variants (SlurmPartition-cpu/-gpu, SlurmGpuAllocation-1gpu/-2gpu, SlurmSbatchWorkload-gpu/-cpu/-inline). Teach apply_config_test_ids to propagate a variant's test_id up to its base class so the bare base entry in the catalog is not orphaned once its class id is removed; variant entries keep only their own id to preserve per-wiring precision. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
… YAML The IAM lifecycle classes (AccessKey*/Tenant*, CP-XX-05..09) are wired in control-plane.yaml, not iam.yaml (which only wires generic checks). Migrate their test_ids into the control-plane wiring and drop the class-level ids, emptying iam.py of all test_id metadata. Finding: the suite a class is wired in does not track the class's source file or domain label (iam.py classes live in the control-plane suite; iam.yaml has no real-mapped checks). Generic checks here (FieldExistsCheck, FieldValueCheck, CrudOperationsCheck, StepSuccessCheck) remain class-level UNMAPPED for now. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
…ring Migrate the four observability checks (OBS-XX-17/18/19, TELEM-XX-04) to singular test_ids in observability.yaml and drop the class-level ids, emptying observability.py. image-registry.yaml needs no migration: its only non-generic checks (ConnectivityCheck, OsCheck) are already mapped via the bm/vm suites, and InstanceStateCheck is generic/UNMAPPED. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
…te test_id
Reviewed example of the BM/VM duality resolution: the class previously carried
("CNP01-05", "CNP01-13") with no way to say which applied in which context.
bare_metal.yaml now declares CNP01-05 and vm.yaml declares CNP01-13, each
unambiguous per wiring; the class tuple is removed.
Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
…o YAML Migrate the rest of instance.py (stop/start/tag/serial-console/stable-id/ power-cycle/topology/list/created/specified-key) into bare_metal.yaml and vm.yaml as singular per-context test_ids, leaving only the generic InstanceStateCheck (UNMAPPED) class-level. This completes the bm/vm suites. Per-context wiring surfaced gaps the class tuples hid: - SerialConsoleCheck is a BM/VM duality (CNP06-01 vs CNP06-03); the class only declared the BM id, so the VM serial-console mapping was missing. - InstanceListCheck's CNP01-11 is a VMaaS item; bare metal has no list-nodes plan entry, so the BM wiring is an explicit N/A. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Adopt option 2: a check with no test_id is allowed. Remove the completeness guardrail from test_plan_coverage.py (--check now runs integrity + consistency only) and delete the now-redundant UNMAPPED sentinels from all validation and workload classes (26 across 10 files). The UNMAPPED constant moves out of the validation framework (core/validation.py) into the coverage tool, since the only remaining sentinel is the YAML test_id: "N/A" marker for intentional per-wiring gaps. BaseValidation keeps an empty test_ids default purely so the catalog can read the attribute. Tradeoff: this removes the "forgot to map a new check" tripwire. It can be restored later by exempting a known generic set and requiring a test_id on everything else wired in a suite. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Migrate the straightforward k8s checks (validations + workloads) to singular test_ids in k8s.yaml and drop their class-level ids: storage (K8S23-04/05/06/07), oidc (K8S18-01), metrics (K8S07-01), control-plane logs (K8S20-01), api network acl (K8S15-01), gpu-operator (K8S25-01), autoscaler (K8S-XX-08), conformance (K8S01-01), network-policy/dual-stack (K8S22-01), and the NCCL/NIM workloads (K8S-XX-03/04/05). UNMAPPED k8s checks are now simply blank. K8sNodePoolCheck is intentionally left for a separate decision (its class declares K8S06-01/02/03 but the wiring only exercises create and update). Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
…te gap
K8sNodePoolCheck declared ("K8S06-01","K8S06-02","K8S06-03") but is only wired
for create (CPU + GPU pools) and update. Map create wirings -> K8S06-01 and the
update wiring -> K8S06-02 in k8s.yaml, drop the class tuple, and flag K8S06-03
(delete a node pool) as an explicit gap: no wiring exercises a delete, so it is
now honestly uncovered rather than falsely claimed by the class tuple.
This completes the k8s suite migration; only network.yaml remains.
Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
…ring Migrate the singular-mapped network checks to network.yaml and drop their class-level ids: SG scoping (SDN02-05..10), SDN logging (SDN09-01/02/03), policy propagation (SDN02-08), IP/DNS/peering (SDN05-01, SDN06-01, SDN07-01, SDN-XX-01), fabric (NET01-01, NET02-02, NET03-01), and CP-XX-01/02, DMS05-01. The three CRUD-style checks (VpcCrudCheck SDN01-01..04, SgCrudCheck SDN02-01..04, VpcIsolationCheck SDN04-02/03) are left on their class tuples pending the per-operation-split decision. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
…g test_ids Finish removing test_id metadata from the validation framework: - nim.py: migrate the BMAAS-XX-10 / VMAAS-XX-12 duality into the bm/vm nim_* wirings; drop the class tuple. - bm_host_status.py: migrate BMAAS-XX-07 to the BmHostStatusLog wiring. - bm_gpu/bm_cuda/bm_driver: drop BMAAS-XX-08 (these on-host checks are wired in no suite; BMAAS-XX-08 stays covered by GpuCheck in bare_metal.yaml). - cluster.py: drop K8S25-01 from the superseded, catalog_exclude GpuOperatorCheck. - network.py: drop the three CRUD class tuples (SDN01/SDN02/SDN04); these are re-covered per-operation in network.yaml in a follow-up (option a). catalog.py no longer sources test_ids from the class (the catalog references the implementation, not the YAML declaration; the proper "catalog reads the declaration" refactor is deferred). Coverage now joins purely on config-declared test_ids. BaseValidation keeps only an empty test_ids default. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Wire a K8sNodePoolCheck for the delete leg of node-pool CRUD. Because validations run only after every step in a phase, a delete-to-zero check cannot share a phase with anything that expects the pool present. Add a throwaway CPU pool (own Terraform state) created in setup and deleted in the test phase, then assert it converges to zero nodes; a teardown safety-net destroy covers setup-only runs. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Test-plan IDs are sourced from the per-check YAML wiring; the catalog never reads the BaseValidation.test_ids ClassVar and get_validation_test_ids had no callers. Remove both now that no validation class declares test_ids. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Log the names of unreleased tests dropped from the catalog at DEBUG (the count stays at INFO). Also fix setup_logging so -v reaches isvtest.* logs: isvtest installs its own "isvtest" logger with propagate=False, so the root basicConfig never affected it; mirror the level onto that logger and its handlers. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
This update removes the reliance on class-level labels in validation classes, consolidating label management within the YAML configuration files. The `get_validation_labels` function has been eliminated, and all references to class-level labels have been replaced with a new approach that sources labels directly from the YAML wiring. This change enhances consistency and simplifies the codebase by ensuring that labels are uniformly managed across the system. Additionally, the documentation has been updated to reflect these changes, and various functions have been adjusted to accommodate the new label sourcing method. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Updated the test-plan coverage reporting to include gaps for uncovered test IDs, improving visibility into unimplemented checks. The documentation has been revised to reflect the new terminology, replacing references to class-level `test_ids` with wired `test_id`s. Additionally, the summary output now includes a detailed breakdown of uncovered plan items, enhancing the clarity of the coverage report. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
abegnoche
force-pushed
the
feat/move-labels-ids
branch
from
June 16, 2026 15:48
aba08a5 to
e4852eb
Compare
Remove the last Python label ClassVars and declare labels plus test-plan test_ids on the suite check wiring for capacity, bare metal, network, and multi-cluster validations. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Add validate_suite_wiring.py with make validate-suites and a pre-commit hook so every wired check in isvctl/configs/suites declares test_id and labels. Errors include the repo-relative path and line number; backfill missing metadata across suite YAML files. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Drop the standalone test-plan CI job; plan-coverage now runs as a pre-commit hook alongside validate-suites. Normalize local hook names to lowercase "check …" wording. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Use YAML-only wiring seeds for plan-coverage --check so pre-commit avoids discover_all_tests while reports keep the catalog-backed merge path. Add an equivalence test to keep both paths aligned. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
apply_config_labels and apply_config_test_ids were ~80% identical copy-paste. Extract the shared base-union + variant-propagation body into _apply_variant_union(entries, value_map, attr); both are now thin wrappers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
load_plan silently let the last entry win when a test_id was repeated in docs/test-plan.yaml, corrupting every derived report. Fail loudly instead, listing the offending ids. Also drop the stale "completeness" claim from --check help (the guardrails only enforce integrity and consistency). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
abegnoche
marked this pull request as ready for review
June 16, 2026 19:42
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-06-16 19:43:57 UTC | Commit: 74d7800 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
scripts/tests/test_test_plan_coverage.py (1)
137-155: ⚡ Quick winAdd a regression assertion for provider-wiring discovery.
test_guardrail_fast_path_matches_catalog_pathcan still pass when both paths omit the same wiring source. Add a focused test that verifies provider YAML entries are included inconfig_test_id_map/config_label_mapso this omission is caught early.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/tests/test_test_plan_coverage.py` around lines 137 - 155, Add a regression assertion after retrieving test_id_map and label_map to verify that provider YAML entries are properly included in these maps. The assertion should check that both config_test_id_map and config_label_map contain provider wiring entries, ensuring that the absence of provider-wiring discovery (if both the fast and slow paths omit the same source) will be caught early before the main fast/slow comparison assertion in the test_guardrail_fast_path_matches_catalog_path function.scripts/validate_suite_wiring.py (1)
104-108: ⚡ Quick winAdd a docstring for nested helper
_from_mapping.This nested function is missing a docstring. As per coding guidelines, “Every function and class must have docstrings following PEP 257.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/validate_suite_wiring.py` around lines 104 - 108, The nested helper function `_from_mapping` is missing a docstring, which violates the coding guideline requiring all functions to have docstrings following PEP 257. Add a docstring to the `_from_mapping` function that describes what it does: it takes a category string and a mapping (dict), iterates over the mapping items, and yields tuples containing the category, item name, and item params (or an empty dict if params is not already a dict).Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.pre-commit-config.yaml:
- Around line 49-56: The validate-suites hook's files pattern only matches suite
YAML files under isvctl/configs/suites/, but since provider wiring metadata is
now contract-critical, the hook should also trigger when provider configuration
files under isvctl/configs/providers/ are modified. Update the files pattern in
the validate-suites hook entry to include both suite and provider YAML paths so
that changes to either location trigger the validation guardrail.
In `@isvtest/src/isvtest/catalog.py`:
- Around line 106-122: The iter_config_checks function skips valid check
configurations including plain strings and dict-based checks without a "checks"
key. Modify the _from_mapping function to handle string inputs by treating them
as check names with empty parameters (yielding the string name with an empty
dict). Additionally, add a branch to handle when cat_config itself is a dict but
doesn't contain a "checks" key—treat this as a direct check definition by
calling _from_mapping on cat_config. These changes ensure that string list
entries like ["StepSuccessCheck"] and direct dict category forms are properly
emitted instead of being silently skipped.
In `@scripts/test_plan_coverage.py`:
- Around line 114-116: The deferred imports at lines 114-116 (the `from
isvtest.catalog import build_catalog` statement) and at lines 236-238 do not
comply with the repository import rule. For each location, either move the
import to the module top-level, or add a single-line comment at the import site
explaining why the import must be deferred to the function scope. Choose the
same approach (either hoist both to the top or add reason comments at both
deferred locations) to maintain consistency across the file.
- Around line 62-63: The SUITES_DIR definition currently only scans
isvctl/configs/suites/*.yaml files, but provider wiring metadata files also
contain test_id and labels that must be included in the coverage and integrity
checks. Extend the scanning logic in the functions that build config_test_id_map
and config_label_map to include provider wiring metadata files in addition to
the suites directory, ensuring that all test identifiers and labels from both
sources are captured in these mappings for complete coverage analysis.
In `@scripts/tests/test_test_plan_yaml_to_adoc.py`:
- Around line 34-40: The function `_plan` is missing a docstring, which violates
the repository's Python coding standards that require all functions and classes
to have docstrings following PEP 257. Add a docstring to the `_plan` function
that describes what it does, its parameters (github_issues as a list of
strings), and what it returns (a dictionary representing the test plan
structure). The docstring should be placed immediately after the function
definition line.
In `@scripts/validate_suite_wiring.py`:
- Around line 77-83: The _normalize_labels function in
scripts/validate_suite_wiring.py currently coerces scalar strings into lists
(lines 79-80), which allows invalid YAML like `labels: kubernetes` to pass
validation even though the wiring contract requires a list. Remove the
string-to-list coercion block (`if isinstance(value, str): value = [value]`) so
that scalar values are rejected and only properly formatted list values in YAML
pass validation, strictly enforcing the declared wiring contract.
- Around line 95-99: The bare except clause that catches all exceptions when
reading and parsing the config file with yaml.safe_load is silently hiding real
errors and should be replaced with explicit exception handling that surfaces the
failure. Modify the exception handler around the yaml.safe_load call to either
re-raise the exception so it propagates to the caller, or explicitly log the
error details and fail the validation. This will ensure that malformed suite
files are properly reported instead of being silently skipped.
---
Nitpick comments:
In `@scripts/tests/test_test_plan_coverage.py`:
- Around line 137-155: Add a regression assertion after retrieving test_id_map
and label_map to verify that provider YAML entries are properly included in
these maps. The assertion should check that both config_test_id_map and
config_label_map contain provider wiring entries, ensuring that the absence of
provider-wiring discovery (if both the fast and slow paths omit the same source)
will be caught early before the main fast/slow comparison assertion in the
test_guardrail_fast_path_matches_catalog_path function.
In `@scripts/validate_suite_wiring.py`:
- Around line 104-108: The nested helper function `_from_mapping` is missing a
docstring, which violates the coding guideline requiring all functions to have
docstrings following PEP 257. Add a docstring to the `_from_mapping` function
that describes what it does: it takes a category string and a mapping (dict),
iterates over the mapping items, and yields tuples containing the category, item
name, and item params (or an empty dict if params is not already a dict).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 81b3c668-107e-4a8d-ad9e-0b6152a6929c
📒 Files selected for processing (84)
.pre-commit-config.yamlAGENTS.mdMakefiledocs/test-plan.adocdocs/test-plan.yamlisvctl/configs/providers/aws/config/eks.yamlisvctl/configs/providers/k3s.yamlisvctl/configs/providers/microk8s.yamlisvctl/configs/providers/minikube.yamlisvctl/configs/suites/bare_metal.yamlisvctl/configs/suites/control-plane.yamlisvctl/configs/suites/iam.yamlisvctl/configs/suites/image-registry.yamlisvctl/configs/suites/k8s.yamlisvctl/configs/suites/network.yamlisvctl/configs/suites/observability.yamlisvctl/configs/suites/security.yamlisvctl/configs/suites/slurm.yamlisvctl/configs/suites/vm.yamlisvctl/src/isvctl/cli/__init__.pyisvctl/src/isvctl/cli/docs.pyisvctl/tests/test_capacity_config.pyisvctl/tests/test_orchestrator_loop.pyisvtest/src/isvtest/catalog.pyisvtest/src/isvtest/core/resolution.pyisvtest/src/isvtest/core/validation.pyisvtest/src/isvtest/core/workload.pyisvtest/src/isvtest/tests/test_validations.pyisvtest/src/isvtest/validations/attestation.pyisvtest/src/isvtest/validations/bm_cuda.pyisvtest/src/isvtest/validations/bm_driver.pyisvtest/src/isvtest/validations/bm_gpu.pyisvtest/src/isvtest/validations/bm_host_status.pyisvtest/src/isvtest/validations/capacity.pyisvtest/src/isvtest/validations/cluster.pyisvtest/src/isvtest/validations/governance.pyisvtest/src/isvtest/validations/hardware.pyisvtest/src/isvtest/validations/health.pyisvtest/src/isvtest/validations/host.pyisvtest/src/isvtest/validations/iam.pyisvtest/src/isvtest/validations/infiniband.pyisvtest/src/isvtest/validations/instance.pyisvtest/src/isvtest/validations/k8s_api_network_acl.pyisvtest/src/isvtest/validations/k8s_autoscaler.pyisvtest/src/isvtest/validations/k8s_cluster.pyisvtest/src/isvtest/validations/k8s_conformance.pyisvtest/src/isvtest/validations/k8s_control_plane_logs.pyisvtest/src/isvtest/validations/k8s_gpu.pyisvtest/src/isvtest/validations/k8s_gpu_operator.pyisvtest/src/isvtest/validations/k8s_metrics.pyisvtest/src/isvtest/validations/k8s_mig.pyisvtest/src/isvtest/validations/k8s_multi_cluster.pyisvtest/src/isvtest/validations/k8s_network_policy.pyisvtest/src/isvtest/validations/k8s_node_pool.pyisvtest/src/isvtest/validations/k8s_nodes.pyisvtest/src/isvtest/validations/k8s_oidc.pyisvtest/src/isvtest/validations/k8s_scheduling.pyisvtest/src/isvtest/validations/k8s_storage.pyisvtest/src/isvtest/validations/network.pyisvtest/src/isvtest/validations/nim.pyisvtest/src/isvtest/validations/observability.pyisvtest/src/isvtest/validations/sanitization.pyisvtest/src/isvtest/validations/security.pyisvtest/src/isvtest/validations/slurm_gpu_allocation.pyisvtest/src/isvtest/validations/slurm_job_submission.pyisvtest/src/isvtest/validations/slurm_node_job_execution.pyisvtest/src/isvtest/validations/slurm_partition.pyisvtest/src/isvtest/workloads/k8s_nccl.pyisvtest/src/isvtest/workloads/k8s_nccl_multinode.pyisvtest/src/isvtest/workloads/k8s_nim.pyisvtest/src/isvtest/workloads/k8s_nim_helm.pyisvtest/src/isvtest/workloads/k8s_stress.pyisvtest/src/isvtest/workloads/slurm_gpu_stress.pyisvtest/src/isvtest/workloads/slurm_nccl_multinode.pyisvtest/src/isvtest/workloads/slurm_sbatch.pyisvtest/tests/test_catalog.pyisvtest/tests/test_resolution.pyisvtest/tests/test_validation.pyscripts/test_plan_coverage.pyscripts/test_plan_yaml_to_adoc.pyscripts/tests/test_test_plan_coverage.pyscripts/tests/test_test_plan_yaml_to_adoc.pyscripts/tests/test_validate_suite_wiring.pyscripts/validate_suite_wiring.py
💤 Files with no reviewable changes (47)
- isvtest/src/isvtest/workloads/slurm_gpu_stress.py
- isvtest/src/isvtest/validations/slurm_gpu_allocation.py
- isvtest/src/isvtest/validations/k8s_multi_cluster.py
- isvtest/src/isvtest/validations/health.py
- isvtest/src/isvtest/validations/attestation.py
- isvtest/src/isvtest/workloads/k8s_nim.py
- isvtest/src/isvtest/validations/k8s_oidc.py
- isvtest/src/isvtest/validations/k8s_mig.py
- isvtest/src/isvtest/workloads/slurm_nccl_multinode.py
- isvtest/src/isvtest/validations/k8s_metrics.py
- isvtest/src/isvtest/core/workload.py
- isvtest/src/isvtest/validations/bm_host_status.py
- isvtest/src/isvtest/validations/k8s_autoscaler.py
- isvtest/src/isvtest/validations/bm_driver.py
- isvtest/src/isvtest/validations/k8s_network_policy.py
- isvtest/src/isvtest/workloads/slurm_sbatch.py
- isvtest/src/isvtest/validations/k8s_cluster.py
- isvtest/src/isvtest/workloads/k8s_nccl_multinode.py
- isvtest/src/isvtest/validations/slurm_job_submission.py
- isvtest/src/isvtest/validations/governance.py
- isvtest/src/isvtest/validations/k8s_api_network_acl.py
- isvtest/src/isvtest/validations/k8s_conformance.py
- isvtest/src/isvtest/validations/sanitization.py
- isvtest/src/isvtest/validations/hardware.py
- isvtest/src/isvtest/validations/k8s_control_plane_logs.py
- isvtest/src/isvtest/validations/k8s_nodes.py
- isvtest/src/isvtest/workloads/k8s_stress.py
- isvtest/src/isvtest/validations/observability.py
- isvtest/src/isvtest/validations/slurm_partition.py
- isvtest/src/isvtest/validations/bm_cuda.py
- isvtest/src/isvtest/validations/infiniband.py
- isvtest/src/isvtest/core/validation.py
- isvtest/src/isvtest/validations/iam.py
- isvtest/src/isvtest/workloads/k8s_nccl.py
- isvtest/src/isvtest/validations/k8s_scheduling.py
- isvtest/src/isvtest/validations/nim.py
- isvtest/src/isvtest/validations/bm_gpu.py
- isvtest/src/isvtest/validations/k8s_storage.py
- isvtest/src/isvtest/validations/k8s_node_pool.py
- isvtest/src/isvtest/validations/instance.py
- isvtest/src/isvtest/validations/cluster.py
- isvtest/src/isvtest/validations/k8s_gpu.py
- isvtest/src/isvtest/validations/slurm_node_job_execution.py
- isvtest/src/isvtest/validations/host.py
- isvtest/src/isvtest/validations/k8s_gpu_operator.py
- isvtest/src/isvtest/validations/security.py
- isvtest/src/isvtest/validations/capacity.py
GitHub's AsciiDoc preview does not render icon: macros. Emit plain issue links only and remove the live gh graphql fetch from make plan. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Surface suite YAML parse failures, require labels as lists, document deferred imports in plan-coverage, and add regression tests. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/test_plan_yaml_to_adoc.py (1)
87-100:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse full-match parsing for issue links to preserve the bare
#Ninvariant.Line 93 (and Line 200) use
re.match, so values like"#40(closed)"are still linkified if rendering is called
without prior validation. This diverges from the strict format check invalidate_test_plan().Proposed fix
- m = re.match(r"#(\d+)", str(entry)) + m = re.fullmatch(r"#(\d+)", str(entry)) if not m: parts.append(esc_adoc(entry)) continue @@ - m = re.match(r"#(\d+)", gh_entries[0]) + m = re.fullmatch(r"#(\d+)", str(gh_entries[0])) if m: num = m.group(1) first_gh_link = f"https://github.com/{GH_REPO}/issues/{num}[#{num}]"Also applies to: 200-203
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/test_plan_yaml_to_adoc.py` around lines 87 - 100, Replace re.match with re.fullmatch in the fmt_gh_issues_adoc function (line 93) to ensure only exact bare issue number formats like "`#N`" are linkified, not partial matches like "`#40` (closed)". The current re.match only checks the beginning of the string, allowing unwanted suffixes to pass through. Make the same change at the other affected location around line 200-203 where similar GitHub issue parsing occurs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@scripts/test_plan_yaml_to_adoc.py`:
- Around line 87-100: Replace re.match with re.fullmatch in the
fmt_gh_issues_adoc function (line 93) to ensure only exact bare issue number
formats like "`#N`" are linkified, not partial matches like "`#40` (closed)". The
current re.match only checks the beginning of the string, allowing unwanted
suffixes to pass through. Make the same change at the other affected location
around line 200-203 where similar GitHub issue parsing occurs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 33e6bfcd-6c5b-4e38-9c65-18212351a95c
📒 Files selected for processing (6)
docs/test-plan.adocscripts/test_plan_coverage.pyscripts/test_plan_yaml_to_adoc.pyscripts/tests/test_test_plan_yaml_to_adoc.pyscripts/tests/test_validate_suite_wiring.pyscripts/validate_suite_wiring.py
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/validate_suite_wiring.py
- scripts/test_plan_coverage.py
mresvanis
approved these changes
Jun 17, 2026
Tighten GitHub issue rendering to the bare-reference contract and include direct dict validation wiring in catalog config scans. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
isvtest/tests/test_catalog.py (1)
77-79: ⚡ Quick winAdd
tmp_pathtype annotation and move the import to module scope.This new test function misses a parameter annotation and introduces a function-local import without a
justification comment.Proposed cleanup
+from pathlib import Path from unittest.mock import patch from isvtest.catalog import build_catalog, get_catalog_version +from isvtest.catalog import _extract_checks_from_config from isvtest.core.validation import BaseValidation @@ - def test_extract_checks_supports_direct_dict_category_form(self, tmp_path) -> None: + def test_extract_checks_supports_direct_dict_category_form(self, tmp_path: Path) -> None: """Direct dict category wiring is included in catalog config scans.""" - from isvtest.catalog import _extract_checks_from_config - config = tmp_path / "direct-dict.yaml"As per coding guidelines: “All Python files must use type annotations …”, and “Place all imports at the top
of the file; defer imports inside functions only with a one-line comment giving the reason.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@isvtest/tests/test_catalog.py` around lines 77 - 79, The test function test_extract_checks_supports_direct_dict_category_form is missing a type annotation for the tmp_path parameter and contains a function-local import of _extract_checks_from_config without justification. Add the appropriate type annotation for the tmp_path parameter (typically pathlib.Path or similar based on pytest conventions) and move the import statement to the module scope at the top of the file with other imports. If there is a specific reason to keep the import local, add a one-line explanatory comment above it.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@isvtest/tests/test_catalog.py`:
- Around line 77-79: The test function
test_extract_checks_supports_direct_dict_category_form is missing a type
annotation for the tmp_path parameter and contains a function-local import of
_extract_checks_from_config without justification. Add the appropriate type
annotation for the tmp_path parameter (typically pathlib.Path or similar based
on pytest conventions) and move the import statement to the module scope at the
top of the file with other imports. If there is a specific reason to keep the
import local, add a one-line explanatory comment above it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a6ce55ae-1d22-4099-915d-12f239b15905
📒 Files selected for processing (4)
isvtest/src/isvtest/catalog.pyisvtest/tests/test_catalog.pyscripts/test_plan_yaml_to_adoc.pyscripts/tests/test_test_plan_yaml_to_adoc.py
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/test_plan_yaml_to_adoc.py
- isvtest/src/isvtest/catalog.py
Drop the AWS EKS delete-node-pool wiring from this metadata branch; the change is preserved separately on backup/eks-yaml-node-pool-delete. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
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
2 participants
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
Makes the test plan the source of truth for "which validation class implements which plan item" and moves both
test_idsand trait/platformlabelsoff PythonClassVars and onto the per-wiring YAML (suite + provider configs).scripts/validate_suite_wiring.py --checkrequires every check wired inisvctl/configs/suites/*.yamlto declaretest_id(plan id or"N/A") and non-emptylabels.scripts/test_plan_coverage.py --checkensures declaredtest_ids exist indocs/test-plan.yamland that labels match the domain the id implies. Rejects duplicate plantest_ids at load time. Uses a YAML-only fast path for--check(no full catalog import).labels: [...]are unioned into pytest marks, the catalog, and orchestrator include/exclude filtering.git grep "labels: ClassVar"is now zero.test_ids live on the wiring; catalogtest_idsstripped. Intentional gaps usetest_id: "N/A"."N/A"). Notable additions includeStepSuccessCheck-delete_tenant→CP-XX-10and node-pool delete →K8S06-03. Plan coverage is now 147 / 324 implemented items (~45%).Catalog labels were verified against the pre-migration baseline throughout the migration.
Test plan
make validate-suites CHECK=1green (all suite checks declaretest_id+labels)make plan-coverage CHECK=1green (146 mapped classes pass integrity + consistency)make testmake demo-testuvx pre-commit run -a(includes both hooks above)isvtest test --platform X/--label Ywithout-f) discovers all classes but has no YAML to read labels from, so platform/trait filtering no-ops there; to be fixed by deriving marks from the catalog.Summary by CodeRabbit
Release Notes
New Features
Improvements
test_id/label metadata.Documentation
#NNN) and removed status icon markup.Chores