Mike Kolesnik has posted comments on this change. Change subject: core: MacPool related Commands ......................................................................
Patch Set 27: (18 comments) http://gerrit.ovirt.org/#/c/28705/27/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MacPoolValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MacPoolValidator.java: Line 61: return null; Line 62: } Line 63: Line 64: private ValidationResult testNameNullity() { Line 65: if (macPool.getName() == null) { This should be done by javax.validation on the MacPool itself Line 66: return new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_NAME_MAY_NOT_BE_EMPTY); Line 67: } else { Line 68: return null; Line 69: } Line 69: } Line 70: } Line 71: Line 72: private ValidationResult testIdNullity() { Line 73: if (macPool.getId() == null) { This should be done by javax.validation on the MacPool itself Line 74: return new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_MAC_POOL_DOES_NOT_EXIST); Line 75: } else { Line 76: return null; Line 77: } Line 79: Line 80: private boolean macPoolNameUnique() { Line 81: final List<MacPool> macPools = getMacPoolDao().getAll(); Line 82: for (MacPool pool : macPools) { Line 83: if ((pool.getId() != null && !pool.getId().equals(macPool.getId())) && You could use Objects.equals if you're concerned about null However, I believe that this isn't a check worth having since if the ID is null then it's clearly a bug and should fail for NPE since the DAO can't be returning entities with null ID in them, so I'd rather it failed on NPE than hide this weird DAO behavior. Line 84: pool.getName().equals(macPool.getName())) { Line 85: return false; Line 86: } Line 87: } http://gerrit.ovirt.org/#/c/28705/27/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateMacPoolValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateMacPoolValidator.java: Line 2: Line 3: import org.ovirt.engine.core.common.businessentities.MacPool; Line 4: import org.ovirt.engine.core.common.errors.VdcBllMessages; Line 5: Line 6: public class UpdateMacPoolValidator { Should inherit MacPoolValidator and probably be an inner class of UpdateMacPoolCommand since it's not needed out of that context.. Line 7: private final MacPool macPoolFromDb; Line 8: private final MacPool newMacPool; Line 9: Line 10: public UpdateMacPoolValidator(MacPool macPoolFromDb, MacPool newMacPool) { Line 15: this.macPoolFromDb = macPoolFromDb; Line 16: this.newMacPool = newMacPool; Line 17: } Line 18: Line 19: public ValidationResult defaultFlagIsNotChanged() { You could receive the new pool in parameters, sparing the need to save in field. Line 20: final boolean failed = macPoolFromDb.isDefaultPool() != newMacPool.isDefaultPool(); Line 21: return ValidationResult.failWith(VdcBllMessages.ACTION_TYPE_FAILED_CHANGING_DEFAULT_MAC_POOL_IS_NOT_SUPPORTED). Line 22: when(failed); Line 23: } Line 16: this.newMacPool = newMacPool; Line 17: } Line 18: Line 19: public ValidationResult defaultFlagIsNotChanged() { Line 20: final boolean failed = macPoolFromDb.isDefaultPool() != newMacPool.isDefaultPool(); Probably a better name would be "defaultChanged", I would even inline it.. Line 21: return ValidationResult.failWith(VdcBllMessages.ACTION_TYPE_FAILED_CHANGING_DEFAULT_MAC_POOL_IS_NOT_SUPPORTED). Line 22: when(failed); Line 23: } Line 18: Line 19: public ValidationResult defaultFlagIsNotChanged() { Line 20: final boolean failed = macPoolFromDb.isDefaultPool() != newMacPool.isDefaultPool(); Line 21: return ValidationResult.failWith(VdcBllMessages.ACTION_TYPE_FAILED_CHANGING_DEFAULT_MAC_POOL_IS_NOT_SUPPORTED). Line 22: when(failed); The operator (dot) should be in the start of this line, making it easier to identify that it's a continuation of the previous line. Line 23: } http://gerrit.ovirt.org/#/c/28705/27/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 19: Line 20: public class MacPoolValidatorTest { Line 21: Line 22: private MacPool macPool; Line 23: private MacPoolDao macPoolDaoMock; You should use @Mock Line 24: Line 25: @Before Line 26: public void setUp() throws Exception { Line 27: macPool = new MacPool(); Line 25: @Before Line 26: public void setUp() throws Exception { Line 27: macPool = new MacPool(); Line 28: Line 29: final DbFacade dbFacadeMock = mock(DbFacade.class); Since DAO mocking seems to be returning in your tests, you can extract this to a JUnit Rule similar to MockConfigRule (for example). This new rule can accept a list of the DAO classes to mock and handle DbFacade & DAO mocking behind the scenes. I think this will be beneficial in many places that need to mock DAOs and currently do all this boilerplate setup manually.. You should do this in a follow up patch for the tests you added in this patch set (or if you want to edit the patches, you can send this as a preliminary patch and rebase the other patches on top, changing them to use the new rule). Line 30: DbFacadeLocator.setDbFacade(dbFacadeMock); Line 31: macPoolDaoMock = mock(MacPoolDao.class); Line 32: Line 33: when(dbFacadeMock.getMacPoolDao()).thenReturn(macPoolDaoMock); Line 91: private ValidationResult callHasUniqueName(boolean sameId, boolean sameName, Guid newId, String newName) { Line 92: final Guid macPool1Id = Guid.newGuid(); Line 93: final Guid macPool2Id = sameId ? macPool1Id : newId; Line 94: final String macPool1Name = "macPool1"; Line 95: final String macPool2Name = sameName ? macPool1Name : newName; This all should be decided outside this method, probably in the one calling it.. Line 96: Line 97: final MacPool existingMacPool = createMacPool(macPool1Id, macPool1Name); Line 98: Line 99: when(macPoolDaoMock.getAll()).thenReturn(Arrays.asList( Line 93: final Guid macPool2Id = sameId ? macPool1Id : newId; Line 94: final String macPool1Name = "macPool1"; Line 95: final String macPool2Name = sameName ? macPool1Name : newName; Line 96: Line 97: final MacPool existingMacPool = createMacPool(macPool1Id, macPool1Name); This should be inlined Line 98: Line 99: when(macPoolDaoMock.getAll()).thenReturn(Arrays.asList( Line 100: existingMacPool Line 101: )); Line 99: when(macPoolDaoMock.getAll()).thenReturn(Arrays.asList( Line 100: existingMacPool Line 101: )); Line 102: Line 103: final MacPool newMacPool = createMacPool(macPool2Id, macPool2Name); This should be inlined Line 104: return new MacPoolValidator(newMacPool).hasUniqueName(); Line 105: } Line 106: Line 107: private MacPool createMacPool(Guid macPool1Id, String macPool1Name) { Line 104: return new MacPoolValidator(newMacPool).hasUniqueName(); Line 105: } Line 106: Line 107: private MacPool createMacPool(Guid macPool1Id, String macPool1Name) { Line 108: final MacPool macPool1 = new MacPool(); No need for "1" in variable name Line 109: macPool1.setName(macPool1Name); Line 110: macPool1.setId(macPool1Id); Line 111: return macPool1; Line 112: } Line 135: Line 136: protected void notRemovingUsedPool(boolean used) { Line 137: macPool.setId(Guid.newGuid()); Line 138: int usageCount = used ? 1 : 0; Line 139: when(macPoolDaoMock.getDcUsageCount(eq(macPool.getId()))).thenReturn(usageCount); This mocking should be in a method that receives "usageCount". Line 140: Line 141: final ValidationResult validationResult = new MacPoolValidator(macPool).notRemovingUsedPool(); Line 142: if (used) { Line 143: hasMessage(validationResult, VdcBllMessages.ACTION_TYPE_FAILED_CANNOT_REMOVE_STILL_USED_MAC_POOL); Line 138: int usageCount = used ? 1 : 0; Line 139: when(macPoolDaoMock.getDcUsageCount(eq(macPool.getId()))).thenReturn(usageCount); Line 140: Line 141: final ValidationResult validationResult = new MacPoolValidator(macPool).notRemovingUsedPool(); Line 142: if (used) { This test logic should be in the tests themselves, just the mocking is shared. If you realla want to "save" on duplicating the call to validator, you can put the also in the mocking method (i.e. mockAndCallNotRemovingUsedPool or something similar) and juts check the result, although I find it less comprehensible that way and would rather see the call itself in each test. Line 143: hasMessage(validationResult, VdcBllMessages.ACTION_TYPE_FAILED_CANNOT_REMOVE_STILL_USED_MAC_POOL); Line 144: } else { Line 145: valid(validationResult); Line 146: } Line 157: valid(new MacPoolValidator(macPool).macPoolExists()); Line 158: } Line 159: Line 160: //should be replaced with custom matches Line 161: static void valid(ValidationResult actual) { You should use ValidationResultMatchers.isValid (See example in NetworkValidatorTest) Line 162: Assert.assertThat(actual.getMessage(), is(ValidationResult.VALID.getMessage())); Line 163: } Line 164: Line 165: //should be replaced with custom matches Line 162: Assert.assertThat(actual.getMessage(), is(ValidationResult.VALID.getMessage())); Line 163: } Line 164: Line 165: //should be replaced with custom matches Line 166: static void hasMessage(ValidationResult actual, VdcBllMessages message) { You should use ValidationResultMatchers.failsWith (See example in NetworkValidatorTest) Line 167: Assert.assertThat(actual.getMessage(), is(message)); Line 168: } Line 169: http://gerrit.ovirt.org/#/c/28705/27/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 58: @DefaultStringValue("Mac pool cannot be removed. Either it's used or it's default mac pool.") Line 59: String MAC_POOL_CANNOT_BE_REMOVED(); Line 60: Line 61: @DefaultStringValue("Mac pool does not exist.") Line 62: String MAC_POOL_DOES_NOT_EXIST(); You should sync the messages from the other files.. Line 63: Line 64: @DefaultStringValue("Mac pool cannot be created. Shared MAC pools must have names.") Line 65: String MAC_POOL_MUST_HAVE_NAME(); Line 66: -- 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: 27 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Mike Kolesnik <mkole...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
