Improve test cases for update_device to create guest XML by the framework#6822
Improve test cases for update_device to create guest XML by the framework#6822yiqianwei wants to merge 1 commit intoautotest:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughMultiple virtual_network update_device test configs were modified to restrict execution to Linux and add libvirt VM lifecycle flags: create_vm_libvirt = yes, use_no_reboot = yes, kill_vm_libvirt = yes, and ovmf.kill_vm_libvirt_options = --nvram. One config removed an extra_attrs default and its usage. Test code now accepts a netdst parameter to derive bridge names when provided, calls network_base.cancel_if_ovs_bridge and network_base.setup_ovs_bridge_attrs to detect/handle OVS, and no longer creates or deletes Linux/OVS bridges inside the tests; interface update and VM XML logic remain. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip Migrating from UI to YAML configuration.Use the |
![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 the current code and only fix it if needed.
Inline comments:
In `@libvirt/tests/cfg/virtual_network/update_device/update_device_coalesce.cfg`:
- Line 39: The dict assigned to iface_attrs ends with an extra closing brace
causing a syntax error after variable substitution; edit the expression that
constructs iface_attrs (the line containing iface_attrs = {'type_name':
'${iface_type}', **${coalesce}, **${iface_source}, 'model': 'virtio',
'mac_address': mac}}) and remove the trailing extra `}` so the dict ends with a
single `}`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a1366821-aefd-4bee-a059-e18fa82dc52b
📒 Files selected for processing (8)
libvirt/tests/cfg/virtual_network/update_device/update_device_coalesce.cfglibvirt/tests/cfg/virtual_network/update_device/update_iface_link_state.cfglibvirt/tests/cfg/virtual_network/update_device/update_iface_source.cfglibvirt/tests/cfg/virtual_network/update_device/update_iface_with_identifier.cfglibvirt/tests/cfg/virtual_network/update_device/update_iface_with_options_active.cfglibvirt/tests/cfg/virtual_network/update_device/update_iface_with_options_inactive.cfglibvirt/tests/cfg/virtual_network/update_device/update_iface_with_unchangable.cfglibvirt/tests/src/virtual_network/update_device/update_device_coalesce.py
libvirt/tests/cfg/virtual_network/update_device/update_device_coalesce.cfg
Outdated
Show resolved
Hide resolved
yiqianwei
force-pushed
the
LIBVIRTAT-22356
branch
from
1ac8fdc to
a496f2b
Compare
|
Test results: update_iface_with_unchangable.cfg: update_iface_with_options_inactive.cfg: update_iface_with_options_active.cfg: update_iface_with_identifier.cfg: |
yiqianwei
force-pushed
the
LIBVIRTAT-22356
branch
from
a496f2b to
8819ec4
Compare
|
Test results: For ovs bridge: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libvirt/tests/src/virtual_network/update_device/update_iface_link_state.py`:
- Around line 29-30: The bridge name uses the raw params.get('netdst') value
which can be comma-delimited; normalize netdst before using it by splitting on
the first comma and stripping whitespace, e.g. compute normalized_netdst =
(params.get('netdst') or '').split(',', 1)[0].strip() and then set bridge_name =
normalized_netdst if normalized_netdst else "br_" + rand_id; update the
assignment sites that reference netdst/bridge_name (the variables netdst and
bridge_name in this module) so only the first token is used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ac0b0140-332c-4708-bcef-59cf323aa393
📒 Files selected for processing (9)
libvirt/tests/cfg/virtual_network/update_device/update_device_coalesce.cfglibvirt/tests/cfg/virtual_network/update_device/update_iface_link_state.cfglibvirt/tests/cfg/virtual_network/update_device/update_iface_source.cfglibvirt/tests/cfg/virtual_network/update_device/update_iface_with_identifier.cfglibvirt/tests/cfg/virtual_network/update_device/update_iface_with_options_active.cfglibvirt/tests/cfg/virtual_network/update_device/update_iface_with_options_inactive.cfglibvirt/tests/cfg/virtual_network/update_device/update_iface_with_unchangable.cfglibvirt/tests/src/virtual_network/update_device/update_device_coalesce.pylibvirt/tests/src/virtual_network/update_device/update_iface_link_state.py
🚧 Files skipped from review as they are similar to previous changes (6)
- libvirt/tests/cfg/virtual_network/update_device/update_iface_with_options_inactive.cfg
- libvirt/tests/cfg/virtual_network/update_device/update_iface_with_identifier.cfg
- libvirt/tests/cfg/virtual_network/update_device/update_iface_with_options_active.cfg
- libvirt/tests/cfg/virtual_network/update_device/update_iface_link_state.cfg
- libvirt/tests/cfg/virtual_network/update_device/update_iface_source.cfg
- libvirt/tests/cfg/virtual_network/update_device/update_iface_with_unchangable.cfg
libvirt/tests/src/virtual_network/update_device/update_iface_link_state.py
Outdated
Show resolved
Hide resolved
|
Test results: |
yiqianwei
force-pushed
the
LIBVIRTAT-22356
branch
from
8819ec4 to
79105b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libvirt/tests/src/virtual_network/update_device/update_device_coalesce.py`:
- Around line 55-62: The current conditional uses iface_type == 'network' which
incorrectly treats the nat_net variant as Linux-bridge-only; change the logic so
only bridge-specific variants (i.e., when br_type == 'linux_br' or br_type ==
'ovs_br' checks) trigger network_base.cancel_if_ovs_bridge(params, test) and
network_base.setup_ovs_bridge_attrs(params, iface_attrs); specifically, remove
or narrow the iface_type == 'network' branch and ensure nat_net
(source={'network': 'default'}) is not rerouted through the OVS setup
path—update the conditional around br_type and iface_type where
network_base.cancel_if_ovs_bridge and network_base.setup_ovs_bridge_attrs are
called so that nat_net runs on both host types.
In `@libvirt/tests/src/virtual_network/update_device/update_iface_link_state.py`:
- Around line 28-29: The code currently uses params.get('netdst', 'virbr0')
which masks an absent netdst and makes the later guards ineffective; change the
params lookup to keep netdst empty when absent (e.g., netdst =
params.get('netdst') or similar) and only normalize/split it when it is present
so bridge_name falls back to "br_" + rand_id when netdst is falsy; update the
assignment of bridge_name (and any other places using netdst) to use the
normalized value only if netdst is truthy, ensuring the existing if not netdst
checks around bridge creation and deletion (references: netdst, bridge_name,
rand_id) will execute correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bc23729e-cf6c-4845-81f6-7486fc0dd9e9
📒 Files selected for processing (9)
libvirt/tests/cfg/virtual_network/update_device/update_device_coalesce.cfglibvirt/tests/cfg/virtual_network/update_device/update_iface_link_state.cfglibvirt/tests/cfg/virtual_network/update_device/update_iface_source.cfglibvirt/tests/cfg/virtual_network/update_device/update_iface_with_identifier.cfglibvirt/tests/cfg/virtual_network/update_device/update_iface_with_options_active.cfglibvirt/tests/cfg/virtual_network/update_device/update_iface_with_options_inactive.cfglibvirt/tests/cfg/virtual_network/update_device/update_iface_with_unchangable.cfglibvirt/tests/src/virtual_network/update_device/update_device_coalesce.pylibvirt/tests/src/virtual_network/update_device/update_iface_link_state.py
🚧 Files skipped from review as they are similar to previous changes (4)
- libvirt/tests/cfg/virtual_network/update_device/update_iface_with_unchangable.cfg
- libvirt/tests/cfg/virtual_network/update_device/update_iface_with_options_active.cfg
- libvirt/tests/cfg/virtual_network/update_device/update_iface_with_identifier.cfg
- libvirt/tests/cfg/virtual_network/update_device/update_iface_link_state.cfg
libvirt/tests/src/virtual_network/update_device/update_device_coalesce.py
Outdated
Show resolved
Hide resolved
libvirt/tests/src/virtual_network/update_device/update_iface_link_state.py
Outdated
Show resolved
Hide resolved
yiqianwei
force-pushed
the
LIBVIRTAT-22356
branch
from
79105b9 to
e37afc0
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
libvirt/tests/src/virtual_network/update_device/update_device_coalesce.py (1)
55-62:⚠️ Potential issue | 🟠 MajorNarrow the bridge-type cancel logic to avoid canceling
nat_neton OVS hosts.At Line 55,
iface_type == 'network'forces nat_net variants into Linux-bridge-only cancellation, which matches the observed 6/9 CANCEL pattern on OVS hosts and drops coverage.Proposed fix
- if br_type == 'linux_br' or iface_type == 'network': + if br_type == 'linux_br': network_base.cancel_if_ovs_bridge(params, test) elif br_type == 'ovs_br': br_backend = utils_net.find_bridge_manager(br_name) if isinstance(br_backend, utils_net.Bridge): test.cancel("Skip OVS bridge test as the host uses Linux bridge") - # Setup OVS bridge configuration if needed - network_base.setup_ovs_bridge_attrs(params, iface_attrs) + # Setup OVS bridge configuration only for ovs_br variants + network_base.setup_ovs_bridge_attrs(params, iface_attrs)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libvirt/tests/src/virtual_network/update_device/update_device_coalesce.py` around lines 55 - 62, The current conditional (br_type == 'linux_br' or iface_type == 'network') is too broad and cancels nat_net variants on OVS hosts; narrow it so nat_net variants are not treated as Linux-bridge-only. Update the if to only call network_base.cancel_if_ovs_bridge when br_type == 'linux_br' or (iface_type == 'network' and the params/iface_attrs do NOT indicate a nat_net variant) — e.g. check params or iface_attrs for the nat_net marker (like a 'nat_net' flag or 'variant' == 'nat_net') before calling network_base.cancel_if_ovs_bridge; keep the rest of the flow (br_backend check using utils_net.find_bridge_manager, utils_net.Bridge and network_base.setup_ovs_bridge_attrs) intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@libvirt/tests/src/virtual_network/update_device/update_device_coalesce.py`:
- Around line 55-62: The current conditional (br_type == 'linux_br' or
iface_type == 'network') is too broad and cancels nat_net variants on OVS hosts;
narrow it so nat_net variants are not treated as Linux-bridge-only. Update the
if to only call network_base.cancel_if_ovs_bridge when br_type == 'linux_br' or
(iface_type == 'network' and the params/iface_attrs do NOT indicate a nat_net
variant) — e.g. check params or iface_attrs for the nat_net marker (like a
'nat_net' flag or 'variant' == 'nat_net') before calling
network_base.cancel_if_ovs_bridge; keep the rest of the flow (br_backend check
using utils_net.find_bridge_manager, utils_net.Bridge and
network_base.setup_ovs_bridge_attrs) intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0c44690f-b5a2-4c4f-b990-c1b55ff23efb
📒 Files selected for processing (9)
libvirt/tests/cfg/virtual_network/update_device/update_device_coalesce.cfglibvirt/tests/cfg/virtual_network/update_device/update_iface_link_state.cfglibvirt/tests/cfg/virtual_network/update_device/update_iface_source.cfglibvirt/tests/cfg/virtual_network/update_device/update_iface_with_identifier.cfglibvirt/tests/cfg/virtual_network/update_device/update_iface_with_options_active.cfglibvirt/tests/cfg/virtual_network/update_device/update_iface_with_options_inactive.cfglibvirt/tests/cfg/virtual_network/update_device/update_iface_with_unchangable.cfglibvirt/tests/src/virtual_network/update_device/update_device_coalesce.pylibvirt/tests/src/virtual_network/update_device/update_iface_link_state.py
🚧 Files skipped from review as they are similar to previous changes (5)
- libvirt/tests/cfg/virtual_network/update_device/update_iface_with_options_inactive.cfg
- libvirt/tests/cfg/virtual_network/update_device/update_iface_link_state.cfg
- libvirt/tests/cfg/virtual_network/update_device/update_iface_with_unchangable.cfg
- libvirt/tests/cfg/virtual_network/update_device/update_iface_source.cfg
- libvirt/tests/cfg/virtual_network/update_device/update_device_coalesce.cfg
…work 1.Add some parameters to allow the framework provide guest xml 2.Add "only Linux" to limit guest type 3.Remove create ovs and linux_br,let them created by the test loop's environment setup par ID: LIBVIRTAT-22356 Signed-off-by: Yiqian Wei <yiwei@redhat.com>
yiqianwei
force-pushed
the
LIBVIRTAT-22356
branch
from
e37afc0 to
3780ce2
Compare
1 participant
1.Add some parameters to allow the framework provide guest xml
2.Add "only Linux" to limit guest type
3.Remove create ovs and linux_br , let them created by the test loop's environment setup par
ID: LIBVIRTAT-22356
Summary by CodeRabbit
Tests
Refactor