Add support for multiarch dedicated tests#5124
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughThe PR adds collection-time multiarch test filtering, makes CPU-architecture handling optional in fixtures and helpers, updates several tests to avoid missing-key failures with ChangesMultiarch collection and marker wiring
Architecture-aware fixtures and safe parametrization
Documentation and CI coverage
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
openshift-virtualization-qe-bot-2
requested review from
Ahmad-Hafe,
RoniKishner,
SamAlber,
dshchedr,
ema-aka-young,
geetikakay,
josemacassan,
jpeimer,
kgoldbla,
kshvaika,
rnetser,
sarahbx and
vsibirsk
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5124 +/- ##
=======================================
Coverage 98.67% 98.68%
=======================================
Files 25 42 +17
Lines 2487 2501 +14
=======================================
+ Hits 2454 2468 +14
Misses 33 33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@utilities/pytest_utils.py`:
- Around line 624-633: Add a Google-style docstring to the public function
remove_tests_from_list explaining its purpose (filtering pytest.Item objects by
a keyword), describing parameters (items: list[pytest.Item] and filter_str: str)
and the return value (tuple of discard_tests and items_to_return as lists of
pytest.Item), and include any important behavior details such as that membership
is checked via "filter_str in item.keywords" and the ordering of returned lists
mirrors input order; place the docstring immediately above the def
remove_tests_from_list(...) declaration.
- Around line 635-642: Add a Google-style docstring to the public function
filter_multiarch_tests explaining its purpose (filters out or returns multiarch
tests depending on cluster type), its parameters (items: list[pytest.Item],
config: pytest.Config), and its return value (list[pytest.Item]); mention
behavior when py_config.get("cluster_type") == MULTIARCH and that it calls
remove_tests_from_list and may invoke config.hook.pytest_deselected with
discarded tests. Ensure the docstring is placed immediately below the def line
and follows Google docstring sections: Args and Returns.
🪄 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: 9a47eda7-a39f-430d-9af5-5e47da246cc7
📒 Files selected for processing (9)
conftest.pytests/chaos/oadp/test_oadp.pytests/conftest.pytests/storage/cdi_upload/test_upload.pytests/storage/cdi_upload/test_upload_virtctl.pytests/storage/golden_image/test_golden_image.pytests/storage/test_disk_preallocation.pytests/virt/node/high_performance_vm/test_high_performance_templates.pyutilities/pytest_utils.py
|
/verified |
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.
Since that comment, the activity in this thread ( If there have been actual code changes to this PR since that comment (beyond rebases/retests), let me know or push a new commit and I'll regenerate the plan against the latest diff. Also, a quick note unrelated to testing: [skip_review_follow_up_policy] |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #5440. Overlapping filesconftest.py |
Currently, only one cpu arch can be passed without failing. for multiarch tests, we need to iterate over cluster archs. Signed-off-by: Harel Meir <hmeir@redhat.com>
Collection fails because of re-import. Transitioning to get method Signed-off-by: Harel Meir <hmeir@redhat.com>
Signed-off-by: Harel Meir <hmeir@redhat.com>
Signed-off-by: Harel Meir <hmeir@redhat.com>
Signed-off-by: Harel Meir <hmeir@redhat.com>
Signed-off-by: Harel Meir <hmeir@redhat.com>
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 `@utilities/pytest_utils.py`:
- Around line 652-673: The multiarch validation path in filter_multiarch_tests
is currently unreachable because pytest_collection_modifyitems removes multiarch
items before validate_collected_tests_arch_params runs. Either move the
cluster_type check earlier so homogeneous clusters are validated before
deselection, or remove the dead py_config["cluster_type"] != MULTIARCH branch
from validate_collected_tests_arch_params and keep the behavior limited to
filtering in filter_multiarch_tests.
🪄 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: 28fd7f71-5b57-4888-a224-ffa73c00f0f7
📒 Files selected for processing (14)
conftest.pydocs/MULTIARCH.mdtests/chaos/oadp/test_oadp.pytests/conftest.pytests/install_upgrade_operators/hco_enablement_golden_image_updates/multiarch/test_multiarch_golden_images_support.pytests/storage/cdi_upload/test_upload.pytests/storage/cdi_upload/test_upload_virtctl.pytests/storage/golden_image/test_golden_image.pytests/virt/node/high_performance_vm/test_high_performance_templates.pytox.iniutilities/architecture.pyutilities/pytest_utils.pyutilities/unittests/test_architecture.pyutilities/unittests/test_pytest_utils.py
|
/verified |
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
set -euo pipefail
gh pr view 5124 --repo RedHatQE/openshift-virtualization-tests --json headRefOid,updatedAt,commits --jq '{head: .headRefOid, updated: .updatedAt, lastCommits: [.commits[-3:][] | {oid: .oid, messageHeadline: .messageHeadline, committedDate: .committedDate}]}'Length of output: 941
I won't repost a duplicate inline comment. If a future push introduces new functional changes to this PR (beyond doc/tox/test additions), ping me again and I'll regenerate the plan against that diff. |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #5443. Overlapping filestests/conftest.py |
|
/lgtm |
|
/approve |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:latest published |
|
/cherry-pick cnv-4.22 |
|
Manual cherry-pick is needed |
11 participants
What this PR does / why we need it:
Adds infrastructure support for running tests on heterogeneous (multi-architecture) clusters with both amd64 and arm64 worker nodes. Three run configurations are now supported:
Which issue(s) this PR fixes:
Special notes for reviewer:
The .get() calls on parametrize decorator values are intentional — because collection with cpu_arch > 1 failes because of import order.
jira-ticket:
https://redhat.atlassian.net/browse/CNV-90470
Summary by CodeRabbit
pytest-check-multiarchtox environment.KeyErrorfailures by safely handling missing image/OS and architecture values during test setup.