Vojtech Szocs has posted comments on this change. Change subject: userportal,webadmin: messages locale unit test ......................................................................
Patch Set 4: (6 comments) http://gerrit.ovirt.org/#/c/32995/4/frontend/webadmin/modules/gwt-common/pom.xml File frontend/webadmin/modules/gwt-common/pom.xml: Line 85: </resource> Line 86: </resources> Line 87: <testResources> Line 88: <testResource> Line 89: <directory>${project.basedir}/src/main/resources</directory> I think we can omit ${project.basedir} prefix here, as it should be the default base directory anyway: <directory>src/main/resources</directory> Line 90: <includes> Line 91: <include>**/*Messages*.properties</include> Line 92: </includes> Line 93: </testResource> http://gerrit.ovirt.org/#/c/32995/4/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 14: @Test Line 15: public void doTest() throws URISyntaxException, IOException { Line 16: File messageFileDir = new File( Line 17: this.getClass().getResource(".").toURI().toASCIIString().replaceAll("file:", "")); //$NON-NLS-1$ $NON-NLS-2$ $NON-NLS-3$ Line 18: List<String> errors = GwtMessagesValidator.validateClass(CommonApplicationMessages.class, messageFileDir); Hm, if we're validating a single class, its properties file is expected to be in a directory that corresponds to class package. Validation of multiple classes can be modified to validation of single class, where we scan all (declared & inherited) methods, and again expect its properties file to be in package class directory. What do you think? Line 19: assertTrue(GwtMessagesValidator.format(errors), errors.isEmpty()); Line 20: } Line 21: http://gerrit.ovirt.org/#/c/32995/4/frontend/webadmin/modules/gwt-common/src/test/java/org/ovirt/engine/ui/common/GwtMessagesValidator.java File frontend/webadmin/modules/gwt-common/src/test/java/org/ovirt/engine/ui/common/GwtMessagesValidator.java: Line 43: File messagesDir) throws URISyntaxException, IOException { Line 44: List<String> errors = new ArrayList<String>(); Line 45: List<Method> messagesMethods = new ArrayList<Method>(); Line 46: messagesMethods.addAll(Arrays.asList(classUnderTest.getDeclaredMethods())); Line 47: classUnderTest = CommonApplicationMessages.class; I'm puzzled by this line, is this here to ensure we also check CommonApplicationMessages (super-interface) methods? I think that we should get all public methods from classUnderTest - both ones declared by classUnderTest & ones declared by any super-class / super-interface. Since Messages is interface, and since Java interface has only public methods, we can use class.getMethods() API which returns all public methods of given class (interface), including methods inherited from any super-class (super-interface). Also, we should also check that classUnderTest is indeed an interface and not an actual class, using class.isInterface() API. Line 48: Line 49: for (File localeFile: getMessagesPropertiesFiles(messagesDir, classUnderTest)) { Line 50: PropertiesFileInfo.properties = loadProperties(localeFile); Line 51: PropertiesFileInfo.fileName = localeFile.getName(); Line 63: } Line 64: Line 65: private static void checkPlaceHolders(Method method, List<String> errors) { Line 66: int count = 0; Line 67: if (PropertiesFileInfo.properties.getProperty(method.getName()) != null) { Minor thing, maybe worth extracting method.getName() into local variable. Line 68: Set<Integer> foundIndex = new HashSet<Integer>(); Line 69: Set<Integer> requiredIndexes = determineRequiredIndexes(method.getParameterAnnotations()); Line 70: int minRequired = requiredIndexes.size(); Line 71: // Check to make sure the number of parameters is inside the range defined. Line 99: } Line 100: } Line 101: } Line 102: Line 103: private static int getMaxMethodParameter(Annotation[][] typeAnnotations) { To count method parameters, you could also use method.getParameterTypes() API. Maybe consider renaming this method to something like getMethodParamCount? Something like this: private static int getMethodParamCount(Method method) { return method.getParameterTypes().length; } Line 104: return typeAnnotations.length; Line 105: } Line 106: Line 107: private static Set<Integer> determineRequiredIndexes(Annotation[][] typeAnnotations) { Line 111: Annotation[] typeAnnotation = typeAnnotations[i]; Line 112: if (typeAnnotation.length > 0) { Line 113: for (Annotation annotation: typeAnnotation) { Line 114: if (annotation.annotationType().equals(Optional.class)) { Line 115: isOptional = true; Maybe break out of for loop once we detect optional index, to avoid unnecessary compute cycles? Line 116: } Line 117: } Line 118: } Line 119: if (!isOptional) { -- 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: 4 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
