Alexander Wels 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 def Done 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 I am not sure I understand what you are saying here. 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 CommonApplic So am I, this shouldn't be there. 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. Done 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() A Done 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 unneces Done 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
