Vered Volansky has posted comments on this change. Change subject: core: Consolidating DiskValidator's RO validations ......................................................................
Patch Set 2: (6 comments) Addressed all comments, will push shortly. http://gerrit.ovirt.org/#/c/27652/2/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddDiskToVmCommandTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddDiskToVmCommandTest.java: Line 482: return mockStorageDomain(storageId, 6, 4, storageType, version); Line 483: } Line 484: Line 485: private StorageDomain mockStorageDomain(Guid storageId, Version version) { Line 486: return mockStorageDomain(storageId, 6, 4, StorageType.UNKNOWN, version); > all of these mockStorageDomain methods that mock the free space should be r These methods are used in other tests I haven't touched. Can be done in a different patch to amend those tests as well. Should that be handled, so it the whole not-unit-test convention of said tests. Line 487: } Line 488: Line 489: private StorageDomain mockStorageDomain(Guid storageId, int availableSize, int usedSize, Line 490: StorageType storageType, Version version) { http://gerrit.ovirt.org/#/c/27652/2/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AttachDiskToVmCommandTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AttachDiskToVmCommandTest.java: Line 37: public class AttachDiskToVmCommandTest { Line 38: Line 39: private static Guid vmId = Guid.newGuid(); Line 40: private static Guid diskId = Guid.newGuid(); Line 41: private static Guid storageId = Guid.newGuid(); > All of these should be instance methods, and the initialization should be d Done Line 42: Line 43: @Mock Line 44: private DiskDao diskDao; Line 45: Line 96: VdcBllMessages.ACTION_TYPE_FAILED_INTERFACE_DOES_NOT_SUPPORT_READ_ONLY_ATTR); Line 97: verify(diskValidator).isReadOnlyPropertyCompatibleWithInterface(); Line 98: } Line 99: Line 100: private void initialSetup() { > This should be public and have a @Before annotation, not be called explicit Done Line 101: mockValidators(); Line 102: doNothing().when(command).updateDisksFromDb(); Line 103: doReturn(mockVm()).when(command).getVm(); Line 104: Line 111: doReturn(storagePoolIsoMapDAO).when(command).getStoragePoolIsoMapDao(); Line 112: mockStoragePoolIsoMap(); Line 113: } Line 114: Line 115: protected void initCommand() { > Make this public and add a @Before annotation instead of calling it explici Done Line 116: when(diskDao.get(diskId)).thenReturn(createDiskImage()); Line 117: AttachDettachVmDiskParameters parameters = createParameters(); Line 118: command = spy(new AttachDiskToVmCommand<AttachDettachVmDiskParameters>(parameters) { Line 119: // Overridden here and not during spying, since it's called in the constructor http://gerrit.ovirt.org/#/c/27652/2/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/DiskValidatorTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/DiskValidatorTest.java: Line 45: private DiskImage disk; Line 46: private LunDisk lunDisk; Line 47: private DiskValidator lunValidator; Line 48: Line 49: private static DiskImage createDisk() { > I'd rename this to createDiskImage() Done Line 50: DiskImage disk = new DiskImage(); Line 51: disk.setId(Guid.newGuid()); Line 52: return disk; Line 53: } http://gerrit.ovirt.org/#/c/27652/2/frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/AppErrors.java File frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/AppErrors.java: Line 3173: Line 3174: @DefaultStringValue("Cannot ${action} ${type}. Custom serial number must be non-empty when \"Custom\" serial number policy is specified.") Line 3175: String ACTION_TYPE_FAILED_INVALID_SERIAL_NUMBER(); Line 3176: Line 3177: @DefaultStringValue("Cannot ${action} ${type}. ${interface} disks can't be read-only") > Why have you removed the "." at the end? By mistake, will fix. Line 3178: String ACTION_TYPE_FAILED_INTERFACE_DOES_NOT_SUPPORT_READ_ONLY_ATTR(); -- To view, visit http://gerrit.ovirt.org/27652 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I292f729c981df6563ec65d684e1b2b5ae12283a0 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Vered Volansky <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Liron Ar <[email protected]> Gerrit-Reviewer: Tal Nisan <[email protected]> Gerrit-Reviewer: Vered Volansky <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
