Skip to content

refactor: convert tests to class-based structure with pytest-dependency#262

Merged
myakove merged 7 commits intomainfrom
feat/class-based-test-structure
Jan 20, 2026
Merged

refactor: convert tests to class-based structure with pytest-dependency#262
myakove merged 7 commits intomainfrom
feat/class-based-test-structure

Conversation

@myakove
Copy link
Collaborator

@myakove myakove commented Jan 20, 2026

Summary

  • Migrates all test files to use a class-based test structure with the 5-step pattern
  • Adds proper test ordering via pytest-dependency markers
  • Improves code organization and separation of concerns in migration utilities

Key Changes

Test Structure

  • Add class-scoped fixtures: class_plan_config, prepared_plan, class_multus_network_name for shared state across test methods
  • Convert all tests to 5-step pattern: storagemap -> networkmap -> plan -> migrate -> check_vms
  • Use @pytest.mark.dependency() decorators for test ordering

Migration Utilities

  • Extract create_plan_resource() and execute_migration() from run_migration() for better separation of concerns
  • Update check_vms() to read from plan["source_vms_data"] instead of separate parameters

VMware Provider

  • Refactor with get_changed_block_tracking_enabled() and is_warm_migration_supported() methods

Dependencies

  • Add pytest-dependency to project dependencies

Documentation

  • Update CLAUDE.md with test structure documentation and remove duplications

Test Plan

  • Run pytest --collect-only to verify test discovery
  • Run tier0 tests on a live cluster
  • Verify test ordering with pytest-dependency markers

Summary by CodeRabbit

  • New Features

    • Migration split into plan-creation and execution phases; helpers added to build plans, map resources, and auto-populate VM identifiers; logging reduced for noisy SSH banner messages.
  • Tests

    • Tests converted to class-based, ordered multi-step flows using class-scoped setup and teardown to speed and stabilize test runs.
  • Documentation

    • Major rewrite with clearer workflows, mandatory rules, stricter code/test standards, and detailed testing patterns.
  • Chores

    • Test tooling config updated; automated migration report generation removed and build context adjusted.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 20, 2026

Walkthrough

Refactors test architecture to class-scoped fixtures and an ordered five-step test pattern; centralizes plan preparation (VM clones, NADs, per-VM source_vms_data); replaces primitive-driven migration helpers with object-based APIs (get_*_migration_map, create_plan_resource, execute_migration); removes report generation module; adds pytest-dependency and suppresses noisy Paramiko banner logs.

Changes

Cohort / File(s) Summary
Documentation & Config
CLAUDE.md, pytest.ini, pyproject.toml
Major rewrite of CLAUDE.md (rules, MUST/SHOULD, stricter test patterns); removed --pdbcls=IPython... from pytest.ini; added pytest-dependency>=0.6.0 in pyproject.toml.
Test Fixture Architecture
conftest.py
Shifted fixtures to class scope: added class_plan_config, prepared_plan, cleanup_migrated_vms; changed vm_ssh_connections to class scope; multus_network_name signature updated to use class hash prefix and return base NAD name; per-class source_vms_data stored in prepared plan.
MTV Test Files
tests/test_mtv_cold_migration.py, tests/test_mtv_warm_migration.py
Converted procedural tests into class-based 5-step pattern (storagemap → networkmap → plan → migrate → check_vms) using new helpers and types (StorageMap, NetworkMap, Plan) and relying on prepared_plan.
Migration Utilities
utilities/mtv_migration.py, utilities/migration_utils.py, utilities/post_migration.py
Introduced object-based helpers: get_storage_migration_map, get_network_migration_map, create_plan_resource, execute_migration; removed prepare_migration_for_tests, migrate_vms, and create_storagemap_and_networkmap; run_migration updated to use new flow; post_migration now reads per-VM source data from plan["source_vms_data"].
Fixture Helpers & Utils
utilities/utils.py
Added generate_class_hash_prefix(nodeid, ...) and populate_vm_ids(plan, inventory); minor docstring improvement.
Logging & Providers
utilities/logger.py, libs/providers/vmware.py
Added ParamikoSSHBannerFilter to suppress Paramiko banner noise; adjusted VMware provider logging to use parameterized logging and LOGGER.exception in exception paths.
Removed Module & Build
report.py, Dockerfile
Removed report.py (report generation logic) and removed it from the Dockerfile COPY step.
Other
pytest.ini
Removed explicit IPython pdb class from pytest options.

