Handle missing cpu-arch config gracefully in pytest fixture#5285
Conversation
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughAdds optional architecture selection to VM image resolution and updates test fixtures to read CPU architecture values from safer or different config keys. ChangesArchitecture-aware Container Image and Fixture Updates
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Your configuration references 1 linked repositories, but your current plan allows 0. Analyzed ``, skipped 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. Comment |
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Branch Management
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
Security Checks
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
openshift-virtualization-qe-bot
added
size/XS
tox:verify-tc-requirement-polarion:running
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/conftest.py`:
- Line 1046: The fixture is using `.get("cpu_arch")` which returns None when the
key is missing, masking configuration errors instead of failing fast. Replace
the `.get()` call with direct dictionary access using square brackets (e.g.,
`py_config["cpu_arch"]`) to raise a KeyError immediately if the key is missing.
Additionally, the returned value needs to handle comma-separated architecture
strings (like `amd64,arm64`) by normalizing them to a single architecture value
that downstream code expecting `nodes_cpu_architecture` can safely use in
architecture map lookups.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 446a9497-c457-452b-9741-232b2aa4aa34
📒 Files selected for processing (1)
tests/conftest.py
|
/verified Verify that the list value of Verify nothing is broken in specific-arch test in multi-arch cluster, i.e. All tests passed, and VM spec was created with the explicit Verify nothing break on homogeneous cluster: All tests passed, and VM spec was created without |
Test execution plan request detailsCRITICAL: You MUST post an inline review comment on the first changed line of the first file. As an expert software testing engineer, analyze all modified files in this PR and create a targeted test execution plan. Analysis Requirements:
Output rules: Your deliverable: Test Execution Plan
Use these formats:
Real test commands (MANDATORY when changes affect session/runtime code): When the affected code runs at session/collection time (conftest fixtures, pytest plugins,
Example output for a session-startup fix: Guidelines:
Hardware-Related Checks (SR-IOV, GPU, DPDK): When PR modifies fixtures for hardware-specific resources:
CRITICAL WORKFLOW COMPLETION RULES: When responding to this test execution plan request, you MUST follow these rules EXACTLY:
ABSOLUTE PROHIBITIONS (violating these creates empty/meaningless reviews):
Remember: The pull request review is visible to users. Additional comments are redundant noise. CRITICAL — Verification rules for smoke and gating decisions: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Inspect the nodes_cpu_architecture fixture in tests/conftest.py
rg -n 'nodes_cpu_architecture|cpu_arch' tests/conftest.py -A5 -B2
echo "=== Find all callers of nodes_cpu_architecture ==="
rg -rn 'nodes_cpu_architecture' tests/ --include='*.py' -l
echo "=== Find smoke-marked tests ==="
rg -rn '`@pytest.mark.smoke`' tests/ --include='*.py' -l | head -30
echo "=== Find gating-marked tests ==="
rg -rn '`@pytest.mark.gating`' tests/ --include='*.py' -l | head -30
echo "=== Check if nodes_cpu_architecture is autouse ==="
rg -n 'autouse.*nodes_cpu_architecture|nodes_cpu_architecture.*autouse' tests/conftest.py
echo "=== Show full fixture definition ==="
rg -n 'def nodes_cpu_architecture' tests/conftest.py -A20Length of output: 7515 🏁 Script executed: #!/bin/bash
echo "=== Find all callers of nodes_cpu_architecture in tests ==="
rg -rn 'nodes_cpu_architecture' tests/ -l
echo "=== Find smoke-marked tests ==="
rg -rn '`@pytest.mark.smoke`' tests/ -l | head -30
echo "=== Find gating-marked tests ==="
rg -rn '`@pytest.mark.gating`' tests/ -l | head -30
echo "=== Check which test files use host_cpu_model, cpu_for_migration, modern_cpu_for_migration, machine_type_from_kubevirt_config, is_s390x_cluster ==="
rg -rn 'host_cpu_model|cpu_for_migration|modern_cpu_for_migration|machine_type_from_kubevirt_config|is_s390x_cluster' tests/ -l | head -30
echo "=== Check which test files use skip_if_no_common_cpu ==="
rg -rn 'skip_if_no_common_cpu' tests/ -l | head -20Length of output: 5026 🏁 Script executed: #!/bin/bash
echo "=== Check nodes_cpu_architecture usage in tests/virt/node/conftest.py ==="
rg -n 'nodes_cpu_architecture|autouse' tests/virt/node/conftest.py -B2 -A5
echo "=== Get the diff to identify first changed line ==="
git diff HEAD~1 -- tests/conftest.py | head -40
echo "=== Check if any smoke test files are in the downstream fixture users ==="
# smoke files: test_container_disk_vm.py, test_csv.py, test_rhel_os_support.py, test_clone.py, test_upload_virtctl.py, test_rhel_os.py
rg -n 'cpu_for_migration|host_cpu_model|modern_cpu_for_migration|is_s390x_cluster|machine_type_from_kubevirt_config|nodes_cpu_architecture|skip_if_no_common_cpu' \
tests/virt/node/general/test_container_disk_vm.py \
tests/install_upgrade_operators/csv/test_csv.py \
tests/virt/cluster/common_templates/rhel/test_rhel_os_support.py \
tests/storage/cdi_clone/test_clone.py \
tests/storage/cdi_upload/test_upload_virtctl.py \
tests/infrastructure/instance_types/supported_os/test_rhel_os.py 2>/dev/null
echo "=== Check conftest hierarchy for smoke test test_container_disk_vm.py ==="
rg -n 'nodes_cpu_architecture|cpu_for_migration|host_cpu_model|is_s390x_cluster' tests/virt/conftest.py | head -20
echo "=== Get PR diff first changed line number ==="
git diff HEAD~1 -- tests/conftest.py | grep '^@@' | head -5Length of output: 2618 🏁 Script executed: #!/bin/bash
# Post inline review comment on the changed line in tests/conftest.py
PR_NUMBER=5285
REPO="RedHatQE/openshift-virtualization-tests"
COMMENT_BODY="## Test Execution Plan
**Run smoke tests: False**
No smoke-marked test file has a verified dependency path to \`nodes_cpu_architecture\`. The \`vmx_disabled_flag\` fixture in \`tests/virt/node/conftest.py\` uses \`nodes_cpu_architecture\` but is not \`autouse\`, so it only affects tests that explicitly request it — none of which are smoke-marked.
**Run gating tests: True**
- \`tests/virt/node/general/test_machinetype.py\` (gating) → directly requests \`nodes_cpu_architecture\` fixture
- \`tests/virt/node/cpu_sockets_threads/test_cpu_support_sockets_threads.py\` (gating) → \`cpu_for_migration\` fixture → \`nodes_cpu_architecture\`
- \`tests/virt/node/migration_and_maintenance/test_vm_unscheduled_node.py\` (gating) → \`cpu_for_migration\` → \`nodes_cpu_architecture\`
**Affected tests to run**
- \`tests/virt/node/general/test_machinetype.py\` (gating — direct \`nodes_cpu_architecture\` user)
- \`tests/virt/node/cpu_sockets_threads/test_cpu_support_sockets_threads.py\` (gating — via \`cpu_for_migration\`)
- \`tests/install_upgrade_operators/strict_reconciliation/test_hco_default_cpu_model.py\` (direct \`nodes_cpu_architecture\` user)
- \`tests/install_upgrade_operators/must_gather/test_must_gather_vms.py\` (direct \`nodes_cpu_architecture\` + \`skip_if_no_common_cpu\` user)
**Real tests (cluster required)**
*Error path (the fix — session startup without \`--cpu-arch\`):*
\`\`\`
uv run pytest tests/virt/node/general/test_machinetype.py --collect-only
\`\`\`
Expected: collection succeeds (no \`KeyError\` on \`py_config[\"cpu_arch\"]\`)
*Happy path — single arch regression:*
\`\`\`
uv run pytest tests/virt/node/general/test_machinetype.py --tc-file=tests/global_config_arm64.py --cpu-arch=arm64 -k test_default_machine_type
\`\`\`
Expected: session starts normally, \`nodes_cpu_architecture == \"arm64\"\`
*Multi-arch path (the new feature):*
\`\`\`
uv run pytest tests/network/network_service/test_service_multiarch.py --cpu-arch=arm64,amd64 -m multiarch --collect-only
\`\`\`
Expected: session starts, \`nodes_cpu_architecture == \"arm64,amd64\"\`"
gh api \
--method POST \
-H "Accept: application/vnd.github+json" \
"/repos/${REPO}/pulls/${PR_NUMBER}/reviews" \
-f body="" \
-f event="COMMENT" \
-f "comments[][path]=tests/conftest.py" \
-F "comments[][line]=1046" \
-f "comments[][side]=RIGHT" \
-f "comments[][body]=${COMMENT_BODY}"Length of output: 4329 |
yossisegev
changed the title
e71c6d6 to
67e366f
Compare
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
/verified Verify that the list value of Verify nothing is broken in specific-arch test in multi-arch cluster, i.e. All tests passed, and VM spec was created with the explicit Verify nothing break on homogeneous cluster: All tests passed, and VM spec was created without Verify cluster arch list is printed as expected in cluster info. |
openshift-virtualization-qe-bot
removed
the
tox:verify-tc-requirement-polarion:running
|
/verified
|
The current implementation of checking the `cpu-arch` argument only supports a single value, however for multi-arch tests it is required to pass a list of architectures (`--cpu-arch=amd64,arm64`).", and be sure to pull a VM image of the correct arch. Signed-off-by: Yossi Segev <ysegev@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: <ysegev@redhat.com>
for more information, see https://pre-commit.ci
|
/verified Verified by running the affected tests (test_ip_specification.py) on different setups, and having the test passing + expected output:
|
Test execution plan request detailsCRITICAL: You MUST post an inline review comment on the first changed line of the first file. As an expert software testing engineer, analyze all modified files in this PR and create a targeted test execution plan. Analysis Requirements:
Output rules: Your deliverable: Test Execution Plan
Use these formats:
Real test commands (MANDATORY when changes affect session/runtime code): When the affected code runs at session/collection time (conftest fixtures, pytest plugins,
Example output for a session-startup fix: Guidelines:
Hardware-Related Checks (SR-IOV, GPU, DPDK): When PR modifies fixtures for hardware-specific resources:
CRITICAL WORKFLOW COMPLETION RULES: When responding to this test execution plan request, you MUST follow these rules EXACTLY:
ABSOLUTE PROHIBITIONS (violating these creates empty/meaningless reviews):
Remember: The pull request review is visible to users. Additional comments are redundant noise. CRITICAL — Verification rules for smoke and gating decisions: |
Test Execution PlanRun smoke tests: False Run gating tests: True
Affected tests to run
Real tests (cluster required) Error path (shared session/runtime fix): Happy path (plugin + shared fixture wiring regression): Runtime regression: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
BASE="$(git merge-base HEAD origin/main 2>/dev/null || true)"
if [[ -z "${BASE}" ]]; then
BASE="HEAD~1"
fi
echo "=== MERGE BASE ==="
echo "$BASE"
echo "=== CHANGED FILES IN ORDER ==="
git diff --name-only "$BASE"...HEAD
echo "=== FIRST HUNK FOR EACH CHANGED FILE ==="
for f in $(git diff --name-only "$BASE"...HEAD); do
echo "--- $f ---"
git diff --unified=0 "$BASE"...HEAD -- "$f" | sed -n '1,40p'
done
echo "=== AST OUTLINES ==="
ast-grep outline tests/conftest.py --view expanded | sed -n '1,220p'
echo "---"
ast-grep outline libs/vm/factory.py --view expanded | sed -n '1,220p'
echo "---"
ast-grep outline libs/vm/vm.py --view expanded | sed -n '1,220p'
echo "=== MODIFIED SYMBOLS IN LIB FILES ==="
git diff --unified=0 "$BASE"...HEAD -- libs/vm/factory.py libs/vm/vm.py | rg -n '^[+-]\s*def '
echo "=== RELEVANT SLICES: tests/conftest.py ==="
rg -n -C3 'def nodes_cpu_architecture|def cluster_info|cpu_arch|cluster_arch|def cpu_for_migration|def host_cpu_model|def modern_cpu_for_migration|def machine_type_from_kubevirt_config|def skip_if_no_common_cpu' tests/conftest.py
echo "=== RELEVANT SLICES: libs/vm/factory.py ==="
rg -n -C3 'def fedora_image|def _fill_vm_spec_defaults|fedora_image\(|container_image\(' libs/vm/factory.py
echo "=== RELEVANT SLICES: libs/vm/vm.py ==="
rg -n -C3 'def container_image|get_oc_image_info|cpu_arch|architecture' libs/vm/vm.py
echo "=== DIRECT CALLERS IN tests/ AND conftest OF CHANGED SYMBOLS ==="
rg -n --type py -C2 '\b(fedora_image|container_image)\s*\(' tests
echo "--- nodes_cpu_architecture users ---"
rg -n --type py -C2 '\bnodes_cpu_architecture\b' tests
echo "=== SAME-FILE / TRANSITIVE CALLERS IN libs/ ==="
rg -n --type py -C2 '\bcontainer_image\s*\(' libs
echo "---"
rg -n --type py -C2 '\bfedora_image\s*\(' libs
echo "=== FIXTURE / HELPER CHAINS TO FEDORA VM HELPERS ==="
rg -n --type py -C2 '\bfedora_vm\b|\bfedora_vm_body\b|_fill_vm_spec_defaults' tests libs
echo "=== SMOKE-MARKED TEST FILES ==="
rg -l --type py '^\s*`@pytest`\.mark\.smoke\b' tests | sort
echo "=== GATING-MARKED TEST FILES ==="
rg -l --type py '^\s*`@pytest`\.mark\.gating\b' tests | sort
echo "=== OVERLAP: SMOKE FILES THAT MENTION AFFECTED FIXTURES/HELPERS ==="
smoke_files="$(mktemp)"
rg -l --type py '^\s*`@pytest`\.mark\.smoke\b' tests | sort > "$smoke_files"
while IFS= read -r f; do
if rg -q --type py '\b(fedora_vm|fedora_image|container_image|nodes_cpu_architecture|cpu_for_migration|machine_type_from_kubevirt_config|host_cpu_model|modern_cpu_for_migration|skip_if_no_common_cpu)\b' "$f"; then
echo "$f"
rg -n --type py -C2 '\b(fedora_vm|fedora_image|container_image|nodes_cpu_architecture|cpu_for_migration|machine_type_from_kubevirt_config|host_cpu_model|modern_cpu_for_migration|skip_if_no_common_cpu)\b' "$f"
fi
done < "$smoke_files"
echo "=== OVERLAP: GATING FILES THAT MENTION AFFECTED FIXTURES/HELPERS ==="
gating_files="$(mktemp)"
rg -l --type py '^\s*`@pytest`\.mark\.gating\b' tests | sort > "$gating_files"
while IFS= read -r f; do
if rg -q --type py '\b(fedora_vm|fedora_image|container_image|nodes_cpu_architecture|cpu_for_migration|machine_type_from_kubevirt_config|host_cpu_model|modern_cpu_for_migration|skip_if_no_common_cpu)\b' "$f"; then
echo "$f"
rg -n --type py -C2 '\b(fedora_vm|fedora_image|container_image|nodes_cpu_architecture|cpu_for_migration|machine_type_from_kubevirt_config|host_cpu_model|modern_cpu_for_migration|skip_if_no_common_cpu)\b' "$f"
fi
done < "$gating_files"
echo "=== CONFTST HIERARCHY CHECK FOR SMOKE FILES ==="
python - <<'PY'
from pathlib import Path
import subprocess
smoke = subprocess.check_output(
["rg", "-l", "--type", "py", r"^\s*`@pytest`\.mark\.smoke\b", "tests"],
text=True
).splitlines()
symbols = r"\b(fedora_vm|fedora_image|container_image|nodes_cpu_architecture|cpu_for_migration|machine_type_from_kubevirt_config|host_cpu_model|modern_cpu_for_migration|skip_if_no_common_cpu)\b"
seen = set()
for f in smoke:
p = Path(f).parent
while True:
c = p / "conftest.py"
key = (f, str(c))
if c.exists() and key not in seen:
seen.add(key)
out = subprocess.run(
["rg", "-n", "--type", "py", "-C2", symbols, str(c)],
text=True, capture_output=True
)
if out.returncode == 0 and out.stdout.strip():
print(f"SMOKE FILE: {f}")
print(f"CONFTST: {c}")
print(out.stdout.rstrip())
print("---")
if p == Path("tests"):
break
p = p.parent
PYLength of output: 71640 🏁 Script executed: #!/bin/bash
set -euo pipefail
BASE="$(git merge-base HEAD origin/main 2>/dev/null || true)"
if [[ -z "${BASE}" ]]; then
BASE="HEAD~1"
fi
echo "=== FIRST CHANGED FILE / LINE ==="
first_file="$(git diff --name-only "$BASE"...HEAD | head -1)"
echo "first_file=$first_file"
git diff --unified=0 "$BASE"...HEAD -- "$first_file" | sed -n '1,20p'
echo "=== MODIFIED LIB SYMBOLS ==="
printf '%s\n' \
'dict_normalization_for_dataclass' \
'cluster_vlans' \
'NodeNetworkConfigurationPolicy' \
'update_nad_references' \
'fedora_image' \
'container_image'
echo "=== DIRECT TEST/CONFTEST CALLERS OR IMPORTERS ==="
for sym in dict_normalization_for_dataclass cluster_vlans update_nad_references fedora_image container_image; do
echo "--- symbol: $sym ---"
rg -n --type py -C2 "\\b${sym}\\b" tests conftest.py || true
done
echo "--- module/class: libs.net.nodenetworkconfigurationpolicy imports/usages ---"
rg -n --type py -C2 'nodenetworkconfigurationpolicy|libnncp|NodeNetworkConfigurationPolicy' tests conftest.py || true
echo "=== FIXTURE CHAINS / SAME-FILE CALLERS ==="
echo "--- fedora_image / _fill_vm_spec_defaults / fedora_vm ---"
rg -n --type py -C2 '\bfedora_image\b|\b_fill_vm_spec_defaults\b|\bfedora_vm\b' libs tests conftest.py || true
echo "--- cluster_vlans / cluster_vlan_ids ---"
rg -n --type py -C2 '\bcluster_vlans\b|\bcluster_vlan_ids\b' libs tests conftest.py || true
echo "--- update_nad_references ---"
rg -n --type py -C2 '\bupdate_nad_references\b' libs tests conftest.py || true
echo "=== MARKER STATUS FOR CANDIDATE TEST FILES ==="
candidates=(
tests/storage/cdi_upload/test_upload_virtctl.py
tests/storage/disk_preallocation/test_disk_preallocation.py
tests/install_upgrade_operators/deployment/test_hco_deployment_params.py
tests/install_upgrade_operators/pod_validation/test_pod_spec.py
tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py
tests/network/localnet/nad_ref_change/test_nad_ref_change.py
tests/network/l2_bridge/rhel9_rhel10_cluster/test_connectivity.py
tests/network/localnet/rhel9_rhel10_cluster/test_connectivity.py
tests/network/connectivity/conftest.py
tests/network/l2_bridge/conftest.py
tests/network/localnet/conftest.py
)
for f in "${candidates[@]}"; do
if [[ -f "$f" ]]; then
echo "--- $f ---"
rg -n --type py '^\s*`@pytest`\.mark\.(smoke|gating)\b|^pytestmark\s*=' "$f" || true
fi
done
echo "=== GLOBAL SMOKE FILES THAT DIRECTLY USE CHANGED SYMBOLS ==="
smoke_files=$(mktemp)
rg -l --type py '^\s*`@pytest`\.mark\.smoke\b' tests | sort > "$smoke_files"
while IFS= read -r f; do
if rg -q --type py '\b(dict_normalization_for_dataclass|cluster_vlans|cluster_vlan_ids|nodenetworkconfigurationpolicy|libnncp|update_nad_references|fedora_image|container_image|fedora_vm|nodes_cpu_architecture)\b' "$f"; then
echo "--- $f ---"
rg -n --type py -C2 '\b(dict_normalization_for_dataclass|cluster_vlans|cluster_vlan_ids|nodenetworkconfigurationpolicy|libnncp|update_nad_references|fedora_image|container_image|fedora_vm|nodes_cpu_architecture)\b' "$f" || true
fi
done < "$smoke_files"
echo "=== GLOBAL GATING FILES THAT DIRECTLY USE CHANGED SYMBOLS ==="
gating_files=$(mktemp)
rg -l --type py '^\s*`@pytest`\.mark\.gating\b' tests | sort > "$gating_files"
while IFS= read -r f; do
if rg -q --type py '\b(dict_normalization_for_dataclass|cluster_vlans|cluster_vlan_ids|nodenetworkconfigurationpolicy|libnncp|update_nad_references|fedora_image|container_image|fedora_vm|nodes_cpu_architecture)\b' "$f"; then
echo "--- $f ---"
rg -n --type py -C2 '\b(dict_normalization_for_dataclass|cluster_vlans|cluster_vlan_ids|nodenetworkconfigurationpolicy|libnncp|update_nad_references|fedora_image|container_image|fedora_vm|nodes_cpu_architecture)\b' "$f" || true
fi
done < "$gating_files"
echo "=== CHANGED TEST FILES WITH SMOKE/GATING MARKERS ==="
git diff --name-only "$BASE"...HEAD -- tests | while read -r f; do
[[ -f "$f" ]] || continue
if rg -q --type py '^\s*`@pytest`\.mark\.(smoke|gating)\b|^pytestmark\s*=' "$f"; then
echo "--- $f ---"
rg -n --type py '^\s*`@pytest`\.mark\.(smoke|gating)\b|^pytestmark\s*=' "$f" || true
fi
doneLength of output: 55747 🏁 Script executed: #!/bin/bash
set -euo pipefail
REPO="RedHatQE/openshift-virtualization-tests"
PR_NUMBER="5285"
PATH_IN_PR=".github/workflows/retest-smoke.yml"
LINE_NO=1
read -r -d '' COMMENT_BODY <<'EOF' || true
## Test Execution Plan
**Run smoke tests: False**
No smoke-marked test has a verified dependency path to the modified shared libraries/fixtures. The changed addition in `tests/storage/cdi_upload/test_upload_virtctl.py` is not smoke-marked, and no smoke-marked file was verified to depend on `nodes_cpu_architecture`, `cluster_vlan_ids`, `update_nad_references`, or the moved `libs.net.nodenetworkconfigurationpolicy` helpers.
**Run gating tests: True**
Verified paths:
- `tests/virt/node/general/test_machinetype.py` (gating) → `nodes_cpu_architecture` fixture → modified `tests/conftest.py`
- `tests/storage/disk_preallocation/test_disk_preallocation.py::test_preallocation_dv` (gating) → changed fixtures in `tests/storage/disk_preallocation/conftest.py`
- `tests/storage/disk_preallocation/test_disk_preallocation.py::test_preallocation_globally_dv_spec_without_preallocation` (gating) → changed fixtures in `tests/storage/disk_preallocation/conftest.py`
- `tests/install_upgrade_operators/deployment/test_hco_deployment_params.py` (gating) → assertions/xfail flow changed directly in the test file
**Affected tests to run**
- `tests/virt/node/general/test_machinetype.py` (gating)
- `tests/install_upgrade_operators/deployment/test_hco_deployment_params.py` (gating)
- `tests/storage/disk_preallocation/test_disk_preallocation.py::test_preallocation_dv` (gating)
- `tests/storage/disk_preallocation/test_disk_preallocation.py::test_preallocation_globally_dv_spec_without_preallocation` (gating)
- `tests/install_upgrade_operators/pod_validation/test_pod_spec.py`
- `tests/storage/cdi_upload/test_upload_virtctl.py::test_virtctl_image_upload_dv_in_pudn_namespace`
- `tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py`
- `tests/network/localnet/nad_ref_change/test_nad_ref_change.py`
- `tests/network/l2_bridge/rhel9_rhel10_cluster/test_connectivity.py`
- `tests/network/localnet/rhel9_rhel10_cluster/test_connectivity.py`
**Real tests (cluster required)**
Error path (shared session/runtime fix):
`uv run pytest tests/virt/node/general/test_machinetype.py --collect-only`
Expected: collection succeeds without `KeyError: 'cpu_arch'`
Happy path (plugin + shared fixture wiring regression):
`uv run pytest tests/network/localnet/rhel9_rhel10_cluster/test_connectivity.py --collect-only`
Expected: collection succeeds with the plugin-provided network fixtures and no fixture/import errors
Runtime regression:
`uv run pytest tests/storage/cdi_upload/test_upload_virtctl.py -k test_virtctl_image_upload_dv_in_pudn_namespace`
Expected: DataVolume upload succeeds in a namespace with a primary UDN
EOF
payload="$(mktemp)"
jq -n \
--arg body "" \
--arg event "COMMENT" \
--arg path "$PATH_IN_PR" \
--arg bodyText "$COMMENT_BODY" \
--argjson line "$LINE_NO" \
'{
body: $body,
event: $event,
comments: [
{
path: $path,
line: $line,
side: "RIGHT",
body: $bodyText
}
]
}' > "$payload"
for attempt in 1 2; do
gh api \
--method POST \
-H "Accept: application/vnd.github+json" \
"/repos/${REPO}/pulls/${PR_NUMBER}/reviews" \
--input "$payload" \
>/dev/null && exit 0
sleep 2
done
exit 1Length of output: 3810 🏁 Script executed: #!/bin/bash
set -euo pipefail
REPO="RedHatQE/openshift-virtualization-tests"
PR_NUMBER="5285"
PATH_IN_PR=".github/workflows/retest-smoke.yml"
HEAD_SHA="$(gh api "/repos/${REPO}/pulls/${PR_NUMBER}" --jq '.head.sha')"
read -r -d '' COMMENT_BODY <<'EOF' || true
## Test Execution Plan
**Run smoke tests: False**
No smoke-marked test has a verified dependency path to the modified shared libraries/fixtures. The changed addition in `tests/storage/cdi_upload/test_upload_virtctl.py` is not smoke-marked, and no smoke-marked file was verified to depend on `nodes_cpu_architecture`, `cluster_vlan_ids`, `update_nad_references`, or the moved `libs.net.nodenetworkconfigurationpolicy` helpers.
**Run gating tests: True**
Verified paths:
- `tests/virt/node/general/test_machinetype.py` (gating) → `nodes_cpu_architecture` fixture → modified `tests/conftest.py`
- `tests/storage/disk_preallocation/test_disk_preallocation.py::test_preallocation_dv` (gating) → changed fixtures in `tests/storage/disk_preallocation/conftest.py`
- `tests/storage/disk_preallocation/test_disk_preallocation.py::test_preallocation_globally_dv_spec_without_preallocation` (gating) → changed fixtures in `tests/storage/disk_preallocation/conftest.py`
- `tests/install_upgrade_operators/deployment/test_hco_deployment_params.py` (gating) → assertions/xfail flow changed directly in the test file
**Affected tests to run**
- `tests/virt/node/general/test_machinetype.py` (gating)
- `tests/install_upgrade_operators/deployment/test_hco_deployment_params.py` (gating)
- `tests/storage/disk_preallocation/test_disk_preallocation.py::test_preallocation_dv` (gating)
- `tests/storage/disk_preallocation/test_disk_preallocation.py::test_preallocation_globally_dv_spec_without_preallocation` (gating)
- `tests/install_upgrade_operators/pod_validation/test_pod_spec.py`
- `tests/storage/cdi_upload/test_upload_virtctl.py::test_virtctl_image_upload_dv_in_pudn_namespace`
- `tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py`
- `tests/network/localnet/nad_ref_change/test_nad_ref_change.py`
- `tests/network/l2_bridge/rhel9_rhel10_cluster/test_connectivity.py`
- `tests/network/localnet/rhel9_rhel10_cluster/test_connectivity.py`
**Real tests (cluster required)**
Error path (shared session/runtime fix):
`uv run pytest tests/virt/node/general/test_machinetype.py --collect-only`
Expected: collection succeeds without `KeyError: 'cpu_arch'`
Happy path (plugin + shared fixture wiring regression):
`uv run pytest tests/network/localnet/rhel9_rhel10_cluster/test_connectivity.py --collect-only`
Expected: collection succeeds with the plugin-provided network fixtures and no fixture/import errors
Runtime regression:
`uv run pytest tests/storage/cdi_upload/test_upload_virtctl.py -k test_virtctl_image_upload_dv_in_pudn_namespace`
Expected: DataVolume upload succeeds in a namespace with a primary UDN
EOF
# Retry inline review comment using diff position on the first changed line of the first changed file.
for attempt in 1 2; do
if gh api \
--method POST \
-H "Accept: application/vnd.github+json" \
"/repos/${REPO}/pulls/${PR_NUMBER}/comments" \
-f body="$COMMENT_BODY" \
-f commit_id="$HEAD_SHA" \
-f path="$PATH_IN_PR" \
-F position=1 \
>/dev/null
then
exit 0
fi
sleep 2
done
# Fallback: regular PR comment
gh api \
--method POST \
-H "Accept: application/vnd.github+json" \
"/repos/${REPO}/issues/${PR_NUMBER}/comments" \
-f body="$COMMENT_BODY" \
>/dev/nullLength of output: 3709 |
|
/build-and-push-container |
@coderabbitai I verified per your test-plan in the job tests-runner #5786 (the full URL is removed). |
|
Tip For best results, initiate chat on the files or code changes.
LOW: The result looks consistent with the scope of this PR: 81 passing tests across the targeted paths is strong coverage, and the 2 failures being isolated to golden image data-source retrieval do not look causally tied to the That distinction matters because the purpose of the requested verification was to prove:
Based on your run, that evidence is now in place. |
|
@coderabbitai I see the icon saying that there are open review comments from you, but I don't find any. Please point me to any open issues you have, or otherwise change the comments icon to a green checkmark. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="RedHatQE/openshift-virtualization-tests"
owner="${repo%%/*}"
name="${repo#*/}"
pr_number=5285
echo "=== Review threads (resolution state + participants) ==="
gh api graphql \
-F owner="$owner" \
-F name="$name" \
-F number="$pr_number" \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
isResolved
isOutdated
path
line
comments(first: 20) {
nodes {
author { login }
body
createdAt
url
}
}
}
}
}
}
}' | jq -r '
.data.repository.pullRequest.reviewThreads.nodes[]
| {
isResolved,
isOutdated,
path,
line,
authors: ([.comments.nodes[].author.login] | unique),
comments: [.comments.nodes[] | {author: .author.login, url, body: (.body | gsub("\n"; " ") | .[0:160])}]
}'
echo "=== Unresolved threads involving coderabbitai[bot] ==="
gh api graphql \
-F owner="$owner" \
-F name="$name" \
-F number="$pr_number" \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
isResolved
isOutdated
path
line
comments(first: 20) {
nodes {
author { login }
body
url
}
}
}
}
}
}
}' | jq -r '
.data.repository.pullRequest.reviewThreads.nodes[]
| select(.isResolved == false)
| select(any(.comments.nodes[]; .author.login == "coderabbitai[bot]"))
| {
path,
line,
comments: [.comments.nodes[] | {author: .author.login, url, body: (.body | gsub("\n"; " ") | .[0:200])}]
}'
echo "=== Recent issue comments by coderabbitai[bot] ==="
gh api "/repos/$repo/issues/$pr_number/comments" --paginate \
--jq '.[] | select(.user.login=="coderabbitai[bot]") | {created_at, html_url, body: (.body|.[0:200])}'Length of output: 14275
Why this matters: GitHub’s review indicator is driven by unresolved review state, so if there are no open threads from me, the right fix is to explicitly resolve my remaining review state so the UI can catch up. I’m clearing my review comments now. |
|
Successfully removed PR tag: quay.io/openshift-cnv/openshift-virtualization-tests:pr-5285. |
|
/cherry-pick cnv-4.22 |
2 similar comments
|
/cherry-pick cnv-4.22 |
|
/cherry-pick cnv-4.22 |
|
Cherry-picked PR Handle missing cpu-arch config gracefully in pytest fixture into cnv-4.22: #5427 |
12 participants
The current implementation of checking the
cpu-archargument only supports a single value, however for multi-arch tests it is required to pass a list of architectures (--cpu-arch=amd64,arm64)."The fixture now uses dict.get() instead of direct indexing to handle missing cpu_arch gracefully.
cpu_archis not set for multi-arch cluster runs now (by design), and this fix handles the KeyError failure.Assisted-by: Claude Code noreply@anthropic.com
Summary by CodeRabbit
Summary of changes
New Features
Bug Fixes
Tests