Skip to content

[IUO] Fix runbook URL test failures #4741

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
OhadRevah wants to merge 1 commit into
base: main
Choose a base branch
from
+23 −4
Open

[IUO] Fix runbook URL test failures #4741

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 7 additions & 0 deletions tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1337,6 +1337,13 @@ def hpp_cr_installed(hostpath_provisioner_scope_session):
return hostpath_provisioner_scope_session.exists


@pytest.fixture(scope="session")
def aaq_enabled(hyperconverged_resource_scope_session):
if not hyperconverged_resource_scope_session:
Copy link
Copy Markdown
Contributor

@hmeir hmeir May 7, 2026

Choose a reason for hiding this comment

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

Hm I dont think we need this check, HCO must exist for functional cluster ^^'

return False
return hyperconverged_resource_scope_session.instance.spec.get("enableApplicationAwareQuota", False)
Comment thread
OhadRevah marked this conversation as resolved.


@pytest.fixture(scope="module")
def cnv_pods(admin_client, hco_namespace):
yield list(Pod.get(client=admin_client, namespace=hco_namespace.name))
Expand Down
13 changes: 11 additions & 2 deletions tests/observability/runbook_url/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,17 @@ def available_runbook_urls():
url=runbooks_api_url,
timeout=TIMEOUT_10SEC,
):
if sample.status_code == requests.codes.ok:
return {entry["html_url"] for entry in sample.json()}
if sample:
try:
runbook_urls = {entry["html_url"] for entry in sample.json()}
except ValueError:
LOGGER.error(
f"Failed to decode JSON from '{runbooks_api_url}', "
f"status: {sample.status_code}, content-type: {sample.headers.get('Content-Type')}"
)
continue
if runbook_urls:
return runbook_urls
Comment on lines +60 to +70
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 5, 2026 •
edited
Loading

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: Validate JSON payload shape before extracting html_url.

At Line 62, valid-but-unexpected JSON (e.g., dict payload or entries without html_url) can raise TypeError/KeyError, escape the sampler loop, and bypass retry behavior.

Proposed fix 
             if sample:
                 try:
-                    runbook_urls = {entry["html_url"] for entry in sample.json()}
+                    payload = sample.json()
                 except ValueError:
                     LOGGER.error(
                         f"Failed to decode JSON from '{runbooks_api_url}', "
                         f"status: {sample.status_code}, content-type: {sample.headers.get('Content-Type')}"
                     )
                     continue
+                if not isinstance(payload, list):
+                    LOGGER.error(
+                        f"Unexpected JSON payload type from '{runbooks_api_url}': "
+                        f"{type(payload).__name__}, status: {sample.status_code}"
+                    )
+                    continue
+                runbook_urls = {
+                    entry.get("html_url")
+                    for entry in payload
+                    if isinstance(entry, dict) and entry.get("html_url")
+                }
                 if runbook_urls:
                     return runbook_urls
🧰 Tools
🪛 Ruff (0.15.12)

[warning] 64-67: Use logging.exception instead of logging.error

Replace with exception

(TRY400)

[warning] 65-66: Logging statement uses f-string

(G004)

🤖 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 `@tests/observability/runbook_url/conftest.py` around lines 60 - 70, The code
builds runbook_urls using a set comprehension from sample.json() which can raise
TypeError/KeyError when the JSON is not the expected list-of-dicts or when
entries lack "html_url"; modify the block around sample.json() in conftest.py so
you first parse the payload into a variable (resp = sample.json()), validate
that resp is an iterable/list, then iterate entries and only add
entry["html_url"] when entry is a dict and "html_url" in entry; catch and handle
TypeError/KeyError (and ValueError already handled) by logging the failure with
runbooks_api_url, sample.status_code and content-type via LOGGER and continue
the sampler loop so retries are preserved (referencing sample, runbook_urls,
runbooks_api_url, LOGGER).

Copy link
Copy Markdown
Contributor

@hmeir hmeir May 7, 2026

