Feature/vm extension test base update - #4610
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a shared VmExtensionTestBase to reduce duplicated VM extension lifecycle logic across publisher-owned extension test suites, and refactors several runtime extension suites to use it. It also adds onboarding documentation for the framework.
Changes:
- Added
VmExtensionTestBasewith reusable helpers for install/uninstall/lifecycle assertions and command verification. - Refactored
CustomScript,RunCommand v1, andRunCommand v2runtime extension suites to use the shared base helpers and consistent tagging. - Added documentation describing conventions (naming, runbook variables, and onboarding workflow) and linked it from the docs index.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| lisa/microsoft/testsuites/vm_extensions/vm_extension_base.py | New shared base class for extension lifecycle helpers. |
| lisa/microsoft/testsuites/vm_extensions/runtime_extensions/run_commandv2.py | Refactor to inherit from VmExtensionTestBase, adjust tagging, and route test cases through shared helper. |
| lisa/microsoft/testsuites/vm_extensions/runtime_extensions/run_commandv1.py | Refactor to inherit from VmExtensionTestBase and route test cases through shared helper. |
| lisa/microsoft/testsuites/vm_extensions/runtime_extensions/custom_script.py | Refactor to inherit from VmExtensionTestBase, simplify boot validation, and route test cases through shared helper. |
| docs/vm_extension_validation_framework.rst | New onboarding/strategy documentation for publisher-owned extension validation. |
| docs/index.rst | Adds the new onboarding doc to the documentation TOC. |
AI Test Case SelectionSelected 12 test case(s): verify_public_script_run,verify_second_public_script_run,verify_script_in_both_settings_failed,verify_public_script_protected_settings_run,verify_public_script_without_command_run_failed,verify_base64_script_with_command_run_failed,verify_public_script_with_base64_script_run,verify_public_script_with_gzip_base64_script_run,verify_private_script_without_sas_run_failed,verify_private_script_with_storage_credentials_run,verify_private_sas_script_run,verify_public_python_script_run Marketplace image: Result: Failed |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (4)
lisa/microsoft/testsuites/vm_extensions/vm_extension_base.py:87
- Major: _install() always calls AzureExtension.delete() even when SUPPORTS_DELETE is False. This contradicts the class contract (CRP-managed extensions) and will raise on extensions that cannot be deleted; guard the pre-delete behind SUPPORTS_DELETE.
version = self._get_version(variables)
extension = node.features[AzureExtension]
extension.delete(name=self.extension_name, ignore_not_found=True)
result: Dict[str, Any] = extension.create_or_update(
lisa/microsoft/testsuites/vm_extensions/vm_extension_base.py:109
- Major: _uninstall() always deletes the extension even when SUPPORTS_DELETE is False. For CRP-managed extensions this can fail and break lifecycle helpers; make uninstall a no-op when deletion is unsupported.
def _uninstall(self, node: Node) -> None:
"""Remove the extension from the node."""
extension = node.features[AzureExtension]
extension.delete(name=self.extension_name, ignore_not_found=True)
lisa/microsoft/testsuites/vm_extensions/vm_extension_base.py:64
- _get_version() coerces the runbook value with str(...), so a None value becomes the literal string "None" and will be treated as a version. Treat missing/None as empty so the DEFAULT_VERSION/skip behavior works as intended.
version = str(variables.get(self.version_variable, "")).strip()
if not version:
version = self.DEFAULT_VERSION.strip()
if not version:
docs/vm_extension_validation_framework.rst:145
- The runbook filtering example uses tag "RunCommandV2", but the refactored suite uses EXTENSION_TYPE as the extension-specific tag (RunCommandHandlerLinux). This mismatch makes the doc example non-functional.
# Run ONLY RunCommand v2 tests
- criteria:
tags: RunCommandV2
AI Test Case SelectionSelected 25 test case(s): verify_azuremonitoragent_linux,verify_application_health_extension,verify_azsecpack,verify_azure_disk_encryption_enabled,verify_azure_disk_encryption_provisioned,verify_key_vault_extension,verify_azure_performance_diagnostics,verify_asr_by_cvt,verify_asr_by_cvt_no_extension,verify_vm_extension_install_uninstall,verify_vm_assess_patches,verify_vm_install_patches,verify_metricsextension,verify_azure_network_watcher,verify_valid_password_run,verify_openssh_key_run,verify_password_and_ssh_key_run,verify_no_password_and_ssh_key_run_failed,verify_pem_certificate_ssh_key_run,verify_ssh2_key_run,verify_remove_username_run,verify_valid_expiration_run,verify_vmsnapshot_extension,verify_exclude_disk_support_restore_point,verify_vm_agent Marketplace image: Result: Failed |
Add a shared base class for publisher-owned VM extension test suites.
Provides reusable lifecycle helpers (_install, _assert_provisioned,
_uninstall, _assert_vm_reachable, _full_lifecycle) so publishers only
write extension-specific tests.
Extension version is always read from a runbook variable named
'{EXTENSION_KEY}_version', enabling multi-extension runs without code
changes.
Include RST documentation with architecture, naming conventions,
onboarding checklist, and runbook filtering examples.
Refactor CustomScriptTests to inherit VmExtensionTestBase instead of TestSuite. Set PUBLISHER, EXTENSION_TYPE, EXTENSION_KEY class constants. Convert module-level _create_and_verify_extension_run to class method using self._get_version(variables). Add variables parameter to all test methods. Mark all tests priority=5, maturity=preview. Add CustomScript tag for per-extension runbook filtering.
Refactor RunCommandV1Tests and RunCommandV2Tests to inherit VmExtensionTestBase instead of TestSuite. Set PUBLISHER, EXTENSION_TYPE, EXTENSION_KEY class constants. Convert module-level _create_and_verify_extension_run to class method using self._get_version(variables). Add variables parameter to all test method signatures.
Collapse single-line 'from lisa import' block, apply black formatting, cast _get_version to str and use a typed _install result to avoid no-any-return, and add '# type: ignore[misc]' to the three VmExtensionTestBase subclasses (namespace-package Any base). [AI Generated]
Deduplicate the three identical/near-identical _create_and_verify_extension_run copies (CustomScript, RunCommand v1/v2) into a single VmExtensionTestBase method. Add a SUPPORTS_DELETE class flag (False for CRP-managed RunCommand v2) to gate the pre-delete step, and drop the now-unused assert_that, execute_command, and Optional imports from the suites. [AI Generated]
Add _resolve_publisher() and _resolve_type() that check runbook variables first (extension_publisher, extension_type, extension_version), then fall back to class constants. This preserves the existing generic VM extension test contract alongside the dedicated suite pattern.
Move run_extension_boot_validation free function into VmExtensionTestBase._boot_validation. Add use_default flag to _get_version so boot validation requires an explicit version (no DEFAULT_VERSION fallback), while detailed cases keep the fallback. Skip the all-or-nothing extension_* check for dedicated suites that set PUBLISHER/EXTENSION_TYPE in code. RCv1/RCv2 boot validation now call self._boot_validation; delete gated by SUPPORTS_DELETE. Verified locally: boot validation without version -> SKIPPED; verify_existing_script_run without version -> PASSED via DEFAULT_VERSION=1.3.
3d0a907 to
3a3fb4c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (5)
lisa/microsoft/testsuites/vm_extensions/vm_extension_base.py:137
- When EXTENSION_KEY is empty (generic usage), version_variable becomes '_version', so this skip message tells users to set a runbook variable that doesn't exist. The message should only mention the dedicated '{EXTENSION_KEY}_version' variable when EXTENSION_KEY is set.
raise SkippedException(
f"No version set for {publisher}.{type_}: set runbook "
f"variable 'extension_version' or '{self.version_variable}'. "
f"Skipping."
)
lisa/microsoft/testsuites/vm_extensions/vm_extension_base.py:156
- _install() unconditionally deletes any existing extension by name and then passes the version string straight to Azure. This breaks the SUPPORTS_DELETE=False contract (CRP-managed extensions cannot be deleted) and can turn a malformed version into a hard Azure failure instead of a clean skip. Consider: (1) only pre-delete when SUPPORTS_DELETE is True, and (2) validate/normalize the version with AzureExtension.normalize_type_handler_version() before calling create_or_update().
publisher = self._resolve_publisher(variables)
type_ = self._resolve_type(variables)
version = self._get_version(variables)
extension = node.features[AzureExtension]
extension.delete(name=self.extension_name, ignore_not_found=True)
lisa/microsoft/testsuites/vm_extensions/vm_extension_base.py:183
- _uninstall() currently always calls AzureExtension.delete(), even when SUPPORTS_DELETE is False (CRP-managed extensions). That makes it too easy for callers to accidentally attempt an unsupported delete and fail the case. It should be a no-op when SUPPORTS_DELETE is False.
def _uninstall(self, node: Node) -> None:
"""Remove the extension from the node."""
extension = node.features[AzureExtension]
extension.delete(name=self.extension_name, ignore_not_found=True)
lisa/microsoft/testsuites/vm_extensions/vm_extension_base.py:311
- _create_and_verify_extension_run() passes the resolved version directly to AzureExtension.create_or_update(). Unlike _boot_validation(), it doesn't validate/normalize the version format first, so a malformed runbook value can become a hard Azure failure rather than a clean skip. It should reuse normalize_type_handler_version() and pass the normalized Major.Minor value to Azure.
publisher = self._resolve_publisher(variables)
type_ = self._resolve_type(variables)
version = self._get_version(variables)
extension = node.features[AzureExtension]
if self.SUPPORTS_DELETE:
extension.delete(name=self.extension_name, ignore_not_found=True)
docs/vm_extension_validation_framework.rst:170
- The doc says to filter RunCommand v2 tests with tag 'RunCommandV2', but the refactored suite sets tags=['VM_Extension', 'RunCommandHandlerLinux'] (matching EXTENSION_TYPE). The example runbook criteria should use the actual tag value so users can copy/paste it successfully.
# Run ONLY RunCommand v2 tests
- criteria:
tags: RunCommandV2
AI Test Case SelectionSelected 25 test case(s): verify_azuremonitoragent_linux,verify_application_health_extension,verify_azsecpack,verify_azure_disk_encryption_enabled,verify_azure_disk_encryption_provisioned,verify_key_vault_extension,verify_azure_performance_diagnostics,verify_asr_by_cvt,verify_asr_by_cvt_no_extension,verify_vm_extension_install_uninstall,verify_vm_assess_patches,verify_vm_install_patches,verify_metricsextension,verify_azure_network_watcher,verify_valid_password_run,verify_openssh_key_run,verify_password_and_ssh_key_run,verify_no_password_and_ssh_key_run_failed,verify_pem_certificate_ssh_key_run,verify_ssh2_key_run,verify_remove_username_run,verify_valid_expiration_run,verify_vmsnapshot_extension,verify_exclude_disk_support_restore_point,verify_vm_agent Marketplace image: Result: Failed |
CustomScript boot validation now uses _boot_validation (version required, no DEFAULT_VERSION fallback, delete gated by SUPPORTS_DELETE). Parameterize _install/_uninstall with optional name/version so _boot_validation, _full_lifecycle, and _create_and_verify_extension_run all reuse the shared install/uninstall/assert_provisioned/assert_vm_reachable core. _boot_validation verifies the installed version (asserts exact match for a Major.Minor.Patch request, mirroring GenericVmExtension) and checks VM reachability. Contract: boot validation skips without a version; detailed cases fall back to DEFAULT_VERSION.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (7)
lisa/microsoft/testsuites/vm_extensions/vm_extension_base.py:238
- _full_lifecycle() always uninstalls the extension in the finally block, but the class supports extensions that cannot be deleted via normal delete (SUPPORTS_DELETE=False). This will break lifecycle validation for CRP-managed extensions and contradicts the SUPPORTS_DELETE contract used elsewhere in this base class.
try:
self._assert_provisioned(result, variables)
log.info(
f"Extension '{self.extension_name}' "
f"({publisher}.{type_}) provisioned successfully."
)
self._assert_vm_reachable(node)
finally:
self._uninstall(node)
self._assert_vm_reachable(node)
lisa/microsoft/testsuites/vm_extensions/vm_extension_base.py:170
- _install() may proceed with an empty publisher/type if neither class constants nor runbook variables are set. That will lead to an AzureExtension.create_or_update() call with invalid parameters, producing a hard failure instead of a clear skip. Consider explicitly skipping when publisher/type are not resolved.
publisher = self._resolve_publisher(variables)
type_ = self._resolve_type(variables)
if version is None:
version = self._get_version(variables)
name = name or self.extension_name
extension = node.features[AzureExtension]
if self.SUPPORTS_DELETE:
extension.delete(name=name, ignore_not_found=True)
result: Dict[str, Any] = extension.create_or_update(
name=name,
publisher=publisher,
type_=type_,
lisa/microsoft/testsuites/vm_extensions/vm_extension_base.py:140
- The retry decorator introduces retry/backoff magic numbers (tries=3, delay=10) without any inline rationale. Per repo review guidelines, retries/timeouts should be documented to make test behavior easier to tune and audit.
@retry(tries=3, delay=10) # type: ignore
lisa/microsoft/testsuites/vm_extensions/vm_extension_base.py:189
- The retry decorator introduces retry/backoff magic numbers (tries=3, delay=10) without any inline rationale. Per repo review guidelines, retries/timeouts should be documented to make test behavior easier to tune and audit.
@retry(tries=3, delay=10) # type: ignore
docs/vm_extension_validation_framework.rst:171
- The docs recommend filtering RunCommand v2 tests by tag 'RunCommandV2', but the updated suite uses EXTENSION_TYPE as the extension-specific tag (e.g. 'RunCommandHandlerLinux'). Update this example so it matches the documented convention and the actual suite metadata.
# Run ONLY RunCommand v2 tests
- criteria:
tags: RunCommandV2
lisa/microsoft/testsuites/vm_extensions/runtime_extensions/custom_script.py:273
- This test expects an HttpResponseError (negative scenario) but the method name doesn’t indicate failure, unlike the analogous RunCommand tests (e.g. verify_public_script_without_command_run_failed). Renaming improves clarity and makes name-based filtering less error-prone.
def verify_public_script_without_command_run(
lisa/microsoft/testsuites/vm_extensions/runtime_extensions/custom_script.py:312
- This test uses assert_exception=HttpResponseError (negative scenario) but the method name doesn’t indicate failure. Consider renaming to include a '_failed' suffix for consistency with other VM extension tests and to avoid confusion when selecting tests by name.
def verify_base64_script_with_command_run(
AI Test Case SelectionSelected 25 test case(s): verify_azuremonitoragent_linux,verify_application_health_extension,verify_azsecpack,verify_azure_disk_encryption_enabled,verify_azure_disk_encryption_provisioned,verify_key_vault_extension,verify_azure_performance_diagnostics,verify_asr_by_cvt,verify_asr_by_cvt_no_extension,verify_vm_extension_install_uninstall,verify_vm_assess_patches,verify_vm_install_patches,verify_metricsextension,verify_azure_network_watcher,verify_valid_password_run,verify_openssh_key_run,verify_password_and_ssh_key_run,verify_no_password_and_ssh_key_run_failed,verify_pem_certificate_ssh_key_run,verify_ssh2_key_run,verify_remove_username_run,verify_valid_expiration_run,verify_vmsnapshot_extension,verify_exclude_disk_support_restore_point,verify_vm_agent Marketplace image: Result: Failed |
|
@johnsongeorge-w There are a few issues I've noticed. I'll investigate them tomorrow. For example, there are test cases with the same name but belonging to different test suites, and only one of them is being picked up: CustomScriptTests.verify_private_script_without_sas_run_failed |
Description
Related Issue
Type of Change
Checklist
Test Validation
Key Test Cases:
Impacted LISA Features:
Tested Azure Marketplace Images:
Test Results