fix: copy cpu_flags to avoid mutating session-scoped fixture dict#4746
fix: copy cpu_flags to avoid mutating session-scoped fixture dict#4746RoniKishner wants to merge 1 commit intoRedHatQE:mainfrom
Conversation
📝 WalkthroughWalkthroughThis PR updates ChangesVirt utilities modernization and defensive-copy
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4746 +/- ##
==========================================
+ Coverage 98.63% 98.67% +0.03%
==========================================
Files 25 25
Lines 2420 2483 +63
==========================================
+ Hits 2387 2450 +63
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:
|
![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 `@utilities/virt.py`:
- Line 3: Replace the module-level import "import copy" with a specific import
"from copy import deepcopy" to match project guidelines, and update all usages
of copy.deepcopy in this file (e.g., any calls within functions or methods that
reference copy.deepcopy) to use deepcopy directly; ensure no other references to
the copy module remain and run tests/lint to confirm compliance.
🪄 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: 3bdd23f5-24aa-49fa-b92a-63e69e796fa0
📒 Files selected for processing (1)
utilities/virt.py
openshift-virtualization-qe-bot
added
changes-requested-coderabbitai[bot]
tox:verify-tc-requirement-polarion:running
Signed-off-by: rkishner <rkishner@redhat.com>
openshift-virtualization-qe-bot-3
added
lgtm-coderabbitai[bot]
and removed
changes-requested-coderabbitai[bot]
labels
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
utilities/virt.py (1)
1-1:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH: Restore required PR template sections before merge
The provided PR description context is missing the required template headings, including meaningful content for
##### What this PR does / why we need it:. These sections are required for review/traceability automation and release hygiene.As per coding guidelines: "
##### What this PR does / why we need it:— MUST be present AND have meaningful content" and required sections must includeWhich issue(s) this PR fixes,Special notes for reviewer, andjira-ticket.🤖 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 `@utilities/virt.py` at line 1, The PR description is missing required template sections; restore the required headings and meaningful content (at minimum add "##### What this PR does / why we need it:", list "Which issue(s) this PR fixes", "Special notes for reviewer", and "jira-ticket") in the PR description before merging; reference the module context where you changed the top of the file (the file that begins with "from __future__ import annotations") so reviewers can confirm the commit/PR text is updated and include clear, concise answers for each required field.
🤖 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.
Outside diff comments:
In `@utilities/virt.py`:
- Line 1: The PR description is missing required template sections; restore the
required headings and meaningful content (at minimum add "##### What this PR
does / why we need it:", list "Which issue(s) this PR fixes", "Special notes for
reviewer", and "jira-ticket") in the PR description before merging; reference
the module context where you changed the top of the file (the file that begins
with "from __future__ import annotations") so reviewers can confirm the
commit/PR text is updated and include clear, concise answers for each required
field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 930a9414-6af6-4e3b-b6d5-90b3f2e84cd9
📒 Files selected for processing (1)
utilities/virt.py
|
/retest build-container |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
/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:
Your deliverable: Test Execution Plan
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: Before setting "Run smoke tests: True", you MUST use shell scripts to: |
| import shlex | ||
| from collections import defaultdict | ||
| from contextlib import contextmanager | ||
| from copy import deepcopy |
There was a problem hiding this comment.
Test Execution Plan
- Run smoke tests: False (No concrete dependency path found from any smoke test to the changed
cpu_flags/nfixture handling; smoke test filestest_container_disk_vm.pyandtest_rhel_os_support.pydo not usecpu_flagsorvm_n.)
Directly affected tests (must run):
tests/virt/node/general/test_wsl2.py::TestWSL2— regression target; usesvm_cpu_flags→VirtualMachineForTests(cpu_flags=...)→ the now-deepcopiedself.cpu_flagstests/virt/node/general/test_oom.py::test_vm_windows_oom— mutation source; setscpu_cores=16, cpu_threads=1which previously polluted the session-scoped fixture
Secondary (use n/cpu_flags param on VirtualMachineForTests):
tests/virt/node/node_labeller/cpu_features/test_vm_with_cpu_flag.pytests/virt/node/node_labeller/cpu_features/test_vm_with_cpu_features.py
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1912 🏁 Script executed: Length of output: 10306 🏁 Script executed: Length of output: 2519 🏁 Script executed: Length of output: 1735 |
|
/approve |
5 participants
Short description:
Add
copy.deepcopywhencpu_flagsis used to create aVirtualMachineForTestsMore details:
The current approach copies the object and makes changes to it. this causes pollution as the object is referenced in a
sessionscope fixture, thus the next time the fixture is used it gets the changed valuesWhat this PR does / why we need it:
When
test_vm_windows_oomis run it changescpu_flags = {cpu_cores: 16, cpu_threads:1}. after whichTestWSL2is run with the new values since the fixture is referencing the object and is scopedsession, causing a failure due to instance type conflict.Special notes for reviewer:
Additional changes were done to the virt.py file due to
ruffcheck in pre-commitSummary by CodeRabbit
Bug Fixes
Refactor