Choose a reason for hiding this comment

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

+1

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 7, 2026

Choose a reason for hiding this comment

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

@hmeir, thanks for the upvote!

To summarize why this matters for @OhadRevah: the GitHub Contents API can return a dict (e.g., when the path resolves to a file rather than a directory) or list entries that lack html_url. Without the shape validation, a TypeError/KeyError would propagate outside the try/except ValueError block, escape the TimeoutSampler loop entirely, and crash the fixture — bypassing all retry behavior.

Here's the proposed fix for reference:

if sample:
    try:
        payload = sample.json()
    except ValueError:
        LOGGER.error(
            f"Failed to decode JSON from '{runbooks_api_url}', "
            f"status: {sample.status_code}, content-type: {sample.headers.get('Content-Type')}"
        )
        continue
    if not isinstance(payload, list):
        LOGGER.error(
            f"Unexpected JSON payload type from '{runbooks_api_url}': "
            f"{type(payload).__name__}, status: {sample.status_code}"
        )
        continue
    runbook_urls = {
        entry.get("html_url")
        for entry in payload
        if isinstance(entry, dict) and entry.get("html_url")
    }
    if runbook_urls:
        return runbook_urls

Why each guard exists:

  • isinstance(payload, list) — catches GitHub API returning a dict (e.g., rate-limit error body, redirect to a file object)
  • isinstance(entry, dict) and entry.get("html_url") — prevents KeyError/TypeError on malformed entries; using .get() is safer than []
  • Both failures continue the sampler loop → retries are preserved (HIGH severity: without this, a bad response crashes the fixture)
🧠 Learnings used 
Learnt from: azhivovk
Repo: RedHatQE/openshift-virtualization-tests PR: 3024
File: tests/network/connectivity/utils.py:17-17
Timestamp: 2025-12-15T12:33:06.686Z
Learning: In the test suite, ensure the ipv6_network_data fixture returns a factory function (Callable) and that all call sites invoke it to obtain the actual data dict, i.e., use ipv6_network_data() at call sites. This enables future extensibility for configuring secondary interfaces' IP addresses without changing call sites. Apply this pattern to all Python test files under tests.

Learnt from: yossisegev
Repo: RedHatQE/openshift-virtualization-tests PR: 3196
File: tests/network/upgrade/test_upgrade_network.py:4-4
Timestamp: 2025-12-22T16:27:40.244Z
Learning: For PRs that remove tests, rely on pytest --collect-only to verify the test discovery results (which tests are selected/deselected) and ensure the removal is clean and the test module remains functional. Full test execution is not required for test deletion PRs. This guideline applies to test files anywhere under the tests/ directory (e.g., tests/network/upgrade/test_upgrade_network.py) and should be used for similar test-deletion scenarios across the repository.

Learnt from: yossisegev
Repo: RedHatQE/openshift-virtualization-tests PR: 3358
File: tests/network/sriov/test_sriov.py:21-21
Timestamp: 2026-01-07T09:52:12.342Z
Learning: When a PR only removes or modifies pytest markers in tests (e.g., removing pytest.mark.post_upgrade) and the test logic remains unchanged, prefer verifying with pytest --collect-only instead of running the full test suite. This validates that marker usage and test selection behavior are preserved. If the test logic changes, or markers affect behavior beyond collection, run the full test suite to confirm.

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: dshchedr
Repo: RedHatQE/openshift-virtualization-tests PR: 3404
File: tests/virt/upgrade/conftest.py:291-301
Timestamp: 2026-01-14T04:08:23.032Z
Learning: In the openshift-virtualization-tests repository, when using VirtualMachine objects from ocp-resources in tests, if vm.ready is True, vm.vmi is guaranteed to exist. Therefore, you can access vm.vmi.instance.status or vm.vmi attributes without additional defensive checks (e.g., if vm.vmi: ...). Do not rely on vm.vmi being present when vm.ready may be False; guard those code paths accordingly. This guideline applies to tests under tests/ (notably in virt/upgrade/conftest.py and related test modules) and should be followed for any code paths that assume vm.vmi exists only when vm.ready is True.

