Allon Mureinik has posted comments on this change. Change subject: engine: Improve test assert messages ......................................................................
Patch Set 4: (16 comments) http://gerrit.ovirt.org/#/c/27775/4/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/StorageDomainCommandBaseTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/StorageDomainCommandBaseTest.java: Line 96: public void checkStorageDomainNotEqualWithStatusActive() { Line 97: setStorageDomainStatus(StorageDomainStatus.Active); Line 98: assertFalse(cmd.checkStorageDomainStatusNotEqual(StorageDomainStatus.Active)); Line 99: List<String> messages = cmd.getReturnValue().getCanDoActionMessages(); Line 100: assertEquals(2, messages.size()); > shouldn't actual <--> expected be replaced here as well ? Yes - will fix. Nice catch! Line 101: assertEquals(messages.get(0), VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_DOMAIN_STATUS_ILLEGAL2.toString()); Line 102: assertEquals(messages.get(1), String.format("$status %1$s", StorageDomainStatus.Active)); Line 103: } Line 104: Line 123: Line 124: assertTrue(cmd.isLunsAlreadyInUse(specifiedLunIds)); Line 125: List<String> messages = cmd.getReturnValue().getCanDoActionMessages(); Line 126: assertEquals(2, messages.size()); Line 127: assertEquals(messages.get(0), VdcBllMessages.ACTION_TYPE_FAILED_LUNS_ALREADY_PART_OF_STORAGE_DOMAINS.toString()); > shouldn't actual <--> expected be replaced here as well ? Done Line 128: assertEquals(messages.get(1), String.format("$lunIds %1$s", cmd.getFormattedLunId(lun1, lun1.getStorageDomainName()))); Line 129: } Line 130: Line 131: private void storagePoolExists() { http://gerrit.ovirt.org/#/c/27775/4/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/GlusterVolumeValidatorTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/GlusterVolumeValidatorTest.java: Line 28: @Test Line 29: public void withoutForceSucceedsIn31Cluster() { Line 30: validator = spy(new GlusterVolumeValidator()); Line 31: ValidationResult result = validator.isForceCreateVolumeAllowed(Version.v3_1, false); Line 32: assertEquals(ValidationResult.VALID, result); > Since the project already introduced the ValidationResultMatchers class for Done Line 33: } Line 34: Line 35: @Test Line 36: public void withForceFailsIn31Cluster() { Line 37: validator = spy(new GlusterVolumeValidator()); Line 38: ValidationResult result = validator.isForceCreateVolumeAllowed(Version.v3_1, true); Line 39: assertEquals Line 40: (new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_GLUSTER_VOLUME_ADD_BRICK_FORCE_NOT_SUPPORTED), Line 41: result); > and here: Done Line 42: } Line 43: Line 44: @Test Line 45: public void withoutForceSucceedsIn33Cluster() { Line 44: @Test Line 45: public void withoutForceSucceedsIn33Cluster() { Line 46: validator = spy(new GlusterVolumeValidator()); Line 47: ValidationResult result = validator.isForceCreateVolumeAllowed(Version.v3_3, false); Line 48: assertEquals(ValidationResult.VALID, result); ...and here too. Line 49: } Line 50: Line 51: @Test Line 52: public void withForceFailsIn33Cluster() { Line 51: @Test Line 52: public void withForceFailsIn33Cluster() { Line 53: validator = spy(new GlusterVolumeValidator()); Line 54: ValidationResult result = validator.isForceCreateVolumeAllowed(Version.v3_3, true); Line 55: assertEquals(ValidationResult.VALID, result); and here. Line 56: } http://gerrit.ovirt.org/#/c/27775/4/backend/manager/modules/common/src/test/java/org/ovirt/engine/core/common/utils/MTUValidatorTest.java File backend/manager/modules/common/src/test/java/org/ovirt/engine/core/common/utils/MTUValidatorTest.java: Line 39: Line 40: @Test Line 41: public void invalidLowMTU() { Line 42: Set<ConstraintViolation<MtuContainer>> validate = validate(new MtuContainer(30)); Line 43: Assert.assertFalse(validate.isEmpty()); > modify to static import ? Done Line 44: Line 45: } Line 46: Line 47: @Test Line 46: Line 47: @Test Line 48: public void useDefaultMTU() { Line 49: Set<ConstraintViolation<MtuContainer>> validate = validate(new MtuContainer(0)); Line 50: Assert.assertEquals(0, validate.size()); > following the change above, i'd expect this one to look as: Done Line 51: } Line 52: Line 53: private <T extends Object> Set<ConstraintViolation<T>> validate(T object) { Line 54: return validator.validate(object); http://gerrit.ovirt.org/#/c/27775/4/backend/manager/modules/common/src/test/java/org/ovirt/engine/core/common/utils/PairTest.java File backend/manager/modules/common/src/test/java/org/ovirt/engine/core/common/utils/PairTest.java: Line 25: Pair<Boolean, String> p1 = new Pair<Boolean, String>(true, "abc"); Line 26: Pair<Boolean, String> p2 = new Pair<Boolean, String>(true, "abc"); Line 27: Pair<Boolean, String> p3 = new Pair<Boolean, String>(false, "abc"); Line 28: Line 29: Assert.assertEquals(p1, p2); > perhaps add here also: This wasn't part of the original test, so such a change will change the semantics of success/failure. Can you explain why it's appropriate? Line 30: Assert.assertNotEquals(p1, p3); Line 31: } http://gerrit.ovirt.org/#/c/27775/4/backend/manager/modules/compat/src/test/java/org/ovirt/engine/core/compat/GuidTest.java File backend/manager/modules/compat/src/test/java/org/ovirt/engine/core/compat/GuidTest.java: Line 16: public void testCompareTo() { Line 17: Guid guid1 = new Guid("5b411bc1-c220-4421-9abd-cfa484aecb6e"); Line 18: Guid guid2 = new Guid("5b411bc1-c220-4421-9abd-cfa484aecb6f"); Line 19: assertTrue(guid1.compareTo(guid2) < 0); Line 20: Assert.assertEquals(0, guid1.compareTo(guid1)); > please omit class notation. Done Line 21: assertTrue(guid2.compareTo(guid1) > 0); Line 22: } Line 23: Line 24: @Test http://gerrit.ovirt.org/#/c/27775/4/backend/manager/modules/compat/src/test/java/org/ovirt/engine/core/compat/RpmVersionTest.java File backend/manager/modules/compat/src/test/java/org/ovirt/engine/core/compat/RpmVersionTest.java: Line 13: assertEquals("2.3.17.0", new RpmVersion("rhev-agent-2.3.17-1.el6").getValue()); Line 14: assertEquals("2.133.4.5", new RpmVersion("glibc-devel-2.133.4.5-1.x86_64").getValue()); Line 15: assertEquals("10.6.2.0", new RpmVersion("test-javadb-common-10.6.2-1.1.i386").getValue()); Line 16: assertEquals("10.6.2.3", new RpmVersion("test-javadb-common-10.6.2.3.3-1.1.i386").getValue()); Line 17: assertEquals(0, new RpmVersion("").getValue().length()); > how about: Done Line 18: assertEquals(0, new RpmVersion(null).getValue().length()); Line 19: } Line 20: Line 21: @Test Line 14: assertEquals("2.133.4.5", new RpmVersion("glibc-devel-2.133.4.5-1.x86_64").getValue()); Line 15: assertEquals("10.6.2.0", new RpmVersion("test-javadb-common-10.6.2-1.1.i386").getValue()); Line 16: assertEquals("10.6.2.3", new RpmVersion("test-javadb-common-10.6.2.3.3-1.1.i386").getValue()); Line 17: assertEquals(0, new RpmVersion("").getValue().length()); Line 18: assertEquals(0, new RpmVersion(null).getValue().length()); > same Done Line 19: } Line 20: Line 21: @Test Line 22: public void agentVersionTest() { Line 27: assertEquals("2.3.7.10", new RpmVersion("rhev-agent-2.3.7.10.3-1.el6", "RHEV-Agent", true).getValue()); Line 28: assertEquals("2.3.7.10", new RpmVersion("rhev-agent-2.3.7.10.3-1.el6", "rhev-agent", false).getValue()); Line 29: assertEquals("2.3.7.10", new RpmVersion("rhev-agent-2.3.7.10.3-1.el6", "RHEV-Agent", false).getValue()); Line 30: assertNotEquals("0.0.0.0", new RpmVersion("rhev-agent-2.3.7.10.3-1.el6", "RHEV-Agent", false).getValue()); Line 31: assertEquals(0, new RpmVersion("", "", false).getValue().length()); > and same here Done Line 32: assertEquals(0, new RpmVersion(null, null, false).getValue().length()); Line 33: } Line 34: Line 35: @Test Line 28: assertEquals("2.3.7.10", new RpmVersion("rhev-agent-2.3.7.10.3-1.el6", "rhev-agent", false).getValue()); Line 29: assertEquals("2.3.7.10", new RpmVersion("rhev-agent-2.3.7.10.3-1.el6", "RHEV-Agent", false).getValue()); Line 30: assertNotEquals("0.0.0.0", new RpmVersion("rhev-agent-2.3.7.10.3-1.el6", "RHEV-Agent", false).getValue()); Line 31: assertEquals(0, new RpmVersion("", "", false).getValue().length()); Line 32: assertEquals(0, new RpmVersion(null, null, false).getValue().length()); And here. Line 33: } Line 34: Line 35: @Test Line 36: public void caseSensitiveTest() { http://gerrit.ovirt.org/#/c/27775/4/backend/manager/modules/compat/src/test/java/org/ovirt/engine/core/compat/StringHelperTest.java File backend/manager/modules/compat/src/test/java/org/ovirt/engine/core/compat/StringHelperTest.java: Line 10: Assert.assertEquals("foobarbaz", StringHelper.join(null, new String[]{"foo", "bar", "baz"})); Line 11: Line 12: //TODO: is this the intended behavior? Line 13: Assert.assertEquals("foo,,bar", StringHelper.join(",", new String[]{"foo", null, "bar"})); Line 14: Assert.assertNull(StringHelper.join(null, null)); > change to static import ? Done Line 15: } http://gerrit.ovirt.org/#/c/27775/4/backend/manager/modules/compat/src/test/java/org/ovirt/engine/core/compat/VersionTest.java File backend/manager/modules/compat/src/test/java/org/ovirt/engine/core/compat/VersionTest.java: Line 18: Assert.assertEquals(new Version(1, 2), new Version("1.2")); Line 19: Assert.assertEquals(new Version(1, 2, 3), new Version("1.2.3")); Line 20: Assert.assertEquals(new Version(1, 2, 3, 4), new Version("1.2.3.4")); Line 21: // nulls and other data types Line 22: Assert.assertNotNull(new Version()); > change to static import ? Done Line 23: Assert.assertNotEquals("foo", new Version()); Line 24: Assert.assertNotEquals(1.0d, new Version()); Line 25: } -- To view, visit http://gerrit.ovirt.org/27775 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie5bdb907e57b51fcdd8fd8b7a9148273c0304d11 Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Mike Kolesnik <mkole...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Ori Liel <ol...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Sahina Bose <sab...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@redhat.com> Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <yzasl...@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