Sequence Diagram(s)

sequenceDiagram
    actor TestRunner
    participant Pytest as "pytest (conftest.py)"
    participant ClassCfg as "class_plan_config"
    participant Prepared as "prepared_plan (class fixture)"
    participant VMSetup as "VM Clone & NAD Setup"
    participant Tests as "Test Class (5-step)"
    participant MigrationAPI as "utilities.mtv_migration"
    participant Cleanup as "cleanup_migrated_vms"

    TestRunner->>Pytest: collect tests
    Pytest->>ClassCfg: resolve class_plan_config
    Pytest->>Prepared: initialize prepared_plan
    Prepared->>VMSetup: deep-copy plan, clone VMs, create NADs
    VMSetup-->>Prepared: store source_vms_data
    Prepared->>Tests: provide prepared_plan

    Tests->>MigrationAPI: get_storage_migration_map / get_network_migration_map
    Tests->>MigrationAPI: create_plan_resource
    Tests->>MigrationAPI: execute_migration
    MigrationAPI-->>Tests: migration result / status

    Tests->>Cleanup: class teardown triggers deletion
    Cleanup-->>TestRunner: teardown complete
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested labels

size/XL

Suggested reviewers

  • krcmarik
  • rnetser
  • Chenli-Hu
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: converting tests from a procedural pattern to a class-based structure using pytest-dependency for ordering.
Docstring Coverage ✅ Passed Docstring coverage is 81.25% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/class-based-test-structure

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@redhat-qe-bot1
Copy link

Report bugs in Issues

Welcome! 🎉

This pull request will be automatically processed with the following features:

🔄 Automatic Actions

  • Reviewer Assignment: Reviewers are automatically assigned based on the OWNERS file in the repository root
  • Size Labeling: PR size labels (XS, S, M, L, XL, XXL) are automatically applied based on changes
  • Issue Creation: Disabled for this repository
  • Branch Labeling: Branch-specific labels are applied to track the target branch
  • Auto-verification: Auto-verified users have their PRs automatically marked as verified
  • Labels: All label categories are enabled (default configuration)

📋 Available Commands

PR Status Management

  • /wip - Mark PR as work in progress (adds WIP: prefix to title)
  • /wip cancel - Remove work in progress status
  • /hold - Block PR merging (approvers only)
  • /hold cancel - Unblock PR merging
  • /verified - Mark PR as verified
  • /verified cancel - Remove verification status
  • /reprocess - Trigger complete PR workflow reprocessing (useful if webhook failed or configuration changed)
  • /regenerate-welcome - Regenerate this welcome message

Review & Approval

  • /lgtm - Approve changes (looks good to me)
  • /approve - Approve PR (approvers only)
  • /automerge - Enable automatic merging when all requirements are met (maintainers and approvers only)
  • /assign-reviewers - Assign reviewers based on OWNERS file
  • /assign-reviewer @username - Assign specific reviewer
  • /check-can-merge - Check if PR meets merge requirements

Testing & Validation

  • /retest tox - Run Python test suite with tox
  • /retest build-container - Rebuild and test container image
  • /retest all - Run all available tests

Container Operations

  • /build-and-push-container - Build and push container image (tagged with PR number)
    • Supports additional build arguments: /build-and-push-container --build-arg KEY=value

Cherry-pick Operations

  • /cherry-pick <branch> - Schedule cherry-pick to target branch when PR is merged
    • Multiple branches: /cherry-pick branch1 branch2 branch3

