feat(catalog): give every test a unique, meaningful name - #563
Merged
abegnoche merged 29 commits intoJul 30, 2026
Conversation
A wiring name is a test's identity in the catalog, the report, and the service, so duplicates are not cosmetic: _build_suite_map last-wins, which silently drops entries (bare_metal's GpuCheck lost to vm's). Wiring generic checks under their class names both creates those duplicates and spends two catalog names on one idea, leaving the second holding test_id: "N/A". Add a `compose:` form so a suite names the property under test and lists the generic checks that establish it. The composite is one catalog entry with one test_id; members run against the group's step output as subtests, so failures still name the part that broke and every member runs even after one fails. Migrate iam.yaml as the first suite: five wiring entries become three named ones, clearing all 4 of its uniqueness violations (99 -> 95 repo-wide). This also maps IAM02-01 (Delete user), which no check claimed before, so the plan entry gains the `iam` label the label-sync guardrail requires on both sides. Composites ship unreleased until a release commit adds them to released_tests.json; run with ISVTEST_INCLUDE_UNRELEASED=1 to exercise them. Global uniqueness enforcement stays behind ISVCTL_ENFORCE_UNIQUE_WIRING until the remaining suites are migrated. docs/test-plan.adoc is regenerated by `make plan`; it was already stale, so the diff also picks up a `storage` label on K8S23-04/05 that was in the source YAML but missing from the rendered doc. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
The generic checks name a mechanism, not a property under test, so wiring one directly spends a catalog identity on "StepSuccessCheck" and forces several unrelated tests to share it. Mark them compose_only and have the wiring validator reject them outside a composite's compose list, so a suite has to say what the test proves before it can assert it. Resolving the wiring name first means variant spellings such as StepSuccessCheck-teardown are rejected too; they leave the same mechanism as the test's public identity. The ban is scoped to the canonical suites because the catalog only reads suites/, so provider wiring never becomes a catalog entry. Enforcement joins the existing uniqueness deferral under one flag, since migrating a suite satisfies both rules at once. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Every validation group here spent two or three catalog names on one idea, leaving the generic members holding test_id "N/A" and making the plan id land on whichever member happened to carry it. Compose each group into the test it proves, so BOOT01-01..05 and BOOT03-02 attach to a name a reader recognises and each group is one catalog entry. The instance-state members fold in too: "boots from image" is only true if the instance reaches running, and that also stops image-registry from claiming the InstanceStateCheck name that vm and bare_metal already wire. vm_ssh keeps ConnectivityCheck and OsCheck, which already say what they prove. Provider docs that listed the old per-step wiring are updated. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Migrates the remaining six suites off directly wired generic checks, so no canonical suite spends a catalog identity on StepSuccessCheck and friends any more. Teardown groups become the thing they verify was released, the storage and object-storage groups become the volume or object behaviour they exercise, and CP10-01 and SEC21-01 now attach to a name a reader recognises instead of a bare step-success assertion. This also retires StepSuccessCheck-delete_access_key and StepSuccessCheck-delete_tenant, the variant spellings that put the mechanism in the test's public identity. bare_metal's topology_placement and host_status_logs drop their extra StepSuccessCheck rather than compose it: TopologyPlacementCheck and BmHostStatusLog both fail when the step produces no usable output, so the generic check asserted nothing they did not, and keeping their names holds CNP01-04 and BMAAS07-01 steady. Two catalog tests used StepSuccessCheck as a stand-in for a catalog entry; they now use a name that still has one, and the unreleased-filter test asserts on a composite, which is what actually ships unreleased. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
The four node-pool assertions all shared the K8sNodePoolCheck wiring name, so the catalog kept only the last one and K8S06-01, K8S06-02 and K8S06-03 had no distinct entry to attach to. Each operation now names what it proves and composes the class, which also removes the reason list form was mandatory (ordering keeps it). Adds a test for a composite forwarding its params to a member listed with none of its own: wrapping a single purpose-built check depends on it, and until now every composite passed params per member. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
storage wired InstanceStateCheck for the instance that carries the test volume, colliding with bare_metal's setup check of the same name and letting the catalog keep only one of them. Name it for the role the instance plays here so both survive. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
vm_ssh wired ConnectivityCheck and OsCheck under their class names, which bare_metal and vm also claim, so the catalog kept one entry for all three suites. Both checks tell one story here - the image boots into an OS you can log into and it is the OS asked for - so they become one named test. This is the group deferred when image-registry was first migrated, and it sets the shape for the ssh groups still to come in vm and bare_metal. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
The suite asserted the same five properties at five points in the host lifecycle by wiring the same check names repeatedly. The catalog keys on the wiring name, so those repeats collapsed into one entry each and the frontend showed a single row where the suite proves five distinct things. Give every wiring a name that says which stage it proves, composing the generic check underneath. Reachability and OS are merged per stage since they share labels and only one carries a plan ID; the three host_os checks stay separate because only the PCI one is gpu-labelled and merging would pull the other two into gpu label filtering. AWS overrides start_gpu, power_cycle_gpu, and serial_console by check name, and NICo excludes SSH checks by name - both are retargeted at the new names, otherwise the override merges as a second check instead of configuring the intended one. Test IDs are unchanged, so plan coverage holds. The new names ship unreleased until a release commit adds them to released_tests.json. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Last suite wiring the same check names at several points in the lifecycle, so the catalog collapsed those repeats into one entry each and hid what the suite proves at start and after reboot. Name every wiring for the stage it proves, composing the generic check underneath. Unlike bare_metal, reachability and the OS image are distinct plan items here (VMAAS-XX-01 and VMAAS-XX-05), so they stay two tests per stage rather than merging into one ready check. Test IDs are unchanged. The new names ship unreleased until a release commit adds them to released_tests.json. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Both rules were written opt-in behind ISVCTL_ENFORCE_WIRING_RULES while the suites were migrated one at a time. Every suite now names what it proves, so the toggle has no remaining use and only offers a way to reintroduce the duplicates it was meant to catch. Enforce unconditionally: the validator reports reused wiring names and directly wired generic checks, and the catalog raises on a duplicate rather than dropping a test through last-wins. Uniqueness had no test of its own while it was opt-in, so add one. Drop the test asserting unmigrated suites still pass, since nothing may be unmigrated now. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Platform rows were rendering as bare_metal / bare_metal because the CLI treated entry capability as a requirement. Align the column with the post-refactor model: platform suites show the capability alone; plain suites show requires (or core). Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
The suite's new composite names spelled out BareMetal while the checks already in the tree use Bm (BmHostStatusLog, BmGpuHealth) and the vm suite uses Vm, so the catalog listed three spellings of the same idea. Settle on Bm. BareMetalOutput keeps its name: it is the provider step output model, not a check, and renaming it would change the JSON contract stubs implement. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
The nico control-plane, iam, and network configs declared their own named checks, so the catalog - which reads only suites/ - never saw them, and a release would have left them permanently unreleased. Two of them restated assertions a suite already made, and the two network probes proved the same property twice behind step names inherited from Carbide that promise traffic tests neither one runs. Name each property in the suite that owns it and let nico import that suite. Checks whose steps nico does not wire skip as step_not_configured, the way nico/config/bare_metal.yaml already works. - control-plane: nico's api_health composite was byte-identical to ControlPlaneApiHealthCheck, so it is gone. That check now declares CP03-01, which an authenticated control-plane request is what proves; DATASVC-XX-01 remains covered end to end by the object-lifecycle check. - iam: add IamCallerIdentityCheck, a whoami on the credentials the run is configured with, for platforms whose IAM is read-only. - network: add a network_inventory group (VpcListedCheck, VpcInfoCheck, SubnetAssignedCheck) for platforms that hand out pre-provisioned VPCs instead of letting a tenant create them. VpcInfoCheck reclaims SDN01-02, which NVIDIA#452 declared and NVIDIA#526 had to drop to the N/A sentinel because an SDN id cannot carry the network label from a provider config. Collapse the duplicate subnet probes into check_subnet_assignment.py and take off the tenant costume: list_vpcs/get_vpc emit vpc_id and vpc_name rather than faking tenant fields so the IAM tenant checks would accept them. list_vpcs now always emits found_target, which closes a hole where a requested VPC absent from the listing still passed. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
The catalog became suite-derived in NVIDIA#561, so the six on-host NVIDIA-stack checks wired only in k3s/microk8s/minikube stopped being catalog entries. They still sit in released_tests.json because that file is a pre-NVIDIA#561 snapshot, so the next version bump regenerates it without them and they begin skipping as "unreleased". These are local-dev tools no ISV runs, so staying out of the catalog is correct; record that intent and point developers at the existing ISVTEST_INCLUDE_UNRELEASED escape hatch instead of inventing a mechanism. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
DpuNetworkCheck arrived in NVIDIA#450 alongside DpuHealthCheck but was never wired, so it has never run and the next manifest regen would drop it from released_tests.json. Publish the contract it expects next to its sibling instead of losing the class. No provider emits interfaces/bgp_enabled/dpu_extension_deployments yet, so it skips as step_not_configured exactly as DpuHealthCheck already does for providers without check_dpu_health. Claims no plan id: BMAAS06-01 is proven over SSH by the nvlink, infiniband, and ethernet checks, and an API reporting an interface "Ready" does not establish that nodes can communicate over it. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
…ry run Both AWS-local checks predate the rule that checks live in suites, and both overlapped coverage the image-registry suite already owns. BmImageInstallationVerifiedCheck moves into the bare-metal suite claiming BOOT01-03. AWS proves that id from its bare-metal run because its image-registry run launches a VM, not a metal host, so it implements neither install_image_bm nor install_config_bm. BOOT01-03 now has two implementers, so the plan item and both checks carry the union of their labels, as the label-sync guardrail requires. BmInstallConfigUsableCheck is deleted with its step and script: an EC2 DryRun proves only that EC2 would accept the launch template, which is not BOOT01-04's "installed on a BMaaS system" and adds little over BOOT01-02's launch-template CRUD coverage. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
…irings Review found the branch violating its own README rule: VpcCrudCheck-* and SgCrudCheck-* spent eight catalog identities on a class name plus a suffix while each maps to a distinct plan id. They become named composites over VpcCrudCheck/SgCrudCheck. The other variant names are parameter variants of one property and stay as they are. BmImageInstallationVerifiedCheck overclaimed: verify_image_installed.py describes an already-running host's image id and installs nothing, so it is BmHostRunsExpectedImageCheck. That also stops it competing with BmHostBootedFromCustomImageCheck for BOOT01-03. SDN01-02's two evidence paths now say which is which: VpcReadCheck for the CRUD lifecycle, VpcReadFromInventoryCheck for a standalone read of a pre-provisioned VPC. Also: the teardown pair carries claimed-vs-confirmed in its names and descriptions, boot names take the past tense of their neighbours, and CompositeCheck now fails on a malformed compose member instead of silently running fewer checks than the config names. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Review noted the prefix rule was "prefix when it would otherwise collide", so a reader could not guess whether a bare-metal test is X or BmX: BmGpusPresentCheck sat beside GpuStressCheck, NcclCheck and NvlinkCheck, and vm.yaml called its subject both Vm and Instance. All 34 classes were wired by exactly one suite, so each is a plain class rename rather than a composite wrapper. Two names change more than their prefix: SpecifiedKeyAccessCheck becomes BmComponentKeyAccessCheck so the AUTH03-01 pair reads as siblings instead of colliding with AUTH02-01's VmLaunchedWithSpecifiedKeyCheck, and DpuNetworkCheck becomes BmDpuNetworkReadinessCheck. bare_metal.yaml and vm.yaml now have no unprefixed wiring names. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
IAM01-01 is "create user and log in as that user", but the id sat on a check that only proved the create step succeeded and returned a username and access key. Issuing a key is not logging in. The test_credentials step already reports identity and access as separate probes, and IamCredentialAccessCheck already takes required_tests, so each plan item can prove its own half over the same output: logging in (IAM01-01) and reaching an authorized resource once logged in (IAM03-01). Creation drops to N/A as the supporting check it is, keeping min_req so a min_req-only run still creates the user the credential checks need. The check's pass message named authorized-resource access unconditionally, which was wrong once identity could be required alone; it now reports the probes it actually required. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
The prefix sweep stamped Bm onto nine checks whose plan items are not bare-metal-scoped: CAP01-01 fleet-wide governance counts, CAP05-02 aggregation at cluster/nodegroup/reservation level, SDN04-04/05 IB fabric, STG02/03/04/05 storage service, and HWING01-01 fleet inventory. They sit in bare_metal.yaml because that is where NICo's config imports from, not because the requirement is per-host, so the name asserted a scope the plan does not have. Our own labels already said so - the IB pair carries network, the STG four carry sds_controller. It would also have cost us later: names are globally unique and a wiring holds one test_id, so the first of these to need a network.yaml or storage.yaml home would have produced the two-names-one-id split we just undid for BOOT01-03. The prefix rule is now stated in the suites README: it follows the subject (one host, one VM), not the suite and not the test_id's requirement family, which is why BmHardwareSerialCheck (BFX03-01) and BmCloudInitCheck (BOOT02-01) keep theirs. Suite placement for these nine is a separate question, since moving them means NICo importing those suites too. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
compose_only was authoring-time only: validate_suite_wiring rejects a generic check wired under its own name, but an ISV's config is never linted, so it would run and report a pass under a name that says nothing about the property proven. parse_validations now rejects it too, as an invalid-config error naming the fix. Config parsing is the right place, not class resolution: the rule is about how a check is wired, so putting it there leaves internal pre-resolved entries alone. Four fixtures wired the generics directly and now compose them instead, which is what the rule asks of any config. vm.yaml also had two rows on launch_instance for one event - VmRunningCheck at N/A beside VmCreatedCheck at CNP01-09 - where bare_metal.yaml proves the same launch with one. The class becomes VmInstanceIdReportedCheck so the VmCreatedCheck composite can claim CNP01-09 over both the returned id and the running state. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
--dry-run read only the CLI's --exclude-label, ignoring the exclude block in the config itself, so it promised checks a real run then skips. The AWS bare-metal plan listed BmSerialConsoleRetentionCheck as [RUN] even though that config excludes it by name, and k3s listed ten workload checks its own exclude labels always drop. Both halves of the block now apply, as the orchestrator already does: labels union with the CLI's, and names are matched first to mirror resolve_entries' precedence. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
A step carrying skip: true is deliberately left out of the step-phase map so its validations skip with it, but that reused the "no such step" path, so AWS's four reinstall checks reported step_not_configured for a step the config declares. The two cases differ in what an operator can do: skip: true is one flag away from coverage, a step the provider never declares is not. resolve_entries now takes the skipped step names and reports step_skipped with "configured but skipped (skip: true)". Checks bound to a step the provider genuinely lacks - AWS has no check_dpu_health or verify_ingestion - keep reporting step_not_configured. Note for downstream: skip reasons become the type attribute on JUnit <skipped> elements, so step_skipped is a new value there. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
📝 WalkthroughWalkthroughThis PR standardizes validation identities, introduces named composite wiring with compose-only primitives, updates suite and provider configurations, changes catalog and skipped-step handling, adds NICo VPC/subnet inventory checks, and aligns tests, documentation, demos, and test-plan labels. ChangesValidation contracts and runtime
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🔐 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-07-29 23:25:37 UTC | Commit: 3e04500 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/guides/configuration.md (1)
566-574: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument compose-only generic checks.
The generic-check tables imply all listed classes can be wired directly, but direct wiring of
StepSuccessCheck,FieldExistsCheck,FieldValueCheck, andCrudOperationsCheckis rejected. Explain that these mechanisms must be referenced through a named composite; keep directly wireable checks distinct.
docs/guides/configuration.md#L566-L574: Mark the four compose-only checks and show that they belong under a namedcompose:wrapper.docs/packages/isvtest.md#L61-L71: Add the same compose-only distinction to the package validation table.🤖 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 `@docs/guides/configuration.md` around lines 566 - 574, The generic validation documentation must distinguish compose-only checks from directly wireable checks. In docs/guides/configuration.md lines 566-574, mark StepSuccessCheck, FieldExistsCheck, FieldValueCheck, and CrudOperationsCheck as compose-only and show they must be referenced under a named compose: wrapper, while keeping SchemaValidation directly wireable; apply the same distinction in docs/packages/isvtest.md lines 61-71.
🧹 Nitpick comments (2)
scripts/validate_suite_wiring.py (1)
122-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHoist the duplicated
discover_all_testsimport to the top of the module.Both helpers import the same symbol locally with no stated reason, and the module already imports freely from
isvtestat the top.♻️ Proposed fix
from isvtest.core.composite import COMPOSE_KEY, composed_members, is_composite +from isvtest.core.discovery import discover_all_tests`@cache` def discovered_check_names() -> frozenset[str]: """Return the names of every discoverable validation class.""" - from isvtest.core.discovery import discover_all_tests - return frozenset(cls.__name__ for cls in discover_all_tests()) `@cache` def compose_only_check_names() -> frozenset[str]: """Return the checks that may only be reached from inside a composite.""" - from isvtest.core.discovery import discover_all_tests - return frozenset(cls.__name__ for cls in discover_all_tests() if getattr(cls, "compose_only", False))As per coding guidelines: "Place all imports at the top of the file; defer imports inside functions only with a one-line comment giving the reason ...; if a function-local import is from a module that's already imported at the top, fold it into the top-level import".
🤖 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 122 - 136, Move the shared discover_all_tests import from the cached helpers discovered_check_names and compose_only_check_names to the module-level imports, then remove both function-local imports while preserving each helper’s existing discovery and filtering behavior.Source: Coding guidelines
isvtest/src/isvtest/core/composite.py (1)
88-93: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
run()is missing a docstring.Every other public method in the module carries one; the repo guideline requires docstrings on all functions.
♻️ Proposed fix
def run(self) -> None: + """Execute each composed member as a subtest and aggregate the verdict.""" raw = self.config.get(COMPOSE_KEY)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 `@isvtest/src/isvtest/core/composite.py` around lines 88 - 93, Add a PEP 257-compliant docstring to the public Composite.run method describing its execution of the configured composite check and failure handling for invalid members. Keep the existing behavior unchanged.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 `@docs/test-plan.yaml`:
- Around line 80-83: Regenerate the derived test plan using make plan so the
docs/test-plan.adoc row for BOOT01-03 includes the newly added bare_metal label,
then commit the synchronized generated output.
In `@isvctl/configs/providers/aws/config/bare_metal.yaml`:
- Around line 22-38: The documented bare-metal workflow is missing configured
steps and has incorrect numbering. In
isvctl/configs/providers/aws/config/bare_metal.yaml lines 22-38, add verify_tags
after list_instances and host_status_log after describe_instance, then renumber
subsequent steps; in
isvctl/configs/providers/aws/scripts/bare_metal/docs/aws-bm.md lines 71-80, add
host_status_log after describe_instance and renumber the remaining rows.
In `@isvctl/configs/providers/aws/scripts/vm/docs/aws-vm.md`:
- Around line 207-208: Complete the validation-name documentation rename: in
isvctl/configs/providers/aws/scripts/vm/docs/aws-vm.md (lines 207-208), rename
the HostSoftwareCheck section heading to VmSoftwareStackCheck or explicitly
document the alias; in isvctl/configs/providers/my-isv/config/bare_metal.yaml
(lines 25-34), update the later SSH-exclusion comment to use the corresponding
Bm-prefixed validation names.
In `@isvctl/configs/suites/bare_metal.yaml`:
- Around line 429-491: Update BmHostReadyAfterPowerCycleCheck,
BmGpusPresentAfterPowerCycleCheck, BmHostReadyAfterReinstallCheck, and
BmGpusPresentAfterReinstallCheck to explicitly bind SSH host and key_file to
their corresponding power_cycle_instance or reinstall_instance step outputs,
matching the existing start/reboot check pattern. Verify those step outputs
provide the refreshed IP and key path; if not, expose or propagate those values
before the checks run.
- Around line 335-342: Add expected_gpus: 8 to the BmGpusPresentAfterStartCheck
suite entry, matching BmGpusPresentCheck and the reboot, power-cycle, and
reinstall GPU checks while leaving its existing GpuCheck composition unchanged.
In `@isvctl/src/isvctl/cli/test.py`:
- Around line 184-185: Update the dry-run label filtering near rejected_labels
to match the precedence used by the orchestrator loop: when CLI include labels
or explicit pytest selection is provided, do not apply
config.tests.exclude.labels; otherwise retain those configured exclusions.
Prefer reusing or extracting a shared helper so dry-run and execution
consistently resolve label exclusions.
In `@isvtest/src/isvtest/core/resolution.py`:
- Around line 166-181: The _compose_only_error function must resolve validation
classes using the same variant-aware mechanism as wiring validation. Replace the
direct get_validation_class(name) lookup with resolve_class_key-based resolution
so names such as StepSuccessCheck-teardown still identify compose_only classes
and trigger the existing error.
In `@isvtest/src/isvtest/tests/test_validations.py`:
- Around line 187-190: Update the configured-class and skipped-test handling
around _resolve_validation_class so validations marked compose_only are not
emitted as top-level skipped tests when show_skipped_tests is enabled. Preserve
their execution as subtests of the resolved composite, and add a regression test
covering this behavior.
In `@isvtest/tests/test_composite.py`:
- Around line 229-264: Move the function-local imports of pytest and
validation/discovery symbols in the affected tests to module scope, unless a
specific import must remain deferred. For any deferred import, add a concise
one-line comment immediately explaining the deferral; update the tests around
test_generic_checks_are_marked_compose_only,
test_purpose_built_checks_are_not_compose_only, and
test_is_excluded_from_discovery accordingly.
---
Outside diff comments:
In `@docs/guides/configuration.md`:
- Around line 566-574: The generic validation documentation must distinguish
compose-only checks from directly wireable checks. In
docs/guides/configuration.md lines 566-574, mark StepSuccessCheck,
FieldExistsCheck, FieldValueCheck, and CrudOperationsCheck as compose-only and
show they must be referenced under a named compose: wrapper, while keeping
SchemaValidation directly wireable; apply the same distinction in
docs/packages/isvtest.md lines 61-71.
---
Nitpick comments:
In `@isvtest/src/isvtest/core/composite.py`:
- Around line 88-93: Add a PEP 257-compliant docstring to the public
Composite.run method describing its execution of the configured composite check
and failure handling for invalid members. Keep the existing behavior unchanged.
In `@scripts/validate_suite_wiring.py`:
- Around line 122-136: Move the shared discover_all_tests import from the cached
helpers discovered_check_names and compose_only_check_names to the module-level
imports, then remove both function-local imports while preserving each helper’s
existing discovery and filtering behavior.
🪄 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: 67074046-ada2-466e-90de-d9d3e522e6cf
📒 Files selected for processing (88)
AGENTS.mdMakefiledocs/guides/configuration.mddocs/packages/isvtest.mddocs/test-plan.adocdocs/test-plan.yamlisvctl/configs/providers/aws/config/bare_metal.yamlisvctl/configs/providers/aws/config/eks.yamlisvctl/configs/providers/aws/scripts/bare_metal/docs/aws-bm.mdisvctl/configs/providers/aws/scripts/bare_metal/host_status_log.pyisvctl/configs/providers/aws/scripts/bare_metal/verify_terminated.pyisvctl/configs/providers/aws/scripts/control-plane/docs/aws-control-plane.mdisvctl/configs/providers/aws/scripts/image-registry/docs/aws-image-registry.mdisvctl/configs/providers/aws/scripts/image-registry/verify_config_installable.pyisvctl/configs/providers/aws/scripts/network/docs/aws-network.mdisvctl/configs/providers/aws/scripts/vm/docs/aws-vm.mdisvctl/configs/providers/k3s.yamlisvctl/configs/providers/microk8s.yamlisvctl/configs/providers/minikube.yamlisvctl/configs/providers/my-isv/config/bare_metal.yamlisvctl/configs/providers/my-isv/config/image-registry.yamlisvctl/configs/providers/my-isv/config/network.yamlisvctl/configs/providers/my-isv/config/vm.yamlisvctl/configs/providers/my-isv/scripts/bare_metal/host_status_log.pyisvctl/configs/providers/my-isv/scripts/bare_metal/verify_terminated.pyisvctl/configs/providers/nico/config/bare_metal.yamlisvctl/configs/providers/nico/config/control-plane.yamlisvctl/configs/providers/nico/config/iam.yamlisvctl/configs/providers/nico/config/key_access.yamlisvctl/configs/providers/nico/config/network.yamlisvctl/configs/providers/nico/scripts/attestation/query_attestation.pyisvctl/configs/providers/nico/scripts/common/inventory.pyisvctl/configs/providers/nico/scripts/hardware_inventory/query_serial_numbers.pyisvctl/configs/providers/nico/scripts/network/check_subnet_assignment.pyisvctl/configs/providers/nico/scripts/network/get_vpc.pyisvctl/configs/providers/nico/scripts/network/list_vpcs.pyisvctl/configs/providers/nico/scripts/network/test_connectivity.pyisvctl/configs/suites/README.mdisvctl/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/security.yamlisvctl/configs/suites/storage.yamlisvctl/configs/suites/vm.yamlisvctl/src/isvctl/cli/catalog.pyisvctl/src/isvctl/cli/docs.pyisvctl/src/isvctl/cli/test.pyisvctl/src/isvctl/orchestrator/loop.pyisvctl/tests/providers/nico/test_nico_provider.pyisvctl/tests/test_catalog_cli.pyisvctl/tests/test_cli_streams.pyisvctl/tests/test_merger.pyisvctl/tests/test_orchestrator_loop.pyisvctl/tests/test_provider_scaffold_cli.pyisvtest/src/isvtest/catalog.pyisvtest/src/isvtest/core/composite.pyisvtest/src/isvtest/core/resolution.pyisvtest/src/isvtest/core/validation.pyisvtest/src/isvtest/tests/test_validations.pyisvtest/src/isvtest/validations/__init__.pyisvtest/src/isvtest/validations/attestation.pyisvtest/src/isvtest/validations/bm_host_status.pyisvtest/src/isvtest/validations/generic.pyisvtest/src/isvtest/validations/hardware.pyisvtest/src/isvtest/validations/health.pyisvtest/src/isvtest/validations/host.pyisvtest/src/isvtest/validations/iam.pyisvtest/src/isvtest/validations/instance.pyisvtest/src/isvtest/validations/key_access.pyisvtest/src/isvtest/validations/reframe_ib_check.pyisvtest/src/isvtest/validations/sanitization.pyisvtest/src/isvtest/validations/security.pyisvtest/tests/test_attestation.pyisvtest/tests/test_catalog.pyisvtest/tests/test_composite.pyisvtest/tests/test_hardware.pyisvtest/tests/test_health.pyisvtest/tests/test_iam.pyisvtest/tests/test_instance.pyisvtest/tests/test_key_access.pyisvtest/tests/test_resolution.pyisvtest/tests/test_sanitization.pyisvtest/tests/test_validation.pyscripts/tests/test_validate_suite_wiring.pyscripts/validate_suite_wiring.py
💤 Files with no reviewable changes (2)
- isvctl/configs/providers/aws/scripts/image-registry/verify_config_installable.py
- isvctl/configs/providers/nico/scripts/network/test_connectivity.py
IamCredentialsAuthenticateCheck is a whoami on the run's own credentials, for platforms whose read-only IAM cannot create a user and log in as them. AWS does exactly that, so IAM01-01 and IAM03-01 already prove the stronger property and the check only added a skipped line to every AWS IAM run. Excluded by name rather than removed from the suite: NICo implements check_credentials and still needs it. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@isvctl/configs/providers/aws/config/iam.yaml`:
- Around line 77-82: Remove IamCredentialsAuthenticateCheck from the AWS
provider’s exclude.tests list so configured-credentials identity validation
remains enabled. If an exclusion is still required for the stated read-only IAM
limitation, target only the lifecycle check that creates and logs in as a newly
created IAM user.
🪄 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: 1c76aa92-20eb-40de-8233-2f26a3e51df8
📒 Files selected for processing (1)
isvctl/configs/providers/aws/config/iam.yaml
Keep dry-run filtering and compose-only enforcement aligned with runtime behavior, and synchronize the related suite and operator documentation. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
isvctl/configs/suites/bare_metal.yaml (1)
430-451: 🎯 Functional Correctness | 🟠 MajorUse refreshed connection details for power-cycle and reinstall checks.
The start/reboot wrappers explicitly use
steps.describe_instance.public_ipandkey_file, but the power-cycle and reinstall SSH/GPU wrappers omit them. These checks can therefore use stale or missing connection data after the lifecycle operation. Add the same overrides, or verify that those step outputs always contain the refreshed values.Also applies to: 472-493
🤖 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 `@isvctl/configs/suites/bare_metal.yaml` around lines 430 - 451, Update the power-cycle and reinstall SSH/GPU check compositions to pass refreshed connection details after the lifecycle operation, matching the existing `steps.describe_instance.public_ip` and `key_file` overrides used by the start/reboot wrappers. Apply this to `power_cycle_ssh`, `power_cycle_gpu`, and the corresponding reinstall checks, or verify their step outputs always provide those refreshed values.
🤖 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.
Duplicate comments:
In `@isvctl/configs/suites/bare_metal.yaml`:
- Around line 430-451: Update the power-cycle and reinstall SSH/GPU check
compositions to pass refreshed connection details after the lifecycle operation,
matching the existing `steps.describe_instance.public_ip` and `key_file`
overrides used by the start/reboot wrappers. Apply this to `power_cycle_ssh`,
`power_cycle_gpu`, and the corresponding reinstall checks, or verify their step
outputs always provide those refreshed values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: aa1b9aa2-cb0c-4c88-93c0-8b8d70c4340d
📒 Files selected for processing (17)
docs/guides/configuration.mddocs/packages/isvtest.mddocs/test-plan.adocisvctl/configs/providers/aws/config/bare_metal.yamlisvctl/configs/providers/aws/config/iam.yamlisvctl/configs/providers/aws/scripts/bare_metal/docs/aws-bm.mdisvctl/configs/providers/aws/scripts/vm/docs/aws-vm.mdisvctl/configs/providers/my-isv/config/bare_metal.yamlisvctl/configs/suites/bare_metal.yamlisvctl/src/isvctl/cli/test.pyisvctl/tests/test_cli_streams.pyisvtest/src/isvtest/core/composite.pyisvtest/src/isvtest/core/resolution.pyisvtest/src/isvtest/tests/test_validations.pyisvtest/tests/test_composite.pyisvtest/tests/test_pytest_generation.pyscripts/validate_suite_wiring.py
🚧 Files skipped from review as they are similar to previous changes (7)
- isvctl/configs/providers/aws/config/iam.yaml
- docs/guides/configuration.md
- docs/test-plan.adoc
- isvctl/configs/providers/my-isv/config/bare_metal.yaml
- isvctl/configs/providers/aws/scripts/bare_metal/docs/aws-bm.md
- scripts/validate_suite_wiring.py
- isvctl/configs/providers/aws/config/bare_metal.yaml
check_credentials made the same two calls as NICo's control-plane check_api - forge_get on the site, then forge_get_all over sites - with the probes renamed. CP03-01 already covers "an authenticated request scoped to the account reaches the platform API", and it maps to a plan item where the IAM copy was N/A. Hoisting it into the shared suite also made every AWS IAM run report a skipped row for a check AWS has no step for, which needed muting. Removing it from the suite drops both the row and the mute: AWS's IAM run is four checks and no skips again. NICo's script stays on disk unwired, in case its IAM story changes. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Enumerating every validation in header comments drifts as suites change; describe JSON-contract coverage and SSH exclusions instead. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
The prefix sweep and the walk-back after it left four names claiming more than they prove. Each was globally unique before the sweep, so the prefix bought consistency only, and each cost a released_tests.json entry. VirtualDeviceHardeningCheck (CNP01-17) already carries its subject: VmVirtualDeviceHardening stutters, and USB/clipboard/virtual-device passthrough has no bare-metal reading to distinguish it from. SerialConsoleRetentionCheck (CNP06-02) asserts that the console archive keeps a month of history, which belongs to the logging service rather than to a host. It is also the one console plan item that is not platform-scoped, unlike CNP06-01 (bare metal) and CNP06-03 (VM). HostHealthCheck (CAP05-01) is per-host by subject, but it is one half of a pair whose other half - HealthAggregationCheck (CAP05-02) - is fleet-level and already unprefixed. Prefixing one half hid that the two are one requirement seen at two scopes. BmDpuNetworkCheck keeps its prefix, since a DPU is per-host, and drops "Readiness": the check asserts interface status, BGP, and extension deployments, which the vaguer word described less well. It is free to change, carrying test_id N/A with no implementation yet. Three of the four land on names already in released_tests.json, so unreleased wiring names drop 125 -> 122 and dangling manifest entries 25 -> 22. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
isvctl/tests/providers/nico/test_nico_provider.py (1)
557-557: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRetain equivalent coverage for the removed IAM wiring tests.
Removing the plain-suite and caller-identity assertions is safe only if those paths were intentionally deleted or replacement tests now cover the shared wiring. The duplicate credential-probe removal alone does not establish that caller-identity regression coverage is still preserved.
🤖 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 `@isvctl/tests/providers/nico/test_nico_provider.py` at line 557, Update test_nico_check_credentials_reports_api_readiness and the surrounding Nico provider tests to retain equivalent coverage for the removed IAM wiring and caller-identity assertions. Verify that replacement tests exercise the shared wiring and caller-identity behavior; if those paths remain supported, restore focused assertions rather than relying solely on the credential-probe test.
🤖 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 `@isvctl/tests/providers/nico/test_nico_provider.py`:
- Line 557: Update test_nico_check_credentials_reports_api_readiness and the
surrounding Nico provider tests to retain equivalent coverage for the removed
IAM wiring and caller-identity assertions. Verify that replacement tests
exercise the shared wiring and caller-identity behavior; if those paths remain
supported, restore focused assertions rather than relying solely on the
credential-probe test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 000a4ab6-9c0f-4d96-9181-72c0e2fc06ee
📒 Files selected for processing (22)
isvctl/configs/providers/aws/config/bare_metal.yamlisvctl/configs/providers/aws/scripts/bare_metal/docs/aws-bm.mdisvctl/configs/providers/my-isv/config/bare_metal.yamlisvctl/configs/providers/my-isv/config/image-registry.yamlisvctl/configs/providers/my-isv/config/network.yamlisvctl/configs/providers/my-isv/config/vm.yamlisvctl/configs/providers/nico/config/bare_metal.yamlisvctl/configs/providers/nico/config/iam.yamlisvctl/configs/suites/README.mdisvctl/configs/suites/bare_metal.yamlisvctl/configs/suites/iam.yamlisvctl/configs/suites/vm.yamlisvctl/tests/providers/nico/test_nico_provider.pyisvctl/tests/test_merger.pyisvtest/src/isvtest/validations/__init__.pyisvtest/src/isvtest/validations/hardware.pyisvtest/src/isvtest/validations/health.pyisvtest/src/isvtest/validations/instance.pyisvtest/src/isvtest/validations/security.pyisvtest/tests/test_hardware.pyisvtest/tests/test_instance.pyisvtest/tests/test_validation.py
💤 Files with no reviewable changes (2)
- isvctl/configs/providers/nico/config/iam.yaml
- isvctl/configs/suites/iam.yaml
🚧 Files skipped from review as they are similar to previous changes (7)
- isvctl/configs/providers/my-isv/config/vm.yaml
- isvctl/configs/providers/my-isv/config/network.yaml
- isvctl/configs/providers/nico/config/bare_metal.yaml
- isvctl/configs/providers/my-isv/config/image-registry.yaml
- isvctl/configs/providers/my-isv/config/bare_metal.yaml
- isvctl/configs/providers/aws/scripts/bare_metal/docs/aws-bm.md
- isvctl/configs/suites/README.md
3 tasks
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
1 participant
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
Every test in the catalog now has a globally unique name that says what it proves. Previously 55 wiring names collided — a dozen rows all called
StepSuccessCheckorFieldExistsCheck— and because the catalog and the report key on that name, colliding rows silently overwrote each other.A check may now be declared as a composite: it carries the name,
test_id, labels and description, and lists the generic checks that establish it undercompose:.CompositeCheckruns each member as a subtest, so a failure still names the part that broke.The generic checks are marked
compose_onlyand are reachable only from inside acompose:list.validate_suite_wiringenforces that plus global name uniqueness in-tree;parse_validationsenforces it at runtime too, since an ISV's own config is never linted.Also in scope:
suites/only. NICo's four checks, AWS's image check and the orphanedDpuNetworkChecknow have suite homes. The six on-host GPU checks ink3s/microk8s/minikubestay out deliberately as local-dev tools, documented inAGENTS.md.Bm/Vmnow follows the subject — one host, one VM — not the suite or thetest_id's requirement family. Platform, fabric and storage-service checks stay unprefixed even insidebare_metal.yaml. The rule is written down insuites/README.md.identityprobe while IAM03-01 keepsaccess. AWS's install-config dry run is deleted — an EC2DryRunproves EC2 would accept the launch template, not that a host was installed from it, and it carried notest_id.--dry-runignored the config's ownexcludeblock, so it promised checks a real run skips (ten workload checks fork3s). And a step carryingskip: truereportedstep_not_configured, which is wrong — that case is nowstep_skipped.Release requirement
This must land with a version bump. 97 catalog entries are absent from
released_tests.json, which is a pre-#561 snapshot, so on merge they gate asunreleasedand the VM, bare-metal, storage, IAM and image-registry suites go quiet while still reporting green.make bump-minorregenerates the manifest and resolves it.That regen drops 44 names. All are accounted for: 28 are generics and composite members that are no longer wiring names (gating keys on the wiring name, so this is inert), 7 are base names replaced by their own variants, 6 are the intentional local-dev checks, and 3 are stale variants replaced in this branch.
Downstream note
Skip reasons become the
typeattribute on JUnit<skipped>elements, sostep_skippedis a new value the service may see.Deferred
Three items need a decision rather than code, and are deliberately not in this PR:
CustomOsImageCrudCheck(BOOT03-02).BmCloudInitCheck(BOOT02-01).Both need one check to declare two plan ids, which is the open question: whether several names on one
test_idare separate scenarios or alternate proofs. Also open is suite placement for the nine platform-level checks, which requires NICo to import those suites and supply the steps there.Test plan
validate_suite_wiring— 287 wiring names, all uniquetest_plan_coverage --check— 230 mapped classes pass integrity, consistency and label syncisvtest,isvctlandscriptsmake demo-test— all nine suites end to endpre-commit run -ag4dn.metal: 67 validations, 39 passed, 28 skipped, 75 subtests, zero failuresSummary by CodeRabbit
New Features
Bug Fixes
Documentation