Copilot commented on code in PR #12651:
URL: https://github.com/apache/cloudstack/pull/12651#discussion_r2812984646
##########
plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtGetVmIpAddressCommandWrapperTest.java:
##########
@@ -52,6 +52,12 @@ public class LibvirtGetVmIpAddressCommandWrapperTest {
" net4 2e:9b:60:dc:49:30 N/A N/A\n" + //
" lxc5b7327203b6f 92:b2:77:0b:a9:20 N/A N/A\n";
+ private static String VIRSH_DOMIF_OUTPUT_WINDOWS = " Name MAC
address Protocol Address\n" + //
+
"-------------------------------------------------------------------------------\n"
+ //
+ " Ethernet Instance 0 02:0c:02:f9:00:80 ipv4
192.168.0.10/24\n" + //
+ " Loopback Pseudo-Interface 1 ipv6 ::1/128\n" + //
Review Comment:
The test data for line 58 appears to be missing the MAC address column.
Based on the virsh domifaddr output format, each line should have 4 columns:
Name, MAC address, Protocol, and Address. Line 58 "Loopback Pseudo-Interface 1"
only has 3 columns (Name, Protocol, Address), which will cause the parsing
logic to incorrectly treat "1" (part of the interface name) as the MAC address
when using parts[parts.length - 3]. This should either include a MAC address or
use "-" as a placeholder to maintain the expected column structure, similar to
line 59.
```suggestion
" Loopback Pseudo-Interface 1 - ipv6 ::1/128\n" +
//
```
##########
plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtGetVmIpAddressCommandWrapperTest.java:
##########
@@ -118,7 +124,34 @@ public void testExecuteWithWindowsVm() {
when(getVmIpAddressCommand.getVmNetworkCidr()).thenReturn("192.168.0.0/24");
when(getVmIpAddressCommand.getMacAddress()).thenReturn("02:0c:02:f9:00:80");
when(getVmIpAddressCommand.isWindows()).thenReturn(true);
- when(Script.executePipedCommands(anyList(),
anyLong())).thenReturn(new Pair<>(0, "192.168.0.10"));
+ when(Script.executePipedCommands(anyList(),
anyLong())).thenReturn(new Pair<>(0, VIRSH_DOMIF_OUTPUT_WINDOWS));
+
+ Answer answer = commandWrapper.execute(getVmIpAddressCommand,
libvirtComputingResource);
+
+ assertTrue(answer.getResult());
+ assertEquals("192.168.0.10", answer.getDetails());
+ } finally {
+ if (scriptMock != null)
+ scriptMock.close();
+ }
+ }
+
+
+ @Test
+ public void testExecuteWithWindowsVm2() {
Review Comment:
The test name "testExecuteWithWindowsVm2" is not descriptive. Consider
renaming it to something more meaningful like
"testExecuteWithWindowsVmFallbackToRegistry" to clarify that this test verifies
the fallback behavior when virsh domifaddr returns empty output and the system
falls back to reading from the Windows registry.
```suggestion
public void testExecuteWithWindowsVmFallbackToRegistry() {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