Label Management

  • /<label-name> - Add a label to the PR
  • /<label-name> cancel - Remove a label from the PR

✅ Merge Requirements

This PR will be automatically approved when the following conditions are met:

  1. Approval: /approve from at least one approver
  2. LGTM Count: Minimum 0 /lgtm from reviewers
  3. Status Checks: All required status checks must pass
  4. No Blockers: No WIP, hold, conflict labels
  5. Verified: PR must be marked as verified (if verification is enabled)

📊 Review Process

Approvers and Reviewers

Approvers:

  • myakove

Reviewers:

  • krcmarik
  • myakove
Available Labels
  • hold
  • verified
  • wip
  • lgtm
  • approve
  • automerge

💡 Tips

  • WIP Status: Use /wip when your PR is not ready for review
  • Verification: The verified label is automatically removed on each new commit
  • Cherry-picking: Cherry-pick labels are processed when the PR is merged
  • Container Builds: Container images are automatically tagged with the PR number
  • Permission Levels: Some commands require approver permissions
  • Auto-verified Users: Certain users have automatic verification and merge privileges

For more information, please refer to the project documentation or contact the maintainers.

Migrate all test files to use class-based test structure with the 5-step
pattern (storagemap, networkmap, plan, migrate, check_vms) and proper
test ordering via pytest-dependency markers.

Key changes:
- Add class-scoped fixtures: class_plan_config, prepared_plan,
  class_multus_network_name for shared state across test methods
- Extract create_plan_resource() and execute_migration() from run_migration()
  for better separation of concerns
- Update check_vms() to read from plan["source_vms_data"] instead of
  separate parameters
- Refactor VMware provider with get_changed_block_tracking_enabled()
  and is_warm_migration_supported() methods
- Add pytest-dependency to project dependencies
- Update CLAUDE.md with test structure documentation and remove duplications
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 11

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)
libs/providers/vmware.py (2)

102-126: Use LOGGER.exception() in exception handlers to preserve stack traces.

In the three exception handlers (lines 119, 122, 125), LOGGER.error() logs only the error message, dropping the full traceback. Use LOGGER.exception() instead—it logs at ERROR level while automatically capturing the exception traceback via sys.exc_info(). This improves debuggability without extra effort.

Since all three handlers re-raise the exception, preserving the traceback in logs provides crucial context before the exception propagates. The python-simple-logger package fully supports exception() as a standard logging wrapper.

🔧 Proposed changes
             except TimeoutExpiredError:
-                LOGGER.error(f"Timed out waiting for provider {self.ocp_resource.name} to be validated after update")
+                LOGGER.exception(f"Timed out waiting for provider {self.ocp_resource.name} to be validated after update")
                 raise
             except ApiException as e:
-                LOGGER.error(f"Kubernetes API error updating provider with esxiCloneMethod: {e.reason}")
+                LOGGER.exception(f"Kubernetes API error updating provider with esxiCloneMethod: {e.reason}")
                 raise
             except (ValueError, RuntimeError) as e:
-                LOGGER.error("Failed to update provider with esxiCloneMethod: %s", e)
+                LOGGER.exception("Failed to update provider with esxiCloneMethod: %s", e)
                 raise

128-163: Chain VmCloneError to the original TimeoutExpiredError to preserve root cause for debugging.

The codebase already uses exception chaining (raise ... from exc) in similar error-wrapping scenarios (e.g., utilities/ssh_utils.py, utilities/post_migration.py). When wrapping a caught exception into a domain-specific error, chaining preserves the original traceback and __cause__ attribute, making root-cause analysis significantly easier during debugging.

Additionally, use LOGGER.exception() instead of LOGGER.error() to automatically include the exception traceback in the log output.

