Copilot commented on code in PR #13353:
URL: https://github.com/apache/cloudstack/pull/13353#discussion_r3528326199
##########
plugins/storage/volume/scaleio/src/test/java/org/apache/cloudstack/storage/datastore/lifecycle/ScaleIOPrimaryDataStoreLifeCycleTest.java:
##########
@@ -129,9 +127,8 @@ public void tearDown() throws Exception {
}
@Test
- public void testAttachZone() throws Exception {
+ public void testAttachZone() {
final DataStore dataStore = mock(DataStore.class);
- when(dataStore.getId()).thenReturn(1L);
MockedStatic<ScaleIOGatewayClientConnectionPool>
scaleIOGatewayClientConnectionPoolMocked =
mockStatic(ScaleIOGatewayClientConnectionPool.class);
Review Comment:
testAttachZone no longer stubs the
StorageManagerImpl.connectHostToSharedPool dependencies. With the new batching
in StorageManagerImpl.connectHostsToPool, the test will call dataStore.getId()
and dataStoreMgr.getDataStore(..., Primary) and then dereference
pool.isShared()/pool.getStorageProviderName(); without stubbing these, the test
will fail (NPE/assertion) rather than exercising the attachZone logic.
##########
plugins/storage/volume/scaleio/src/test/java/org/apache/cloudstack/storage/datastore/lifecycle/ScaleIOPrimaryDataStoreLifeCycleTest.java:
##########
@@ -152,12 +149,6 @@ public void testAttachZone() throws Exception {
when(resourceManager.getEligibleUpAndEnabledHostsInZoneForStorageConnection(dataStore,
scope.getScopeId(), Hypervisor.HypervisorType.KVM))
.thenReturn(Arrays.asList(host1, host2));
Review Comment:
StorageManagerImpl.connectHostsToPool now batches hosts via
hostDao.listByIds(...) and iterates the result immediately. In this test,
hostDao.listByIds(...) is not stubbed, so Mockito will return null and the
production code will throw a NullPointerException when building the hostMap.
##########
engine/schema/src/main/java/com/cloud/vm/dao/VMInstanceDaoImpl.java:
##########
@@ -1352,4 +1352,17 @@ public List<Long> listIdsByHostIdForVolumeStats(long
hostId) {
sc.setParameters("lastHost", hostId);
return customSearch(sc, null);
}
+
+ @Override
+ public List<VMInstanceVO> listByIds(List<Long> ids) {
+ if (ids == null || ids.isEmpty()) {
+ return new ArrayList<>();
+ }
Review Comment:
VMInstanceDaoImpl.listByIds allocates a new ArrayList for empty/null inputs.
Most other *DaoImpl.listByIds implementations in this codebase use
CollectionUtils.isEmpty(...) and return Collections.emptyList() to avoid
allocations and keep behavior consistent.
--
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]
