Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
from utilities.bitwarden import get_cnv_tests_secret_by_name
from utilities.constants import (
AMD_64,
MULTIARCH,
Comment thread
coderabbitai[bot] marked this conversation as resolved.
QUARANTINED,
SETUP_ERROR,
TIMEOUT_5MIN,
Expand Down Expand Up @@ -558,6 +559,15 @@ def filter_sno_only_tests(items: list[Item], config: Config) -> list[Item]:
return items


def filter_multiarch_tests(items: list[Item], config: Config) -> list[Item]:
if py_config.get("cluster_type") == MULTIARCH:
return items
discard_tests, items_to_return = remove_tests_from_list(items=items, filter_str="multiarch")
if discard_tests:
config.hook.pytest_deselected(items=discard_tests)
return items_to_return
Comment on lines +562 to +568

@coderabbitai coderabbitai Bot May 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

HIGH: Keep non-fixture helpers out of conftest.py.

Line 562 adds filter_multiarch_tests(...) as utility logic inside conftest.py. Please move it to a shared module (for example utilities/pytest_utils.py) and import it here; this keeps fixture wiring separate from reusable behavior and prevents further growth of monolithic conftest files.

♻️ Suggested refactor
# conftest.py
 from utilities.pytest_utils import (
     ...
+    filter_multiarch_tests,
     ...
 )

- def filter_multiarch_tests(items: list[Item], config: Config) -> list[Item]:
-     if py_config.get("cluster_type") == MULTIARCH:
-         return items
-     discard_tests, items_to_return = remove_tests_from_list(items=items, filter_str="multiarch")
-     if discard_tests:
-         config.hook.pytest_deselected(items=discard_tests)
-     return items_to_return
# utilities/pytest_utils.py
from _pytest.config import Config
from pytest import Item
from pytest_testconfig import config as py_config

from utilities.constants import MULTIARCH

def filter_multiarch_tests(items: list[Item], config: Config) -> list[Item]:
    if py_config.get("cluster_type") == MULTIARCH:
        return items
    discard_tests = [item for item in items if "multiarch" in item.keywords]
    kept_items = [item for item in items if "multiarch" not in item.keywords]
    if discard_tests:
        config.hook.pytest_deselected(items=discard_tests)
    return kept_items

As per coding guidelines: "**/conftest.py: conftest.py is for fixtures only. Helper functions, utility functions, and classes must NOT be defined in conftest.py or test_*.py."

🤖 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 `@conftest.py` around lines 562 - 568, The helper function
filter_multiarch_tests should be moved out of conftest.py into a shared
utilities module (e.g., utilities/pytest_utils.py) and then imported from
conftest.py; create utilities/pytest_utils.py, add the existing
filter_multiarch_tests implementation (ensure it imports py_config, MULTIARCH,
and pytest Config/Item types) and replace the inline definition in conftest.py
with from utilities.pytest_utils import filter_multiarch_tests; ensure
references to remove_tests_from_list remain valid (move it too if it’s also
defined in conftest.py) and keep the behavior of calling
config.hook.pytest_deselected for discarded tests.

✅ Confirmed as addressed by @hmeir

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

all other similar functions are there

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@hmeir, you're right — filter_upgrade_tests, filter_deprecated_api_tests, filter_sno_only_tests, and remove_tests_from_list are all defined directly in conftest.py following this same pattern. filter_multiarch_tests is consistent with the established convention. I'll withdraw my suggestion.


✏️ Learnings added
Learnt from: hmeir
Repo: RedHatQE/openshift-virtualization-tests PR: 4972
File: conftest.py:562-568
Timestamp: 2026-05-25T11:26:45.804Z
Learning: In the RedHatQE/openshift-virtualization-tests repository, collection-time filter helper functions (e.g., `filter_upgrade_tests`, `filter_deprecated_api_tests`, `filter_sno_only_tests`, `filter_multiarch_tests`, `remove_tests_from_list`) are intentionally defined directly in the top-level `conftest.py`, not moved to `utilities/pytest_utils.py`. Do not flag or suggest moving these pytest-collection filter helpers out of `conftest.py`; this is the established pattern for this repo.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: servolkov
Repo: RedHatQE/openshift-virtualization-tests PR: 3387
File: tests/network/provider_migration/libprovider.py:50-52
Timestamp: 2026-01-12T11:24:13.825Z
Learning: In the RedHatQE/openshift-virtualization-tests repository, when catching exceptions in Python, use LOGGER.error before re-raising and do not replace it with LOGGER.exception in except blocks. This follows the established pattern across the codebase.

Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 3366
File: tests/storage/cdi_clone/test_clone.py:5-9
Timestamp: 2026-01-12T14:25:05.723Z
Learning: In Python tests and utility code across the repository, bitmath.parse_string_unsafe correctly parses Kubernetes quantities (e.g., '4Gi', '512Mi', PVC storage requests) without supplying system=bitmath.NIST. There are 30+ usages indicating this is the standard behavior. Reviewers should verify that code that builds or compares quantity strings does not pass the NIST parameter, and if a new test relies on quantity parsing, assume no NIST parameter is required unless explicitly documented.

Learnt from: servolkov
Repo: RedHatQE/openshift-virtualization-tests PR: 3387
File: tests/network/provider_migration/libprovider.py:1-8
Timestamp: 2026-01-20T01:03:13.139Z
Learning: In the openshift-virtualization-tests repository, Python imports should consistently use module-level imports for the logging module (i.e., import logging) rather than from logging import ... The established pattern spans 270+ files and should not be flagged for refactoring. Apply this guideline to Python files across the repo (e.g., tests/network/provider_migration/libprovider.py).

Learnt from: geetikakay
Repo: RedHatQE/openshift-virtualization-tests PR: 3559
File: utilities/infra.py:251-254
Timestamp: 2026-01-21T21:26:41.805Z
Learning: In the RedHatQE/openshift-virtualization-tests repository, when reviewing Python code, recognize that with Python 3.14 the syntax 'except ValueError, TypeError:' is valid if there is no 'as' clause, and should not be flagged as Python 2 syntax. If you use an 'as' binding (e.g., 'except (ValueError, TypeError) as e:'), parentheses are required. Ensure this pattern is version-consistent and not flagged as Python 2 syntax when 'as' is absent.

Learnt from: jpeimer
Repo: RedHatQE/openshift-virtualization-tests PR: 3571
File: tests/storage/storage_migration/utils.py:158-167
Timestamp: 2026-01-25T13:18:21.675Z
Learning: In reviews of the openshift-virtualization-tests repo (and similar Python code), avoid suggesting minor stylistic changes that require extra verification (e.g., removing dict.keys() checks for membership) unless the change has clear correctness or maintainability impact. Focus on fixes with observable behavior, security, performance, or maintainability benefits; defer low-impact style tweaks that are costly to verify.

Learnt from: OhadRevah
Repo: RedHatQE/openshift-virtualization-tests PR: 2348
File: tests/observability/upgrade/conftest.py:1-7
Timestamp: 2026-01-26T10:11:23.629Z
Learning: In conftest.py files that define session-scoped fixtures which provision resources (e.g., VMs, namespaces) for upgrade testing, logging imports and structured INFO logs for setup are optional. Prefer quieter fixture setup to reduce log noise unless debugging is required. Apply this guideline across similar conftest.py fixtures that perform straightforward resource provisioning.

Learnt from: EdDev
Repo: RedHatQE/openshift-virtualization-tests PR: 3649
File: tests/network/user_defined_network/ip_specification/conftest.py:32-48
Timestamp: 2026-01-29T15:01:46.322Z
Learning: In test fixtures (e.g., tests/network/user_defined_network/ip_specification/conftest.py and other VM-related conftest.py files), after vm.start(), prefer calling vm.wait_for_agent_connected() as the validation step. Do not add vm.wait_for_ready_status(status=True) beforehand, since wait_for_agent_connected() ensures the guest OS is actually running (not just powered on) and provides a stronger, more reliable readiness check for test initialization.

Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 3847
File: utilities/virt.py:2449-2453
Timestamp: 2026-02-18T06:35:39.536Z
Learning: In Python code, a function named clearly and self-descriptively can be deemed not to require a docstring. However, treat this as a context-specific guideline and not a universal rule. For public APIs or functions with side effects, prefer concise docstrings explaining behavior, inputs, outputs, and side effects. This guidance is based on the example in utilities/virt.py from RedHatQE/openshift-virtualization-tests where validate_libvirt_persistent_domain(vm, admin_client) was considered self-documenting.

Learnt from: vsibirsk
Repo: RedHatQE/openshift-virtualization-tests PR: 3883
File: utilities/pytest_utils.py:441-463
Timestamp: 2026-02-23T16:33:22.070Z
Learning: In Python code reviews, the guideline to always use named arguments for multi-argument calls does not apply to built-ins or methods that have positional-only parameters (those defined with a / in their signature). Do not flag or require named arguments for calls like dict.get(key, default=None, /), list.pop(), str.split(sep, maxsplit) and similar built-ins that cannot accept keyword arguments. Apply the named-argument rule only to functions/methods that explicitly accept keyword arguments.

Learnt from: jpeimer
Repo: RedHatQE/openshift-virtualization-tests PR: 4053
File: tests/storage/cross_cluster_live_migration/conftest.py:47-55
Timestamp: 2026-03-03T12:01:13.888Z
Learning: In test configuration files (for example tests/.../conftest.py), log the absence of optional CLI arguments or configurations at INFO level. Use WARNING only when missing configuration could cause a problem or be unusual. For parameters that may be present or absent by design, prefer INFO to reflect normality.

Learnt from: dshchedr
Repo: RedHatQE/openshift-virtualization-tests PR: 4118
File: utilities/database.py:0-0
Timestamp: 2026-03-17T01:32:02.617Z
Learning: In RedHatQE/openshift-virtualization-tests, when reviewing Python files, post targeted inline comments on the Files changed tab at the exact location (file and line) of the issue rather than opening a single discussion thread for multiple issues. This should be done for each applicable location to improve traceability and clarity. If multiple issues exist in the same file, address them with separate inline comments pointing to the specific lines.

Learnt from: dshchedr
Repo: RedHatQE/openshift-virtualization-tests PR: 4118
File: utilities/database.py:0-0
Timestamp: 2026-03-17T01:32:02.617Z
Learning: In the RedHatQE/openshift-virtualization-tests repository, CodeRabbit should post targeted inline comments at each applicable location in the Files Changed tab, rather than aggregating multiple issues into a single PR discussion thread reply. This guideline applies to all Python files (any file ending in .py) changed in a PR; for non-Python files, follow the same inline-comment-at-location principle if relevant.

Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 4725
File: utilities/console.py:54-59
Timestamp: 2026-05-04T13:45:29.122Z
Learning: During review of RedHatQE/openshift-virtualization-tests “lint-cleanup” PRs (e.g., changes targeting lint issues like stale noqa/utf-8 headers), do not flag existing `# type: ignore` directives that were already present before the PR and were not introduced or modified by the PR. Only raise findings for `# type: ignore` suppressions that the PR itself adds, changes, or otherwise makes newly effective (i.e., they appear in the diff as additions/edits).

Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 4725
File: tests/virt/cluster/common_templates/centos/test_centos_os_support.py:78-83
Timestamp: 2026-05-04T13:45:33.892Z
Learning: When reviewing lint-cleanup or formatting-only pull requests in this repo (e.g., changes like removing/updating `# noqa` comments or UTF-8 headers), do not raise findings for code patterns that already existed before the PR. Specifically, if a problematic construct such as `.is_connective(tcp_timeout=120)` was present in the base branch, suppress that finding and only raise issues when the PR itself introduces or modifies that construct (i.e., the diff adds/changes the call or its arguments). Apply this rule across all Python files (`**/*.py`).

Learnt from: dshchedr
Repo: RedHatQE/openshift-virtualization-tests PR: 4739
File: tests/virt/node/descheduler/conftest.py:2-2
Timestamp: 2026-05-05T17:01:15.294Z
Learning: In this repo’s Python code, it’s acceptable (and preferred by convention) to build `run_command` inputs using `shlex.split(f"<command> {arg}")` rather than converting to direct list literals like `['oc', 'adm', 'uncordon', name]`. During code review, generally don’t flag `shlex.split(...)` usage for `run_command` calls and don’t suggest replacing it with list literals; the string-form pattern is used to keep commands readable and consistent with how they’re typed in a terminal.

Learnt from: geetikakay
Repo: RedHatQE/openshift-virtualization-tests PR: 4788
File: utilities/os_utils.py:257-262
Timestamp: 2026-05-08T12:49:20.694Z
Learning: In RedHatQE/openshift-virtualization-tests, the Ruff flake8-boolean-trap rules FBT001/FBT002 are intentionally not enabled (pyproject.toml does not select the FBT rules; confirmed via `ruff check --show-settings`). Therefore, do not flag boolean positional parameters as FBT001/FBT002 violations in this repository. If Ruff configuration changes and starts selecting FBT rules, this exception should be reconsidered.

Learnt from: acinko-rh
Repo: RedHatQE/openshift-virtualization-tests PR: 4780
File: tests/storage/utils.py:568-572
Timestamp: 2026-05-12T05:10:24.601Z
Learning: In this repository, Ruff rule UP043 ("unnecessary default type arguments") is enforced. When annotating `collections.abc.Generator` return types, prefer the single-parameter form `Generator[YieldType]` rather than `Generator[YieldType, None, None]`. Explicit `None, None` for the SendType and ReturnType are unnecessary defaults (per PEP 696) and will trigger UP043. Apply this consistently across all Python files.

Learnt from: Anatw
Repo: RedHatQE/openshift-virtualization-tests PR: 4833
File: tests/network/localnet/migration_stuntime/libstuntime.py:25-25
Timestamp: 2026-05-13T19:23:09.603Z
Learning: In this repository, do not recommend adding `from __future__ import annotations` to fix forward-reference type annotation issues (e.g., Ruff UP037). Follow the established convention: use quoted string type annotations for forward references when the referenced class/type is defined later in the same file (e.g., `"ContinuousPing"`), and prefer `typing.Self` for self-referential return types.

Learnt from: EdDev
Repo: RedHatQE/openshift-virtualization-tests PR: 4819
File: utilities/unittests/test_bitwarden.py:207-207
Timestamp: 2026-05-18T06:30:56.781Z
Learning: During Ruff/lint rule-enablement PRs in this repository (e.g., when introducing a new rule like PLC0415), it’s acceptable to keep CI green by adding per-line, targeted suppressions for pre-existing violations: add only `# noqa: <single-ruff-rule-id>` at the end of the specific violating line. In this PR context, reviewers should NOT flag these targeted `# noqa: PLC0415` comments as policy violations, assuming the suppression is for a pre-existing issue and is documented in the PR description as a candidate for follow-up cleanup. Do not allow blanket `# noqa` (without a specific rule) or `per-file-ignores`; those remain disallowed.

Learnt from: EdDev
Repo: RedHatQE/openshift-virtualization-tests PR: 4819
File: utilities/unittests/test_pytest_utils.py:270-270
Timestamp: 2026-05-18T06:31:12.015Z
Learning: In RedHatQE/openshift-virtualization-tests, if a PR is a Ruff rule-enforcement PR and its “Special notes for reviewer” documents that pre-existing Ruff violations are being temporarily handled via per-line suppressions (e.g., `# noqa: PLC0415`) to keep CI green, reviewers should treat those specific `# noqa: <rule>` comments as an agreed, temporary mechanism. Do not flag them as code-quality issues and do not recommend removing, consolidating, or refactoring those suppressions within the same PR; cleanup/remediation is expected to happen in dedicated follow-up PRs instead.

Learnt from: EdDev
Repo: RedHatQE/openshift-virtualization-tests PR: 4819
File: utilities/unittests/test_data_collector.py:304-304
Timestamp: 2026-05-18T06:31:15.083Z
Learning: When reviewing Python code in this repository for Ruff/linter rule rollouts, do not treat temporary suppression comments as violations in the specific migration scenario where a PR enables a new Ruff rule (e.g., PLC0415) and the PR description explicitly documents that all *pre-existing* violations are being annotated with `# noqa: <RULE>` as a short-lived measure. In that case, only flag `# noqa: <RULE>` suppressions that are newly introduced on code that did not previously violate the rule—i.e., verify via the PR diff against the prior state (and/or prior Ruff findings) that the suppressed line was already violating before the rule was enabled. Ignore suppressions that are covering violations that existed before the new rule rollout and were intentionally bulk-added for cleanup in follow-up PRs.

Learnt from: EdDev
Repo: RedHatQE/openshift-virtualization-tests PR: 4819
File: utilities/unittests/test_hco.py:501-501
Timestamp: 2026-05-18T06:31:20.848Z
Learning: When reviewing Python code in RedHatQE/openshift-virtualization-tests, avoid flagging Ruff `# noqa: <RULE>` suppressions as issues if they were intentionally added as a temporary measure to keep CI green after a PR enables a new Ruff/lint rule (e.g., PLC0415) and the PR description documents this under "Special notes for reviewer". Treat these suppressions as deferred technical debt. Only flag `# noqa: PLC0415` (and similar rule-specific suppressions) when they are newly introduced without an accompanying documented intent in the PR (and thus appear to be masking a new violation rather than a pre-existing one).

Learnt from: EdDev
Repo: RedHatQE/openshift-virtualization-tests PR: 4878
File: utilities/unittests/test_pytest_utils.py:2194-2197
Timestamp: 2026-05-18T09:09:09.479Z
Learning: In this repository (RedHatQE/openshift-virtualization-tests), do not flag missing return type annotations or missing argument type annotations as Ruff “ANN” rule violations (e.g., ANN001/ANN002/ANN201/ANN202). The repo’s Ruff configuration does not enable ANN rules and only uses `extend-select = ["PLC0415"]`, so missing type annotations should not be treated as ANN lint failures during code review.

Learnt from: azhivovk
Repo: RedHatQE/openshift-virtualization-tests PR: 4784
File: libs/vm/affinity.py:104-104
Timestamp: 2026-05-19T07:48:17.119Z
Learning: When using Kubernetes API models like `NodeSelectorRequirement` or `LabelSelectorRequirement` with operators `Exists` or `DoesNotExist`, the `values` field must not be non-empty. It is valid for `values` to be omitted / left as `None` (Python) / passed as `null`—Kubernetes rejects non-empty `values` for these operators, but does not require the field to be present or explicitly set to an empty list. In code reviews, do not treat missing `values=[]` for `Exists`/`DoesNotExist` as a validation issue; only flag cases where `values` is provided with actual elements.

Learnt from: azhivovk
Repo: RedHatQE/openshift-virtualization-tests PR: 4784
File: libs/vm/affinity.py:104-104
Timestamp: 2026-05-19T07:48:17.119Z
Learning: When constructing Kubernetes `NodeSelectorRequirement` (or `LabelSelectorRequirement`) objects in code, do not treat `values` being omitted, `None`, or an empty list as an API-validation problem when the requirement’s operator is `Exists` or `DoesNotExist`. Per the Kubernetes API spec, these operators only require that the `values` array is not non-empty (i.e., it must be empty); they do not require the field to be explicitly present as `[]`. Therefore, reviewers should not flag `values=None`/missing `values` for `Exists`/`DoesNotExist`.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@hmeir please move, as commented by coderabbit, to pytest_utils.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

not addressed



def remove_tests_from_list(items: list[Item], filter_str: str) -> tuple[list[Item], list[Item]]:
discard_tests: list[Item] = []
items_to_return: list[Item] = []
Expand Down Expand Up @@ -635,6 +645,7 @@ def pytest_collection_modifyitems(session, config, items):
config.hook.pytest_deselected(items=discard)
items[:] = filter_deprecated_api_tests(items=items, config=config)
items[:] = filter_sno_only_tests(items=items, config=config)
items[:] = filter_multiarch_tests(items=items, config=config)
items[:] = mark_nmstate_dependent_tests(items=items)


Expand Down
8 changes: 5 additions & 3 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@
from libs.net.ip import filter_link_local_addresses, random_ipv4_address, random_ipv6_address
from libs.net.vmspec import lookup_iface_status
from tests.utils import download_and_extract_tar
from utilities.architecture import get_cluster_architecture
from utilities.artifactory import get_artifactory_header, get_http_image_url, get_test_artifact_server_url
from utilities.bitwarden import get_cnv_tests_secret_by_name
from utilities.cluster import cache_admin_client, get_oc_whoami_username
Expand Down Expand Up @@ -446,6 +447,7 @@ def schedulable_nodes(nodes):
"""
schedulable_label = "kubevirt.io/schedulable"
cpu_arch = py_config.get("cpu_arch")
cpu_archs = [cpu_arch] if isinstance(cpu_arch, str) else cpu_arch
Comment thread
hmeir marked this conversation as resolved.
schedulable = [
node
for node in nodes
Expand All @@ -454,7 +456,7 @@ def schedulable_nodes(nodes):
and not node.instance.spec.unschedulable
and not kubernetes_taint_exists(node)
and node.kubelet_ready
and (not cpu_arch or node.labels.get(KUBERNETES_ARCH_LABEL) == cpu_arch)
and (not cpu_archs or node.labels.get(KUBERNETES_ARCH_LABEL) in cpu_archs)
]

LOGGER.info(f"Schedulable nodes: {[node.name for node in schedulable]}, node architecture: {cpu_arch or 'all'}")
Expand Down Expand Up @@ -1026,7 +1028,7 @@ def mac_pool(admin_client, hco_namespace):

@pytest.fixture(scope="session")
def nodes_cpu_architecture():
return py_config["cpu_arch"]
return py_config.get("cpu_arch")


@pytest.fixture(scope="session")
Expand Down Expand Up @@ -1478,7 +1480,7 @@ def cluster_info(
f"\tOCS version: {ocs_current_version}\n"
f"\tCNI type: {get_cluster_cni_type(admin_client=admin_client)}\n"
f"\tWorkers type: {workers_type}\n"
f"\tCluster CPU Architecture: {nodes_cpu_architecture}\n"
f"\tCluster CPU Architecture: {nodes_cpu_architecture or ', '.join(sorted(get_cluster_architecture()))}\n"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

why not use py_config["cluster_arch"]?

f"\tIPv4 cluster: {ipv4_supported_cluster()}\n"
f"\tIPv6 cluster: {ipv6_supported_cluster()}\n"
f"\tVirtctl version: \n\t{virtctl_client_version}\n\t{virtctl_server_version}\n"
Expand Down