Suggested change
-        except TimeoutExpiredError:
-            LOGGER.error("Timed out waiting for secret '%s' to be created.", secret_name)
-            raise VmCloneError(f"SSH public key secret '{secret_name}' not found.")
+        except TimeoutExpiredError as exc:
+            LOGGER.exception("Timed out waiting for secret '%s' to be created.", secret_name)
+            raise VmCloneError(f"SSH public key secret '{secret_name}' not found.") from exc
utilities/migration_utils.py (1)

187-259: HIGH: Missing fail‑fast validation for target scheduling fixtures.

When plan["target_node_selector"] or plan["target_labels"] is set but the corresponding fixtures are absent, the scheduling config is silently dropped. This defeats the test intent and hides config errors. Add explicit validation here.

✅ Proposed fix (add parameters + checks)
 def prepare_migration_for_tests(
     ocp_admin_client: DynamicClient,
     plan: dict[str, Any],
     request: FixtureRequest,
     source_provider: BaseProvider,
     destination_provider: OCPProvider,
     network_migration_map: NetworkMap,
     storage_migration_map: StorageMap,
     target_namespace: str,
     fixture_store: Any,
     source_vms_namespace: str,
+    labeled_worker_node: Any | None = None,
+    target_vm_labels: dict[str, str] | None = None,
     cut_over: datetime | None = None,
     pre_hook_name: str | None = None,
     pre_hook_namespace: str | None = None,
     after_hook_name: str | None = None,
     after_hook_namespace: str | None = None,
 ) -> dict[str, Any]:
@@
     if not destination_provider.ocp_resource:
         raise ValueError("destination_provider.ocp_resource is not set")
+
+    if plan.get("target_node_selector") and labeled_worker_node is None:
+        raise ValueError("target_node_selector requested but labeled_worker_node fixture is None")
+
+    if plan.get("target_labels") and target_vm_labels is None:
+        raise ValueError("target_labels requested but target_vm_labels fixture is None")

Based on learnings, this fail‑fast validation is required to avoid silent drops.

conftest.py (1)

806-865: Session teardown is already idempotent—verify scope separation instead.

The concern about double cleanup is mitigated by session_teardown's explicit if virtual_machine_obj.exists: check (line 198 in utilities/pytest_utils.py). However, there is a real architectural issue: prepared_plan (class-scoped) registers VMs for session teardown after cleanup_migrated_vms (class-scoped) has already deleted them.

Due to pytest fixture teardown order (reverse dependency), this creates inefficient duplicate registration: VMs are cleaned at class scope, then re-registered for session scope cleanup after they no longer exist.

Per the coding guidelines, use class-scoped cleanup for test-specific resources like VMs. Remove the session-level registration of migrated VMs from prepared_plan, or implement the suggested mitigation to remove cleaned VMs from the teardown list in cleanup_migrated_vms.

🤖 Fix all issues with AI agents
In `@conftest.py`:
- Around line 553-556: Replace the md5 usage and 4-char truncation with a
FIPS-safe SHA-256 prefix derived from request.node.nodeid to reduce collisions:
instead of hashlib.md5(test_class_name.encode()), use
hashlib.sha256(request.node.nodeid.encode()) and take a 6–8 character hex
prefix, then set base_name = f"cb-{hash_prefix}" (ensure the final bridge name
stays ≤15 chars). Update the same change in both occurrences where
test_class_name, short_hash, and base_name are computed.

In `@libs/providers/vmware.py`:
- Around line 873-895: Replace the f-string logger calls in the disk-datastore
selection block so they use parameterized logging instead of eager f-string
formatting: update the LOGGER.info calls that reference LOGGER,
available_unit_number, disk_datastore_id, disk_datastore, secondary_datastore,
target_datastore and the get_obj([vim.Datastore], ...) result to use a format
string with %s placeholders and pass available_unit_number, disk_datastore.name
and disk_datastore._moId as separate args (e.g. "Disk %s: Using secondary
datastore '%s' (ID: %s)", available_unit_number, disk_datastore.name,
disk_datastore._moId); make the same change for the custom and default datastore
branches and repeat the same conversion for the analogous logger calls in the
later block (around the other occurrence at 933-939), and verify that the
project's simple_logger supports %s-style parameterized formatting before
committing.

