Vojtech Szocs has posted comments on this change. Change subject: userportal,webadmin: messages locale unit test ......................................................................
Patch Set 2: (6 comments) http://gerrit.ovirt.org/#/c/32995/2/frontend/webadmin/modules/gwt-common/src/test/java/org/ovirt/engine/ui/common/CommonApplicationMessagesTest.java File frontend/webadmin/modules/gwt-common/src/test/java/org/ovirt/engine/ui/common/CommonApplicationMessagesTest.java: Line 46: for (File localeFile: getMessagesPropertiesFiles(".")) { Line 47: Properties properties = loadProperties(localeFile); Line 48: compareMethodsToProperties(messagesMethods, properties, localeFile); Line 49: } Line 50: assertTrue(format(errors), errors.isEmpty()); +1 on using List to aggregate any errors during test execution and asserting that List is empty at the end. Line 51: } Line 52: Line 53: private String format(List<String> errors) { Line 54: StringBuilder builder = new StringBuilder(); Line 58: } Line 59: return builder.toString(); Line 60: } Line 61: Line 62: protected void compareMethodsToProperties(Method[] messagesMethods, Properties properties, File propertiesFile) { Just a small thing, I see that "File propertiesFile" is passed around only to do "propertiesFile.getName()", you could just pass "String propertiesFileName" instead. Or better yet, you could consider creating helper structure to aggregate all relevant data like this: static class MessagesPropertiesFileInfo { private Properties properties; private String propertiesFileName; // no need for accessor methods, outer class has access to above fields } But I'm OK with current code too, so it's up to your consideration :-) Line 63: for (Method method: messagesMethods) { Line 64: int count = 0; Line 65: checkForMissingDefault(properties, propertiesFile, method); Line 66: checkPlaceHolders(properties, propertiesFile, method, count); Line 66: checkPlaceHolders(properties, propertiesFile, method, count); Line 67: } Line 68: } Line 69: Line 70: private void checkPlaceHolders(Properties properties, File propertiesFile, Method method, int count) { I assume "count" is the starting placeholder index here? Primitive int is passed into method by value (not by reference) so doing "count++" and exiting this method will not modify "int count" value inside compareMethodsToProperties() method. In other words, we could have "int count = 0;" right inside this method, why do we need it inside compareMethodsToProperties() method? Line 71: if (properties.getProperty(method.getName()) != null) { Line 72: Set<Integer> foundIndex = new HashSet<Integer>(); Line 73: Set<Integer> requiredIndexes = determineRequiredIndexes(method.getParameterAnnotations()); Line 74: int minRequired = requiredIndexes.size(); Line 148: * @param path The path to the directory. Line 149: * @return An {@code Array} of {@code File} objects. Line 150: * @throws URISyntaxException If path doesn't exist Line 151: */ Line 152: private File[] getMessagesPropertiesFiles(final String path) throws URISyntaxException { Hm, we could accept "Class<? extends Messages> messagesClass" argument here, so that we can resolve corresponding properties files more accurately: * name starts with <messagesClassSimpleName> * name ends with ".properties" In other words, if there are other properties files (for other message classes) we wouldn't want to pick them up here, we just want properties files for given message class. Line 153: File currentDir = new File( Line 154: this.getClass().getResource(path).toURI().toASCIIString().replaceAll("file:", "")); Line 155: return currentDir.listFiles(new FilenameFilter() { Line 156: http://gerrit.ovirt.org/#/c/32995/2/frontend/webadmin/modules/pom.xml File frontend/webadmin/modules/pom.xml: Line 199: </sources> Line 200: </configuration> Line 201: </plugin> Line 202: <plugin> Line 203: <artifactId>maven-checkstyle-plugin</artifactId> Why is this change needed? Perhaps the intention was to put common checkstyle configuration in parent POM for all frontend modules? If so, shouldn't we remove checkstyle configuration in specific frontend module POMs? Line 204: <dependencies> Line 205: <dependency> Line 206: <groupId>org.ovirt.engine</groupId> Line 207: <artifactId>checkstyles</artifactId> http://gerrit.ovirt.org/#/c/32995/2/frontend/webadmin/modules/userportal-gwtp/src/test/java/org/ovirt/engine/ui/userportal/ApplicationMessagesTest.java File frontend/webadmin/modules/userportal-gwtp/src/test/java/org/ovirt/engine/ui/userportal/ApplicationMessagesTest.java: Line 16: super.setUp(); Line 17: List<Method> allMessageMethods = new ArrayList<Method>(); Line 18: allMessageMethods.addAll(Arrays.asList(messagesMethods)); Line 19: allMessageMethods.addAll(Arrays.asList(ApplicationMessages.class.getDeclaredMethods())); Line 20: messagesMethods = allMessageMethods.toArray(new Method[allMessageMethods.size()]); I think this code could be a lot simpler if we avoid using Java arrays, for example: protected List<Method> messagesMethods; instead of: protected Method[] messagesMethods; If we expect list-like structure to be manipulated, representing it as plain Java array complicates future modification since Java arrays are fixed-size. In such cases, using ArrayList / List abstraction should simplify the code: // assume messagesMethods is List<Method> super.setUp(); messagesMethods.addAll(ApplicationMessages.class.getDeclaredMethods()); Line 21: } -- To view, visit http://gerrit.ovirt.org/32995 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Icda389f2ca08e9d3b7515bb4a0dc1213bc91aeef Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Einav Cohen <eco...@redhat.com> Gerrit-Reviewer: Vojtech Szocs <vsz...@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
