Copilot commented on code in PR #14050:
URL: https://github.com/apache/cloudstack/pull/14050#discussion_r3968164172
##########
framework/config/src/main/java/org/apache/cloudstack/framework/config/impl/ConfigDepotImpl.java:
##########
@@ -172,7 +172,8 @@ private void createOrupdateConfigObject(Date date, String
componentName, ConfigK
Pair<String, Long> subGroup = key.subGroup();
ConfigurationSubGroupVO subGroupVO =
_configSubGroupDao.findByNameAndGroup(subGroup.first(), groupId);
if (subGroupVO == null) {
- subGroupVO = new ConfigurationSubGroupVO();
+ subGroupVO = new ConfigurationSubGroupVO(subGroup.first(),
null, subGroup.second());
Review Comment:
Passing a literal `null` into the `ConfigurationSubGroupVO` constructor is
ambiguous and reduces readability (it’s not obvious which field is
intentionally null). If there is an overload/builder/setter-based construction
available (e.g., construct with required fields only, then set optional fields
explicitly), prefer that. Otherwise, consider a short inline comment indicating
what the null represents to avoid future misuse.
##########
framework/config/src/test/java/org/apache/cloudstack/framework/config/impl/ConfigDepotImplTest.java:
##########
@@ -24,24 +24,54 @@
import org.apache.cloudstack.framework.config.ConfigKey;
import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
+import org.apache.cloudstack.framework.config.dao.ConfigurationSubGroupDao;
import org.junit.Assert;
import org.junit.Test;
import org.junit.runner.RunWith;
+import org.mockito.ArgumentCaptor;
import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.Mockito;
import org.mockito.junit.MockitoJUnitRunner;
import org.springframework.test.util.ReflectionTestUtils;
+import com.cloud.utils.Pair;
+
+import java.util.Date;
+
@RunWith(MockitoJUnitRunner.class)
public class ConfigDepotImplTest {
@Mock
ConfigurationDao _configDao;
+ @Mock
+ ConfigurationSubGroupDao _configSubGroupDao;
+
@InjectMocks
private ConfigDepotImpl configDepotImpl = new ConfigDepotImpl();
+ @Test
+ public void createConfigObjectPersistsSubGroupWithNameAndGroupId() {
+ ConfigKey<?> key = Mockito.mock(ConfigKey.class);
+ Mockito.when(key.group()).thenReturn(null);
+ Mockito.when(key.subGroup()).thenReturn(new Pair<>("ConsoleProxy VM",
5L));
+ Mockito.when(key.key()).thenReturn("consoleproxy.capacity.standby");
+ Mockito.when(key.scope()).thenReturn(ConfigKey.Scope.Global);
+ Mockito.when(_configSubGroupDao.findByNameAndGroup("ConsoleProxy VM",
1L)).thenReturn(null);
+
Mockito.when(_configSubGroupDao.persist(Mockito.any(ConfigurationSubGroupVO.class)))
+ .thenAnswer(invocation -> invocation.getArgument(0));
+
Mockito.when(_configDao.findById("consoleproxy.capacity.standby")).thenReturn(Mockito.mock(ConfigurationVO.class));
+
+ ArgumentCaptor<ConfigurationSubGroupVO> captor =
ArgumentCaptor.forClass(ConfigurationSubGroupVO.class);
+ ReflectionTestUtils.invokeMethod(configDepotImpl,
"createOrupdateConfigObject",
+ new Date(), "components", key, "someValue");
+
+ Mockito.verify(_configSubGroupDao).persist(captor.capture());
+ Assert.assertEquals("ConsoleProxy VM", captor.getValue().getName());
+ Assert.assertEquals(Long.valueOf(1L), captor.getValue().getGroupId());
Review Comment:
The test hard-codes the expected `groupId` as `1L`, which makes it brittle
if the internal group-id mapping changes (e.g., different fixture
initialization). Consider stubbing `findByNameAndGroup` with `anyLong()` and
capturing the actual groupId used (either capture the `long` argument passed to
`findByNameAndGroup`, or assert that `captor.getValue().getGroupId()` matches
the groupId argument used in the DAO lookup). This keeps the test focused on
'groupId is propagated consistently' rather than on a specific numeric id.
##########
framework/config/src/test/java/org/apache/cloudstack/framework/config/impl/ConfigDepotImplTest.java:
##########
@@ -24,24 +24,54 @@
import org.apache.cloudstack.framework.config.ConfigKey;
import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
+import org.apache.cloudstack.framework.config.dao.ConfigurationSubGroupDao;
import org.junit.Assert;
import org.junit.Test;
import org.junit.runner.RunWith;
+import org.mockito.ArgumentCaptor;
import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.Mockito;
import org.mockito.junit.MockitoJUnitRunner;
import org.springframework.test.util.ReflectionTestUtils;
+import com.cloud.utils.Pair;
+
+import java.util.Date;
+
@RunWith(MockitoJUnitRunner.class)
public class ConfigDepotImplTest {
@Mock
ConfigurationDao _configDao;
+ @Mock
+ ConfigurationSubGroupDao _configSubGroupDao;
+
@InjectMocks
private ConfigDepotImpl configDepotImpl = new ConfigDepotImpl();
+ @Test
+ public void createConfigObjectPersistsSubGroupWithNameAndGroupId() {
+ ConfigKey<?> key = Mockito.mock(ConfigKey.class);
+ Mockito.when(key.group()).thenReturn(null);
+ Mockito.when(key.subGroup()).thenReturn(new Pair<>("ConsoleProxy VM",
5L));
+ Mockito.when(key.key()).thenReturn("consoleproxy.capacity.standby");
+ Mockito.when(key.scope()).thenReturn(ConfigKey.Scope.Global);
+ Mockito.when(_configSubGroupDao.findByNameAndGroup("ConsoleProxy VM",
1L)).thenReturn(null);
+
Mockito.when(_configSubGroupDao.persist(Mockito.any(ConfigurationSubGroupVO.class)))
+ .thenAnswer(invocation -> invocation.getArgument(0));
+
Mockito.when(_configDao.findById("consoleproxy.capacity.standby")).thenReturn(Mockito.mock(ConfigurationVO.class));
+
+ ArgumentCaptor<ConfigurationSubGroupVO> captor =
ArgumentCaptor.forClass(ConfigurationSubGroupVO.class);
+ ReflectionTestUtils.invokeMethod(configDepotImpl,
"createOrupdateConfigObject",
+ new Date(), "components", key, "someValue");
+
+ Mockito.verify(_configSubGroupDao).persist(captor.capture());
+ Assert.assertEquals("ConsoleProxy VM", captor.getValue().getName());
+ Assert.assertEquals(Long.valueOf(1L), captor.getValue().getGroupId());
Review Comment:
The test hard-codes the expected `groupId` as `1L`, which makes it brittle
if the internal group-id mapping changes (e.g., different fixture
initialization). Consider stubbing `findByNameAndGroup` with `anyLong()` and
capturing the actual groupId used (either capture the `long` argument passed to
`findByNameAndGroup`, or assert that `captor.getValue().getGroupId()` matches
the groupId argument used in the DAO lookup). This keeps the test focused on
'groupId is propagated consistently' rather than on a specific numeric id.
--
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]