In `@tests/test_mtv_cold_migration.py`:
- Around line 55-64: Replace direct usage of get_storage_migration_map and
execute_migration with the standardized two-step helper pattern used across
tests: call create_storagemap_and_networkmap(...) to obtain (storage_map,
network_map), pass those into create_plan_resource(..., storage_map=storage_map,
network_map=network_map, ...) to build the plan, then call
migrate_vms(plan=plan_resource, ...) instead of execute_migration; update each
occurrence (including the blocks around lines 81-90, 138-143, 205-213, 230-238,
287-292) to create and register teardown/reporting via the helpers so
storage/network map creation, cleanup, and reporting follow the suite
convention.
- Around line 49-54: The test mutates the class-scoped prepared_plan fixture by
overriding prepared_plan["virtual_machines"] inside test_create_storagemap when
source_provider.type == Provider.ProviderType.OVA; move that OVA-specific
override into the prepared_plan fixture initialization so the fixture is
deterministically configured at class scope (set plan["virtual_machines"] =
[{"name": "1nisim-rhel9-efi"}] when source_provider.type ==
Provider.ProviderType.OVA within the prepared_plan fixture, e.g., using the
local plan/virtual_machines variables), and remove the conditional override from
test_create_storagemap so the test no longer mutates the fixture.

In `@tests/test_mtv_warm_migration.py`:
- Around line 119-124: Extract the duplicated loop that sets vm["id"] from
source_provider_inventory.get_vm into a single helper function (e.g.,
populate_vm_ids(plan: dict[str, Any], inventory)) and replace each inline loop
that iterates prepared_plan["virtual_machines"] and calls
source_provider_inventory.get_vm(vm["name"]) with a call to this helper;
reference the existing symbols prepared_plan, virtual_machines,
source_provider_inventory, and get_vm when locating the code, add the helper to
a shared test utilities module or fixture, and update imports/usages in the
affected test classes to call populate_vm_ids(prepared_plan,
source_provider_inventory).
- Around line 65-66: Update the docstrings for all new/modified test functions
in this file (e.g., test_create_storagemap) to follow the required
Args/Returns/Raises structure: keep the one-line summary ("Create StorageMap
resource for migration."), then add an Args section listing parameters (name:
type and brief description, e.g., prepared_plan (dict[str, Any]): Prepared plan
configuration), include a Returns section if the test returns a value (usually
none) and a Raises section listing expected exceptions (e.g., AssertionError: If
StorageMap creation fails). Apply this pattern consistently across every new
test method in tests/test_mtv_warm_migration.py.
- Around line 54-155: The test breaks the mandated two-step pattern: remove the
manual calls to get_storage_migration_map, get_network_migration_map,
create_plan_resource, and execute_migration and instead use the shared two-step
helpers create_storagemap_and_networkmap() for setup and migrate_vms() for
execution (replace
test_create_storagemap/test_create_networkmap/test_create_plan/test_migrate_vms
with calls to those helpers, ensuring storage_map/network_map/plan_resource are
set via the helper contract).
- Around line 46-47: The class decorator currently lists multus_network_name in
`@pytest.mark.usefixtures` while test functions also accept multus_network_name as
a parameter (e.g., class TestSanityWarmMtvMigration), which causes duplicate
fixture requests; remove multus_network_name from the usefixtures list on all
classes in this file and keep it only as a test parameter so the fixture is
injected via function arguments (leave other fixtures like
precopy_interval_forkliftcontroller and cleanup_migrated_vms in usefixtures if
they are not passed as parameters).

