make client in migration tests mandatory#4994
Conversation
|
Warning Review limit reached
More reviews will be available in 3 minutes and 7 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (55)
📝 WalkthroughWalkthroughThis PR updates the ChangesMigration client requirement propagation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes are highly repetitive and follow a consistent mechanical pattern: add Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4994 +/- ##
==========================================
- Coverage 98.67% 98.65% -0.02%
==========================================
Files 25 25
Lines 2487 2459 -28
==========================================
- Hits 2454 2426 -28
Misses 33 33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
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
added
size/L
needs-rebase
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot
requested review from
Anatw,
SamAlber,
azhivovk,
dalia-frank,
dshchedr,
geetikakay,
hmeir,
josemacassan,
kshvaika,
mijankow,
orelmisan,
rlobillo,
rnetser and
servolkov
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4999. Overlapping filestests/network/l2_bridge/vmi_interfaces_stability/test_interfaces_stability.py |
vsibirsk
force-pushed
the
vk-migration-client
branch
from
7376869 to
7e04694
Compare
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
- modified migrate_vm_and_verify func client arg to be mandatory - added type hints for affected tests/fixtures/functions - moved setup fixtures to @pytest.mark.usefixtures Signed-off-by: vsibirsk <vsibirsk@redhat.com>
vsibirsk
force-pushed
the
vk-migration-client
branch
from
7e04694 to
0efdd7b
Compare
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
/approve |
|
/lgtm |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4770. Overlapping filestests/network/flat_overlay/conftest.py |
| from utilities.network import assert_ping_successful, network_nad | ||
| from utilities.virt import migrate_vm_and_verify | ||
|
|
||
| if TYPE_CHECKING: |
There was a problem hiding this comment.
why TYPE_CHECKING is needed here?
There was a problem hiding this comment.
because DynamicClient and VirtualMachineForTests are used only for typehints (no need to import them in runtime)
|
|
||
|
|
||
| def migrate_validate_run_strategy_vm(vm, run_strategy): | ||
| def migrate_validate_run_strategy_vm(vm, client, run_strategy): |
There was a problem hiding this comment.
nit
It was modified to add client but none of the parameters have type hints. Every other modified function/fixture in this PR adds type hints consistently — this one was missed.
There was a problem hiding this comment.
good catch. modified only those places that coderabbit listed. will go through all modified places and update typehints
| def migrated_vm_with_policy(migration_policy_with_bandwidth, vm_for_migration_progress_test): | ||
| migrate_vm_and_verify(vm=vm_for_migration_progress_test, wait_for_migration_success=False) | ||
| def migrated_vm_with_policy( | ||
| admin_client: DynamicClient, vm_for_migration_progress_test: VirtualMachineForTests |
There was a problem hiding this comment.
why migration_policy_with_bandwidth was moved out? I think the dependency was right.
There was a problem hiding this comment.
if fixture is used only as setup (it's value not used in test/fixture), then it should be called via @pytest.mark.usefixtures
There was a problem hiding this comment.
if fixture is used only as setup (it's value not used in test/fixture), then it should be called via @pytest.mark.usefixtures
I agree if we talk about tests, but it is completely wrong if one fixture depends on another. When a fixture depends on another fixture purely for its side effects, listing it as an argument is the only and proper way to express that dependency. And it doesn't matter if that fixture returns or doesn't return a result.
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4954. Overlapping filestests/network/user_defined_network/test_user_defined_network_passt.py |
9 participants
What this PR does / why we need it:
modified
migrate_vm_and_verifyfunc client arg to be mandatoryand update relevant calls in functions and tests
Which issue(s) this PR fixes:
Aligns migration tests with default client deprecation in python-wrapper repo
https://redhat.atlassian.net/browse/CNV-68519
Special notes for reviewer:
jira-ticket:
https://redhat.atlassian.net/browse/CNV-88319
Summary by CodeRabbit
Tests
Refactor