Mike Kolesnik has posted comments on this change.
Change subject: engine: Replace return type of ReplacementUtils
......................................................................
Patch Set 1: (6 inline comments)
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ReplacementUtils.java
Line 38: if (items.size() > MAX_NUMBER_OF_PRINTED_ITEMS) {
Line 39: printedItems.add("\t...");
Line 40: }
Line 41:
Line 42: return new ArrayList<String>(
Since you're doing this to return a mutable list, do you mind documenting it so
that whomever is using the method can know that he can modify the returned list?
Line 43: Arrays.asList(MessageFormat.format("${0} {1}",
propertyName, StringUtils.join(printedItems, ",\n")),
Line 44: MessageFormat.format("${0}_COUNTER {1}", propertyName,
items.size())));
Line 45: }
Line 46:
....................................................
File
backend/manager/modules/utils/src/test/java/org/ovirt/engine/core/utils/ReplacementUtilsTest.java
Line 52: List<Object> items = createItems();
Line 53: Collection<String> messageItems =
ReplacementUtils.replaceWith(PROPERTY_NAME, items);
Line 54: validateMessageContainsProperties(messageItems);
Line 55: Iterator<String> messageIterator = messageItems.iterator();
Line 56:
validateMessageDoesNotContainUnexpectedItems(messageIterator.next(), items);
Since Collection doesn't guarantee order, it becomes problematic to test that
the 1st item is the message and the 2nd is the count.
Line 57:
assertTrue(messageIterator.next().contains(String.valueOf(items.size())));
Line 58: }
Line 59:
Line 60: private void validateMessageDoesNotContainUnexpectedItems(String
message, List<Object> items) {
Line 78: }
Line 79:
Line 80: private void validateMessageContainsProperties(Collection<String>
messageItems) {
Line 81: assertNotNull(messageItems);
Line 82: assertFalse(messageItems.isEmpty());
No need since you check later the size which is equivalent to emptyness check
Line 83: assertTrue(messageItems.size() == 2);
Line 84: Iterator<String> messageIterator = messageItems.iterator();
Line 85: assertTrue(messageIterator.next().contains(PROPERTY_NAME));
Line 86:
assertTrue(messageIterator.next().contains(PROPERTY_COUNTER_NAME));
Line 79:
Line 80: private void validateMessageContainsProperties(Collection<String>
messageItems) {
Line 81: assertNotNull(messageItems);
Line 82: assertFalse(messageItems.isEmpty());
Line 83: assertTrue(messageItems.size() == 2);
Preferably use assertEquals(2, messageItems.size())
Which will print the size in case it didn't match, so that you get more info
and not just "assert failed" message
Line 84: Iterator<String> messageIterator = messageItems.iterator();
Line 85: assertTrue(messageIterator.next().contains(PROPERTY_NAME));
Line 86:
assertTrue(messageIterator.next().contains(PROPERTY_COUNTER_NAME));
Line 87: }
Line 81: assertNotNull(messageItems);
Line 82: assertFalse(messageItems.isEmpty());
Line 83: assertTrue(messageItems.size() == 2);
Line 84: Iterator<String> messageIterator = messageItems.iterator();
Line 85: assertTrue(messageIterator.next().contains(PROPERTY_NAME));
Same comment about order here..
Line 86:
assertTrue(messageIterator.next().contains(PROPERTY_COUNTER_NAME));
Line 87: }
Line 88:
Line 89: private <T> void validateMessageItems(Collection<String>
messageItems, List<T> items) {
Line 88:
Line 89: private <T> void validateMessageItems(Collection<String>
messageItems, List<T> items) {
Line 90: validateMessageContainsProperties(messageItems);
Line 91: Iterator<String> messageIterator = messageItems.iterator();
Line 92: assertTrue(messageIterator.next().contains(PROPERTY_VALUE));
And here
Line 93:
assertTrue(messageIterator.next().contains(String.valueOf(items.size())));
Line 94: }
--
To view, visit http://gerrit.ovirt.org/10761
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If0733f3bb5fec70896a685452125c67d1f14784f
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Moti Asayag <[email protected]>
Gerrit-Reviewer: Mike Kolesnik <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