In `@utilities/mtv_migration.py`:
- Around line 246-332: The run_migration function currently creates the Plan and
Migration CR but returns immediately; replace the final
create_and_store_resource(...) + return plan behavior by delegating to the
existing execute_migration(...) helper (or call the appropriate
wait_for_migration_complete routine) so the function waits for migration
completion; specifically, after create_plan_resource(...) use
execute_migration(ocp_admin_client=ocp_admin_client,
fixture_store=fixture_store, plan=plan, target_namespace=target_namespace,
cut_over=cut_over, /* pass any other needed flags like
copyoffload/preserve_static_ips if execute_migration requires them */) and
return its result instead of returning plan so callers only proceed once the
migration finishes.
- Around line 100-118: The module currently imports
kubernetes.dynamic.DynamicClient at runtime which violates the repo guideline to
use openshift-python-wrapper and ocp_utilities.get_client(); remove the runtime
import and instead place DynamicClient inside a typing-only import guarded by
TYPE_CHECKING (e.g., add "from typing import TYPE_CHECKING" and under "if
TYPE_CHECKING: from kubernetes.dynamic import DynamicClient") so
create_plan_resource(...) can keep its annotation without importing
DynamicClient at runtime; ensure no other runtime uses of DynamicClient remain
and rely on ocp_utilities.get_client()/OCPProvider for actual client calls.

In `@utilities/post_migration.py`:
- Around line 1141-1152: The current gate only runs check_static_ip_preservation
when per-VM data exists in plan.get("source_vms_data"), which silently skips
verification if that map is missing; update the logic around plan,
source_vms_data and the call to check_static_ip_preservation (references: plan,
source_vms_data, check_static_ip_preservation, vm_name, vm_ssh_connections,
source_provider_data, source_provider.type/Provider.ProviderType.VSPHERE) to
fail fast: if the plan requests static IP preservation (e.g.,
plan.get("preserve_static_ips") or equivalent flag) and source_provider.type is
VSPHERE, assert that per-VM data for the given vm["name"] exists and
raise/return an explicit error when it does not, rather than silently skipping
the check.

Remove unused functions and imports:
- Delete report.py (all functions unused)
- Remove run_migration from utilities/mtv_migration.py
- Remove prepare_migration_for_tests from utilities/migration_utils.py
- Remove source_vms_network fixture from conftest.py
- Clean up related unused imports
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
conftest.py (1)

15-16: HIGH: Move DynamicClient import to TYPE_CHECKING block.

Same issue as in utilities/mtv_migration.py. Repo guidelines require DynamicClient only for type annotations via TYPE_CHECKING block. With from __future__ import annotations, string annotations work without runtime imports.

✅ Required fix
-from kubernetes.dynamic import DynamicClient
+from typing import TYPE_CHECKING
+
+if TYPE_CHECKING:
+    from kubernetes.dynamic import DynamicClient
🤖 Fix all issues with AI agents
In `@conftest.py`:
- Around line 828-865: The fixture cleanup_migrated_vms currently calls
VirtualMachine.clean_up() unconditionally which can raise if the VM is already
absent; update the teardown loop that iterates prepared_plan["virtual_machines"]
to check vm_obj.exists (the VirtualMachine.exists property/method) before
calling vm_obj.clean_up(), logging/skipping when the VM does not exist to match
the defensive pattern used in session teardown.
♻️ Duplicate comments (3)
utilities/mtv_migration.py (1)

6-6: HIGH: Move DynamicClient import to TYPE_CHECKING block.

This runtime import violates repo guidelines. With from __future__ import annotations already present, type annotations are strings and don't require runtime imports.

✅ Required fix
+from typing import TYPE_CHECKING
+
+if TYPE_CHECKING:
+    from kubernetes.dynamic import DynamicClient
-from kubernetes.dynamic import DynamicClient
conftest.py (2)

553-556: HIGH: Replace md5 with SHA-256 for FIPS compliance.

hashlib.md5 can raise at runtime under FIPS mode (common in RHEL/OCP). Additionally, 4-char hex truncation increases collision risk across classes in parallel runs.

✅ Recommended fix
-    test_class_name = request.node.cls.__name__ if request.node.cls else request.node.name
-    short_hash = hashlib.md5(test_class_name.encode()).hexdigest()[:4]
+    test_class_name = request.node.cls.__name__ if request.node.cls else request.node.name
+    # Use SHA-256 for FIPS compliance; 6-char prefix for lower collision risk
+    short_hash = hashlib.sha256(test_class_name.encode()).hexdigest()[:6]

Ensure the bridge name stays ≤15 chars: cb-{6-char}-{1-digit} = 11 chars max.


734-737: HIGH: Replace md5 with SHA-256 for FIPS compliance.

Same issue as in multus_network_name fixture. Use SHA-256 with 6-char prefix for FIPS compliance and reduced collision risk.

✅ Recommended fix
-            test_class_name = request.node.cls.__name__ if request.node.cls else request.node.name
-            short_hash = hashlib.md5(test_class_name.encode()).hexdigest()[:4]
+            test_class_name = request.node.cls.__name__ if request.node.cls else request.node.name
+            short_hash = hashlib.sha256(test_class_name.encode()).hexdigest()[:6]

coderabbitai[bot]
coderabbitai bot previously approved these changes Jan 20, 2026
- Move DynamicClient import to TYPE_CHECKING block (conftest.py, mtv_migration.py)
- Extract _generate_class_hash_prefix helper using SHA-256 (FIPS-compliant)
- Add defensive check in cleanup_migrated_vms (skip if VM already deleted)
- Move OVA provider VM override to prepared_plan fixture
- Remove duplicate VM/pod registration from prepared_plan teardown
- Add ParamikoSSHBannerFilter to suppress noisy SSH banner errors
- Add fail-fast validation for preserve_static_ips in check_vms
- Extract populate_vm_ids helper to utilities/utils.py
- Use LOGGER.exception for exception handlers in vmware.py
- Add docstrings to warm migration test methods
- Remove unused multus_network_name from usefixtures decorators
- Remove unused report.py from Dockerfile COPY
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🤖 Fix all issues with AI agents
In `@conftest.py`:
- Line 12: Replace the typing import for Generator with the modern
collections.abc version: change the import line that currently references
typing.Generator to import Generator from collections.abc (keep TYPE_CHECKING
and Any from typing as needed) and update any references relying on
typing.Generator to use the newly imported Generator symbol; this addresses Ruff
UP035 and aligns conftest.py with Python 3.9+ typing conventions.
- Around line 692-731: The prepared_plan fixture currently declares an unused
parameter target_namespace: remove target_namespace from the prepared_plan
function signature and from its docstring so the fixture only lists the fixtures
it directly uses; keep all other parameters (request, class_plan_config,
fixture_store, source_provider, source_vms_namespace, ocp_admin_client,
multus_cni_config, source_provider_inventory) intact and ensure no references to
target_namespace remain in the prepared_plan body, docstring, or type hints so
dependency is relied on transitively via source_provider.

In `@tests/test_mtv_cold_migration.py`:
- Line 28: The test decorator currently double-invokes the multus_network_name
fixture; remove "multus_network_name" from the `@pytest.mark.usefixtures` list in
tests/test_mtv_cold_migration.py so only non-parameter fixtures like
"cleanup_migrated_vms" remain, leaving the test function test_create_networkmap
to receive multus_network_name via its parameter signature (ensure the test
still accepts the multus_network_name argument).
- Around line 250-254: The inline loop that populates VM IDs (iterating
prepared_plan["virtual_machines"], calling source_provider_inventory.get_vm and
assigning vm["id"]) is duplicated; extract this logic into a shared helper
(e.g., populate_vm_ids_from_inventory or similar) and replace the inline loop in
TestColdRemoteOcp.test_create_plan with a call to that helper so prepared_plan,
source_provider_inventory, get_vm and virtual_machines usage is centralized and
avoids duplication.
- Around line 101-105: Replace the inline loop that assigns VM IDs using
source_provider_inventory.get_vm with the shared helper populate_vm_ids to avoid
duplication: remove the for-loop referencing prepared_plan["virtual_machines"],
vm["name"], and vm["id"] and call populate_vm_ids(prepared_plan,
source_provider_inventory) (same usage as in test_mtv_warm_migration.py); ensure
the helper returns or mutates prepared_plan consistently so subsequent
assertions still see the populated IDs.
- Line 14: Import the existing helper populate_vm_ids from utilities.utils
(alongside get_value_from_py_config) and replace the two inline loops that
construct VM ID lists in tests/test_mtv_cold_migration.py with calls to
populate_vm_ids; specifically, locate the places where vm_ids (or similar lists)
are built via for-loops and swap them for populate_vm_ids(...) so the cold
migration test uses the same shared helper as the warm migration file.
- Line 177: The TestColdRemoteOcp class is using `@pytest.mark.usefixtures` in the
same problematic way; replace that decorator with a class-level marker variable
so pytest applies the fixtures correctly: remove the
`@pytest.mark.usefixtures`("multus_network_name", "cleanup_migrated_vms")
decorator and add pytestmark = pytest.mark.usefixtures("multus_network_name",
"cleanup_migrated_vms") inside the TestColdRemoteOcp class (referencing the
class name TestColdRemoteOcp and the fixture names multus_network_name and
cleanup_migrated_vms) so fixtures are applied to all tests in the class
consistently.
♻️ Duplicate comments (1)
utilities/post_migration.py (1)

1141-1161: LOW: Fail‑fast guard prevents silent skips of static‑IP checks.
Requiring source_vms_data when preserve_static_ips is enabled ensures the verification doesn’t silently pass without required inputs.

coderabbitai[bot]
coderabbitai bot previously approved these changes Jan 20, 2026
Address CodeRabbit review comments:
- Move _generate_class_hash_prefix from conftest.py to utilities/utils.py
  as generate_class_hash_prefix with configurable length parameter
- Change yield to return for multus_network_name fixture (no teardown)
- Update fixtures to use the new imported function
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In `@conftest.py`:
- Line 937: Change the three copy-offload fixtures to session scope: update the
pytest fixture decorators for copyoffload_config, copyoffload_storage_secret,
and setup_copyoffload_ssh so they use scope="session" instead of scope="class";
this ensures the read-only copyoffload_config, the Kubernetes Secret created in
copyoffload_storage_secret, and the idempotent ESXi setup in
setup_copyoffload_ssh are created once per test session (matching session-scoped
source_provider_data and target_namespace) to avoid repeated expensive
operations and duplicate resources.

In `@utilities/utils.py`:
- Around line 555-568: The function populate_vm_ids currently assumes
plan["virtual_machines"] exists and is iterable; add defensive validation at the
start of populate_vm_ids to ensure plan is a dict and contains a
"virtual_machines" key whose value is a list (raise a ValueError with a clear
message like "plan must contain 'virtual_machines' list" on failure), then
proceed to iterate and call inventory.get_vm(vm_name) as before; also update the
populate_vm_ids docstring to note that a ValueError may be raised if the plan is
malformed or if inventory.get_vm raises ValueError when a VM is not found
(preserve the existing fail-fast behavior).

coderabbitai[bot]
coderabbitai bot previously approved these changes Jan 20, 2026
Change copyoffload_config, copyoffload_storage_secret, and
setup_copyoffload_ssh fixtures from class to session scope for
better resource reuse across test classes.

Add defensive validation in populate_vm_ids to ensure plan
structure is valid before processing virtual machines.
@myakove
Copy link
Collaborator Author

myakove commented Jan 20, 2026

/verified
/approve

@prabinovRedhat
Copy link
Collaborator

/approve

from copyoffload side

@myakove myakove merged commit 07b5bc9 into main Jan 20, 2026
4 of 5 checks passed
@myakove myakove deleted the feat/class-based-test-structure branch January 20, 2026 19:33
@redhat-qe-bot2
Copy link

Failed to build and push ghcr.io/redhatqe/mtv-api-tests:latest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants