Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -196,10 +196,16 @@ public StrategyPriority canHandle(DataObject srcData, DataObject destData) {
}

private boolean isVolumeOnManagedStorage(VolumeInfo volumeInfo) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about a unit test here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DaanHoogland is planning on writing a unit test for the higher-level canHandle method (to add to this PR).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I am. I am struggling with the large number of injects but I think the canHandle() should be tested indeed, as the error was because the strategy returned a wrong priority from that call.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is why we should unit test methods instead. We need to reduce the test code. Otherwise it becomes almost impossible to maintain them.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, I'm missing your point

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mike-tutkowski are you going to add some unit test(s)? this is ready for merging otherwise.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rhtyd I believe @rafaelweingartner noted (perhaps on the mailing list) that he would like to add one unit test to this PR before it is merged. Thanks

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I think @DaanHoogland was planning on adding a unit test, as well.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mike-tutkowski Daan has already sent a PR to add unit test to your branch, can you merge that. See mike-tutkowski#6

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mike-tutkowski the PR I said I wanted to create some unit tests is not this one, it is the other one I worked.

long storagePooldId = volumeInfo.getDataStore().getId();
StoragePoolVO storagePoolVO = _storagePoolDao.findById(storagePooldId);
DataStore dataStore = volumeInfo.getDataStore();

return storagePoolVO.isManaged();
if (dataStore.getRole() == DataStoreRole.Primary) {
long storagePooldId = dataStore.getId();
StoragePoolVO storagePoolVO = _storagePoolDao.findById(storagePooldId);

return storagePoolVO.isManaged();
}

return false;
}

// canHandle returns true if the storage driver for the DataObject that's passed in can support certain features (what features we
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.cloudstack.storage.motion;

import com.cloud.storage.DataStoreRole;
import com.cloud.storage.ImageStore;
import org.apache.cloudstack.engine.subsystem.api.storage.DataMotionStrategy;
import org.apache.cloudstack.engine.subsystem.api.storage.DataObject;
import org.apache.cloudstack.engine.subsystem.api.storage.DataStore;
import org.apache.cloudstack.engine.subsystem.api.storage.PrimaryDataStore;
import org.apache.cloudstack.engine.subsystem.api.storage.StrategyPriority;
import org.apache.cloudstack.storage.datastore.PrimaryDataStoreImpl;
import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
import org.apache.cloudstack.storage.image.store.ImageStoreImpl;
import org.apache.cloudstack.storage.volume.VolumeObject;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.runners.MockitoJUnitRunner;

import static org.junit.Assert.assertTrue;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.mock;
import static org.mockito.MockitoAnnotations.initMocks;

@RunWith(MockitoJUnitRunner.class)
public class StorageSystemDataMotionStrategyTest {

@Mock
VolumeObject source;
@Mock
DataObject destination;
@Mock
PrimaryDataStore sourceStore;
@Mock
ImageStore destinationStore;

@InjectMocks
DataMotionStrategy strategy = new StorageSystemDataMotionStrategy();
@Mock
PrimaryDataStoreDao _storagePoolDao;

@Before public void setUp() throws Exception {
sourceStore = mock(PrimaryDataStoreImpl.class);
destinationStore = mock(ImageStoreImpl.class);
source = mock(VolumeObject.class);
destination = mock(VolumeObject.class);

initMocks(strategy);
}

@Test
public void cantHandleSecondary() {
doReturn(sourceStore).when(source).getDataStore();
doReturn(DataStoreRole.Primary).when(sourceStore).getRole();
doReturn(destinationStore).when(destination).getDataStore();
doReturn(DataStoreRole.Image).when((DataStore)destinationStore).getRole();
doReturn(sourceStore).when(source).getDataStore();
doReturn(destinationStore).when(destination).getDataStore();
StoragePoolVO storeVO = new StoragePoolVO();
doReturn(storeVO).when(_storagePoolDao).findById(0l);

assertTrue(strategy.canHandle(source,destination) == StrategyPriority.CANT_HANDLE);
}
}