Martin Mucha has posted comments on this change. Change subject: core: MacPool related Commands ......................................................................
Patch Set 30: (6 comments) http://gerrit.ovirt.org/#/c/28705/30/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateMacPoolCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateMacPoolCommand.java: Line 79: this.macPoolFromDb = macPoolFromDb; Line 80: this.newMacPool = newMacPool; Line 81: } Line 82: Line 83: public ValidationResult defaultFlagIsNotChanged() { > I think this method doesn't really require a separate class, it can be just ok, consequece of that is, that there's shouldn't be test for this method in class named UpdateMacPoolValidatorTest. I'm creating new UpdateMacPoolCommandTest, into where are tests moved. I turned this method into static one, for shorter code in tests. I think this method should not be static, so If you don't like it this way, I'll change it. Done Line 84: final boolean defaultChanged = macPoolFromDb.isDefaultPool() != newMacPool.isDefaultPool(); Line 85: return ValidationResult.failWith( Line 86: VdcBllMessages.ACTION_TYPE_FAILED_CHANGING_DEFAULT_MAC_POOL_IS_NOT_SUPPORTED) Line 87: .when(defaultChanged); http://gerrit.ovirt.org/#/c/28705/30/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/MacPoolValidatorTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/MacPoolValidatorTest.java: Line 41: Line 42: @Test Line 43: public void testDefaultPoolFlagIsNotSetValidUsage() throws Exception { Line 44: macPool.setDefaultPool(false); Line 45: Assert.assertThat(new MacPoolValidator(macPool).defaultPoolFlagIsNotSet(), > You should statically import Assert.assertThat Done Line 46: isValid()); Line 47: } Line 48: Line 49: @Test Line 130: public void testNotRemovingUsedPoolRecordIsUsed() throws Exception { Line 131: macPool.setId(Guid.newGuid()); Line 132: when(macPoolDaoMock.getDcUsageCount(eq(macPool.getId()))).thenReturn(1); Line 133: Line 134: final ValidationResult validationResult = new MacPoolValidator(macPool).notRemovingUsedPool(); > This can be inlined Done Line 135: Assert.assertThat(validationResult, Line 136: failsWith(VdcBllMessages.ACTION_TYPE_FAILED_CANNOT_REMOVE_STILL_USED_MAC_POOL)); Line 137: } Line 138: Line 140: public void testNotRemovingUsedPoolRecordNotUsed() throws Exception { Line 141: macPool.setId(Guid.newGuid()); Line 142: when(macPoolDaoMock.getDcUsageCount(eq(macPool.getId()))).thenReturn(0); Line 143: Line 144: final ValidationResult validationResult = new MacPoolValidator(macPool).notRemovingUsedPool(); > This can be inlined Done Line 145: Assert.assertThat(validationResult, isValid()); Line 146: } Line 147: Line 148: @Test http://gerrit.ovirt.org/#/c/28705/30/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/UpdateMacPoolValidatorTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/UpdateMacPoolValidatorTest.java: Line 23: final MacPool macPool1 = new MacPool(); Line 24: final MacPool macPool2 = new MacPool(); Line 25: macPool2.setDefaultPool(!macPool1.isDefaultPool()); Line 26: Line 27: Assert.assertThat(new UpdateMacPoolCommand.UpdateMacPoolValidator(macPool1, macPool2).defaultFlagIsNotChanged(), ValidationResultMatchers.failsWith(VdcBllMessages.ACTION_TYPE_FAILED_CHANGING_DEFAULT_MAC_POOL_IS_NOT_SUPPORTED)); > You should import Assert.assertThat and ValidationResultMatchers.failsWith Done Line 28: } Line 29: Line 30: @Test Line 31: public void testDefaultFlagIsNotChangedFlagNotChanged() throws Exception { http://gerrit.ovirt.org/#/c/28705/30/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/MacPool.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/MacPool.java: Line 13: public class MacPool extends IVdcQueryable implements BusinessEntity<Guid> { Line 14: Line 15: private static final long serialVersionUID = -7952435653821354188L; Line 16: Line 17: @NotNull(message = "ACTION_TYPE_FAILED_MAC_POOL_DOES_NOT_EXIST", > I wouldn't specify this message here, it would be confusing. Done Line 18: groups = { UpdateEntity.class }) Line 19: private Guid id; Line 20: Line 21: @NotEmpty(message = "ACTION_TYPE_FAILED_NAME_MAY_NOT_BE_EMPTY") -- To view, visit http://gerrit.ovirt.org/28705 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9c0c3657e3e53699bcafa375befdce848b01a2f3 Gerrit-PatchSet: 30 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <[email protected]> Gerrit-Reviewer: Lior Vernia <[email protected]> Gerrit-Reviewer: Martin Mucha <[email protected]> Gerrit-Reviewer: Mike Kolesnik <[email protected]> Gerrit-Reviewer: Moti Asayag <[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