Learnt from: Anatw
Repo: RedHatQE/openshift-virtualization-tests PR: 3376
File: tests/network/general/test_ip_family_services.py:96-96
Timestamp: 2026-01-18T09:44:17.044Z
Learning: In the openshift-virtualization-tests repository, function-scoped fixtures must use pytest.fixture() with empty parentheses (not pytest.fixture without parentheses). This repo follows this convention despite Ruff PT001. Apply this consistently to all Python test files under tests/ (not just this one) to maintain repository-wide consistency.

Learnt from: EdDev
Repo: RedHatQE/openshift-virtualization-tests PR: 3273
File: tests/network/connectivity/test_ovs_linux_bridge.py:5-9
Timestamp: 2026-01-18T13:18:48.808Z
Learning: In tests/network/connectivity/test_ovs_linux_bridge.py and similar test files, prefer importing ipaddress as a module and using qualified calls like ipaddress.ip_interface(...) rather than from ipaddress import ip_interface. This preserves module context for readability, especially when chaining properties (e.g., ipaddress.ip_interface(...).ip). This is an intentional exception to the general rule favoring specific imports, and should apply to test files under the tests directory where module context aids understanding.

Learnt from: yossisegev
Repo: RedHatQE/openshift-virtualization-tests PR: 3495
File: tests/network/third_part_ip_request/test_third_party_ip_request.py:4-12
Timestamp: 2026-01-18T14:51:50.846Z
Learning: In the openshift-virtualization-tests repository, tests consistently import pytest as a module (import pytest) and avoid from pytest import ...; this is the established pattern across 398+ test files. Do not flag or refactor imports to use specific pytest names in tests under tests/**. If a file already follows this pattern, leave it as is; this guideline applies broadly to Python test files under the tests directory.

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: azhivovk
Repo: RedHatQE/openshift-virtualization-tests PR: 3619
File: tests/network/user_defined_network/test_user_defined_network.py:97-97
Timestamp: 2026-01-27T17:18:49.973Z
Learning: In tests that exercise lookup_iface_status_ip from libs.net.vmspec, rely on the function's built-in descriptive error messages for failures. Do not add extra assertion messages for IP presence checks using this function; instead, assert on the function behavior or catch its exceptions as appropriate. This reduces duplication and clarifies failures.

Learnt from: EdDev
Repo: RedHatQE/openshift-virtualization-tests PR: 3649
File: tests/network/user_defined_network/ip_specification/libipspec.py:1-4
Timestamp: 2026-01-29T05:30:13.982Z
Learning: In the openshift-virtualization-tests repository, Python imports should use module import style for the standard library 'json' (import json) rather than 'from json import ...'. This improves readability by making calls like json.loads and json.dumps explicit, and aligns with patterns used for modules like logging, ipaddress, cloudinit, and pytest. Apply this to all Python test files under tests/ (and similar test directories).

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: 3690
File: tests/after_cluster_deploy_sanity/test_after_cluster_deploy_sanity.py:65-65
Timestamp: 2026-02-02T17:41:12.759Z
Learning: In test files, keep test_* functions with simple one-line docstrings. For helper functions, utilities, and library code with non-obvious return values or side effects, use Google-style docstrings with Args, Returns, and Side effects sections. Do not require Google-style docstrings for pytest test functions themselves.

Learnt from: RoniKishner
Repo: RedHatQE/openshift-virtualization-tests PR: 3697
File: tests/infrastructure/instance_types/test_common_vm_instancetype.py:53-98
Timestamp: 2026-02-03T07:34:34.184Z
Learning: In test files (Python, pytest), prefer using tier3 markers for categorization since tier2, tier1, and tier4 are not used in this repository. Do not rely on non-official markers; formalize and document the allowed markers in pytest.ini (or equivalent) to ensure consistent usage. When reviewing new tests, ensure markers align with this convention (only tier3, unless a project-wide decision defines additional tiers) and remove any unnecessary or undocumented markers.

Learnt from: vsibirsk
Repo: RedHatQE/openshift-virtualization-tests PR: 3577
File: tests/virt/conftest.py:251-267
Timestamp: 2026-02-10T15:04:14.799Z
Learning: In Python tests, remove all bare time.sleep() calls. Replace with a waiting mechanism such as TimeoutSampler from the timeout_sampler package or a function decorated with retry (with appropriate timeout/conditions) to ensure determinism and avoid flakiness. This applies to all Python tests under the tests directory (not just this file) to maintain consistent waiting behavior across the suite.

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/unittests/test_os_utils.py:333-425
Timestamp: 2026-02-23T16:31:34.505Z
Learning: In integration/functional tests located under the tests/ directory, require assertion failure messages using the pattern: assert condition, "descriptive message". For unit tests under utilities/unittests/, rely on pytest's assertion introspection and descriptive test names; explicit failure messages are not required. This guidance helps maintain clear diagnostics for integration tests while keeping unit tests concise and leveraging pytest's built-in introspection.

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: azhivovk
Repo: RedHatQE/openshift-virtualization-tests PR: 3930
File: tests/network/non_functional/ip_persistence/conftest.py:66-73
Timestamp: 2026-02-23T18:07:57.649Z
Learning: In all conftest.py files under the tests directory, do not flag or require fixes for Ruff FBT001 (Boolean-typed positional argument in function definition) in pytest fixture definitions. Pytest injects fixture parameters by name through dependency injection, so boolean positional arguments in fixture definitions are a false positive in this context. Apply this exception across all tests/* fixtures and document this behavior in review guidelines.

Learnt from: yossisegev
Repo: RedHatQE/openshift-virtualization-tests PR: 3873
File: tests/network/localnet/test_non_udn_localnet.py:7-9
Timestamp: 2026-02-25T10:52:09.679Z
Learning: In the RedHatQE/openshift-virtualization-tests repository, networking infrastructure requirements (nmstate, localnet bridge mappings, NIC availability) are not automatically tier3; they are considered standard test environment capabilities. Only tests with truly platform-specific, time-consuming, or bare-metal requirements should be marked as tier3. Apply this guidance to all Python tests under tests/, including tests/network/localnet/test_non_udn_localnet.py, ensuring tier3 designation is reserved for genuine platform-specific or complex scenarios rather than general networking infra necessities.

Learnt from: yossisegev
Repo: RedHatQE/openshift-virtualization-tests PR: 3873
File: tests/network/localnet/test_non_udn_localnet.py:19-27
Timestamp: 2026-02-25T11:00:02.013Z
Learning: In the test codebase, do not import from conftest.py files. This avoids import ambiguity in pytest. Do not import constants or helpers defined in conftest.py into tests. If a value is needed in both conftest.py and test files, duplicate it in both places or place it in a separate utility module that is importable by tests.

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: azhivovk
Repo: RedHatQE/openshift-virtualization-tests PR: 4147
File: tests/network/upgrade/test_upgrade_network.py:166-177
Timestamp: 2026-03-19T10:36:59.023Z
Learning: In this repository’s pytest-based test files (under `tests/`), do not flag unused test method parameters/fixture arguments for removal when the parameters are intentionally kept only to enforce pytest fixture dependency ordering (e.g., an unused fixture like `bridge_on_one_node`). Treat this as an intentional convention consistent with other fixture definitions in the codebase, and do not open follow-up review issues for those unused parameters.

Learnt from: jpeimer
Repo: RedHatQE/openshift-virtualization-tests PR: 4267
File: tests/storage/cross_cluster_live_migration/conftest.py:530-531
Timestamp: 2026-03-25T11:24:07.687Z
Learning: In this repo’s OpenShift virtualization tests, it is a standard pattern to call `to_dict()` on `ocp-resources` objects (e.g., `DataVolume`) without using its return value. The call is used only to populate the object’s `res` attribute, which is then read or mutated (e.g., `dv.res[...] = ...`). Do not flag this as an unused return value and do not request adding an inline comment just to justify it, since maintainers treat this behavior as consistent and intentional across the codebase.

Learnt from: jpeimer
Repo: RedHatQE/openshift-virtualization-tests PR: 4267
File: tests/storage/cross_cluster_live_migration/test_cclm.py:96-106
Timestamp: 2026-03-29T13:51:25.599Z
Learning: In this repository, follow the existing pytest convention for `pytest.mark.parametrize` argument names: use a single comma-separated string for `argnames` (e.g., `"dv_wait_timeout, vms_for_cclm"`), not a tuple (e.g., `("dv_wait_timeout", "vms_for_cclm")`). Do not flag or suggest changing `argnames` to a tuple. Also note that PT006 is not enforced by Ruff in this repo, so reviewers should not treat PT006 as a reason to alter the `argnames` format.

Learnt from: azhivovk
Repo: RedHatQE/openshift-virtualization-tests PR: 4318
File: tests/network/bandwidth/test_bandwidth.py:1-3
Timestamp: 2026-03-31T08:35:22.802Z
Learning: In this repository, when reviewing Python test modules under tests/, only require an STP link in the module docstring for STD (new-feature) tests. If the test module is a support-exception test tied to SUPPORTEX-* tickets (e.g., SUPPORTEX-29574) and therefore has no STP, do not flag missing STP links in the module docstring.

Learnt from: azhivovk
Repo: RedHatQE/openshift-virtualization-tests PR: 4318
File: tests/network/l2_bridge/bandwidth/test_bandwidth.py:5-27
Timestamp: 2026-03-31T10:39:33.409Z
Learning: In this repository’s pytest suite, some STD (Standard Definition) placeholder tests under the `tests/` directory are intentionally implemented as docstring-only functions. They must be marked with `__test__ = False` immediately after the function definition (e.g., `test_x.__test__ = False`) to prevent pytest from collecting/executing them until the real test body exists. During code review, do not flag these as missing executable coverage and do not recommend removing or changing the `__test__ = False` setting when the placeholder pattern matches this accepted STD approach.

Learnt from: ema-aka-young
Repo: RedHatQE/openshift-virtualization-tests PR: 4342
File: tests/storage/cdi_config/test_cdi_config.py:195-205
Timestamp: 2026-04-02T09:03:57.004Z
Learning: When calling `utilities.storage.get_dv_size_from_datasource()` in this repo, do not require an extra `None` guard for its return value if the `datasource` argument originates from a fixture that uses `ensure_exists=True` (e.g., `fedora_data_source_scope_module`). Those fixtures guarantee the datasource exists and provides a valid size, and other repo usages follow the same pattern without additional fail-fast/`None` checks; therefore, reviewers should not flag missing `None` guarding in this specific fixture-driven scenario.

Learnt from: yossisegev
Repo: RedHatQE/openshift-virtualization-tests PR: 4328
File: tests/network/flat_overlay/test_multi_network_policy.py:32-32
Timestamp: 2026-04-14T13:00:57.514Z
Learning: In this repository’s pytest quarantine tests, do not flag or change `pytest.mark.jira(<id>, run=False)` as ineffective. `run=False` on `pytest.mark.jira()` is intentionally handled by the `pytest_jira` plugin, which conditionally skips the test when the referenced Jira issue is open. This is the correct behavior for **Category 1 (Product Bug)** quarantines; the `pytest.mark.xfail(run=False)` approach is reserved for **Category 2 (Automation Issue)** quarantines only (as documented in `docs/QUARANTINE_GUIDELINES.md`).

Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 4328
File: tests/network/flat_overlay/test_multi_network_policy.py:0-0
Timestamp: 2026-04-14T16:15:31.065Z
Learning: When reviewing Python tests, avoid redundant parentheses around f-strings in `pytest.mark.xfail` decorators. Prefer `pytest.mark.xfail(reason=f"...", run=False)` over `pytest.mark.xfail(reason=(f"..."), run=False)`, and flag/suggest removing the extra wrapping parentheses where found.

Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 4328
File: tests/network/flat_overlay/test_multi_network_policy.py:32-32
Timestamp: 2026-04-14T16:15:33.012Z
Learning: For this repository (RedHatQE/openshift-virtualization-tests), when reviewing any PR with "Quarantine" in the title or a `quarantine` label, check compliance with `docs/QUARANTINE_GUIDELINES.md` in any affected pytest test code. Specifically: (1) For Category 1 (Product Bug), require `pytest.mark.jira("CNV-XXXXX", run=False)` and do not suggest replacing it with `xfail` (the `pytest_jira` plugin conditionally skips when the Jira issue is open). (2) For Category 2 (Automation Issue), require `pytest.mark.xfail(run=False, reason=...)` (pytest handles the skip). During review, flag quarantine PRs that use the wrong marker/category, omit a Jira ticket reference for Category 1, or use `run=False` in an incorrect context; raise these compliance questions even if later resolution confirms the marker was correct.

Learnt from: servolkov
Repo: RedHatQE/openshift-virtualization-tests PR: 4542
File: tests/network/libs/bgp.py:333-354
Timestamp: 2026-04-21T19:08:39.771Z
Learning: In this codebase, when using `retry` imported from `timeout_sampler`, the decorated function must indicate success by returning a truthy value. The decorator retries until it encounters a truthy `sample` (i.e., it does `if sample: return sample`). Therefore, ensure `retry`-decorated functions include `return True` (or another truthy value) on success; avoid removing/altering it on the assumption that the caller ignores the return value. If the function returns `None` (or has no return statement), it will keep retrying and typically end in `TimeoutExpiredError`. 

Learnt from: azhivovk
Repo: RedHatQE/openshift-virtualization-tests PR: 4578
File: tests/network/localnet/nad_ref_change/test_nad_ref_change.py:18-18
Timestamp: 2026-04-26T11:44:20.150Z
Learning: In this repository’s STD (Standard Test Definition) PRs under `tests/**`, it’s expected that Polarion IDs may be placeholders (e.g., `pytest.mark.polarion("CNV-00000")`) while the test scenarios are still being agreed upon. If the test scenario is disabled for the moment (e.g., the scenario/module sets `__test__ = False`), reviewers should not treat placeholder Polarion IDs as a blocking issue. Real Polarion IDs should be filled in before or during the implementation PR.

Learnt from: dalia-frank
Repo: RedHatQE/openshift-virtualization-tests PR: 4603
File: tests/data_protection/oadp/test_velero.py:101-130
Timestamp: 2026-04-27T15:40:31.167Z
Learning: When reviewing tests in RedHatQE/openshift-virtualization-tests, do not require STD-first workflow and STP/RFE/Jira-epic module docstring/traceability for tests that are being re-enabled after previously being blocked by a product bug (e.g., after a CNV Jira bug fix). Only enforce these STD-first and STP/RFE/Jira-epic module docstring requirements for genuinely new feature tests; previously blocked tests that are now unblocked/re-enabled should not be flagged as missing traceability.

Learnt from: azhivovk
Repo: RedHatQE/openshift-virtualization-tests PR: 4569
File: tests/network/user_defined_network/rhel9_rhel10_cluster/test_connectivity.py:17-51
Timestamp: 2026-05-03T14:47:13.096Z
Learning: In this repository’s pytest suite (files under `tests/`), it is acceptable to have identically named `test_*` functions in different test modules (e.g., same function name in different `.../test_*.py` files). Do not request renaming solely to disambiguate function names across modules, since pytest node IDs remain unique and unambiguous due to the module path prefix. The team accepts the trade-off that `pytest -k <name>` will match tests in all modules containing that name.

Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 4701
File: tests/virt/node/general/test_windows_vtpm_bitlocker.py:50-52
Timestamp: 2026-05-03T15:38:09.624Z
Learning: During review of PRs that are lint cleanups or tooling/version bumps, do not flag code-quality issues for patterns that pre-existed before the PR. Specifically, if the diff does not introduce/modify constructs such as nested `if` blocks or unnecessary list comprehensions, treat them as known/deferred and leave them for dedicated follow-up cleanup PRs. Only raise issues when the PR itself adds, changes, or refactors the problematic code.

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: dshchedr
Repo: RedHatQE/openshift-virtualization-tests PR: 4739
File: tests/virt/node/descheduler/conftest.py:145-145
Timestamp: 2026-05-05T17:27:32.109Z
Learning: In this repo’s Python tests, `pyhelper_utils.shell.run_command` defaults to `check=True` (so it raises `subprocess.CalledProcessError` on non-zero exit). Therefore, don’t flag missing return-code handling/rc checks for bare calls like `run_command(...)` (or calls where `check` is not explicitly provided). Only flag cases where `check=False` is explicitly passed and the exit code/return value is ignored or not otherwise handled.

Learnt from: dshchedr
Repo: RedHatQE/openshift-virtualization-tests PR: 4739
File: tests/virt/node/descheduler/conftest.py:142-146
Timestamp: 2026-05-05T18:28:01.097Z
Learning: In this repository, ignore Ruff rule PT022 in Python test files under `tests/`. If PT022 is triggered in a `pytest` fixture that uses `yield` but has no teardown code, treat it as an acceptable low-value nitpick and do not suggest changing the fixture to use `return` or otherwise flag the issue during code review.

except TimeoutExpiredError:
LOGGER.error(
f"Failed to fetch runbooks directory listing from '{runbooks_api_url}', "
Expand Down
6 changes: 4 additions & 2 deletions tests/observability/runbook_url/test_runbook_url.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

import pytest

from utilities.constants import CNV_PROMETHEUS_RULES
from utilities.constants import AAQ_PROMETHEUS_RULE, CNV_PROMETHEUS_RULES

LOGGER = logging.getLogger(__name__)

Expand All @@ -29,7 +29,7 @@ def validate_downstream_runbook_url(

class TestRunbookUrlsAndPrometheusRules:
@pytest.mark.polarion("CNV-10081")
def test_no_new_prometheus_rules(self, cnv_prometheus_rules_names, hpp_cr_installed):
def test_no_new_prometheus_rules(self, cnv_prometheus_rules_names, hpp_cr_installed, aaq_enabled):
"""
Since validations for runbook url of all cnv alerts are done via polarion parameterization of prometheusrules,
this test has been added to catch any new cnv prometheusrules that is not part of cnv_prometheus_rules_matrix
Expand All @@ -38,6 +38,8 @@ def test_no_new_prometheus_rules(self, cnv_prometheus_rules_names, hpp_cr_instal
if not hpp_cr_installed:
LOGGER.warning("HPP CR is not installed, removing prometheus-hpp-rules from the list of prometheus rules")
expected_prometheus_rules_names.remove("prometheus-hpp-rules")
if aaq_enabled:
expected_prometheus_rules_names.append(AAQ_PROMETHEUS_RULE)
assert sorted(cnv_prometheus_rules_names) == sorted(expected_prometheus_rules_names), (
f"New cnv prometheusrule found: {set(cnv_prometheus_rules_names) - set(expected_prometheus_rules_names)}"
)
Expand Down
1 change: 1 addition & 0 deletions utilities/constants.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -722,6 +722,7 @@ class UpgradeStreams:
f"kubevirt-cnv-{PROMETHEUS_RULES_STR}",
KUBEMACPOOL_PROMETHEUS_RULE,
]
AAQ_PROMETHEUS_RULE = "prometheus-aaq-rules"


class StorageClassNames:
Expand Down
Loading