Allon Mureinik has posted comments on this change.
Change subject: core,ui: test duplicate keys in appErrors
......................................................................
Patch Set 4: I would prefer that you didn't submit this
(13 inline comments)
See inline.
....................................................
File
backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/DuplicateKeysTest.java
Line 12: public void testkDuplicateKeys() {
Line 13: String baseDir = System.getProperty("basedir");
Line 14: assumeNotNull(baseDir);
Line 15: String fileName = "AppErrors.properties";
Line 16: File file = new File(baseDir + "/src/main/resources/bundles/"
+ fileName);
fileName seems a bit redundant - why not do this in a single line?
File file = new File(baseDir +
"/src/main/resources/bundles/AppErrors.properties");
Line 17: DuplicateKeysCheck.checkDuplicateKeys(file.getAbsolutePath());
Line 18: }
Line 19:
....................................................
File
backend/manager/modules/utils/src/test/java/org/ovirt/engine/core/utils/DuplicateKeysCheck.java
Line 7: import java.io.InputStream;
Line 8:
Line 9:
Line 10: public class DuplicateKeysCheck {
Line 11: public static void checkDuplicateKeys(String filePath) {
I'd rename the method to assertNoDuplicateKeys(path) - it's a tad clearer, and
it overcomes some annoying code analyzers that insist that every test have an
assert.
Line 12: InputStream is = null;
Line 13: try {
Line 14: is = new FileInputStream(filePath);
Line 15: } catch (FileNotFoundException e) {
Line 11: public static void checkDuplicateKeys(String filePath) {
Line 12: InputStream is = null;
Line 13: try {
Line 14: is = new FileInputStream(filePath);
Line 15: } catch (FileNotFoundException e) {
It's better to just throw this upwards - if you're giving a wrong file, your
test shouldn't fail logically, it should break with a
DeveloperDoesntKnowWhereHisFilesAreException, which is of course a subclass of
ThisTestWillNeverWorkException.
Line 16: fail(e.getMessage());
Line 17: }
Line 18: ExtendedProperties props = new ExtendedProperties();
Line 19: try {
Line 17: }
Line 18: ExtendedProperties props = new ExtendedProperties();
Line 19: try {
Line 20: props.load(is);
Line 21: }
redundant blank line
Line 22:
Line 23: catch (Exception exception) {
Line 24: fail("Check for duplicate keys in " + filePath + " failed:
" + exception.getMessage());
Line 25: }
....................................................
File
backend/manager/modules/utils/src/test/java/org/ovirt/engine/core/utils/ExtendedProperties.java
Line 1: package org.ovirt.engine.core.utils;
Line 2:
Line 3: import java.util.Properties;
Line 4:
Line 5: public class ExtendedProperties extends Properties {
Really not a fan of the name - it does not imply what this class does.
How about "NoDuplicateProperties", or something like that?
Line 6: /**
Line 7: * This method overrides the put method of Map in order to add the
validation
Line 8: * of existing key. Otherwise the Hashtable implementation
inherited by
Line 9: * Properties class it would just override existing value without
indicating
Line 3: import java.util.Properties;
Line 4:
Line 5: public class ExtendedProperties extends Properties {
Line 6: /**
Line 7: * This method overrides the put method of Map in order to add the
validation
consider using a @link javadoc to "put" and "Map", "Hashtable" and "Properties"
Line 8: * of existing key. Otherwise the Hashtable implementation
inherited by
Line 9: * Properties class it would just override existing value without
indicating
Line 10: * the caller that it already exists.
Line 11: * @param key
Line 4:
Line 5: public class ExtendedProperties extends Properties {
Line 6: /**
Line 7: * This method overrides the put method of Map in order to add the
validation
Line 8: * of existing key. Otherwise the Hashtable implementation
inherited by
either "existing keys" or "an existing key"
Line 9: * Properties class it would just override existing value without
indicating
Line 10: * the caller that it already exists.
Line 11: * @param key
Line 12: * @param value
Line 9: * Properties class it would just override existing value without
indicating
Line 10: * the caller that it already exists.
Line 11: * @param key
Line 12: * @param value
Line 13: * @return
Either document @param and @return, or remove them
Line 14: */
Line 15: @Override
Line 16: public Object put(Object key, Object value) {
Line 17: if(this.containsKey(key)) {
Line 13: * @return
Line 14: */
Line 15: @Override
Line 16: public Object put(Object key, Object value) {
Line 17: if(this.containsKey(key)) {
drop the "this.", it's useless.
Line 18: throw new RuntimeException("The key " + key + " already
exists");
Line 19: }
Line 20: return super.put(key, value);
Line 21: }
Line 14: */
Line 15: @Override
Line 16: public Object put(Object key, Object value) {
Line 17: if(this.containsKey(key)) {
Line 18: throw new RuntimeException("The key " + key + " already
exists");
Since this is a test implementation anyway, consider just calling Assert.fail.
Line 19: }
Line 20: return super.put(key, value);
Line 21: }
....................................................
Commit Message
Line 6:
Line 7: core,ui: test duplicate keys in appErrors
Line 8:
Line 9: The tests introduced in this patch check if in AppErrors.properties
Line 10: files in 3 locations there are duplicate keys (same key with different
a/there are/contain/
Line 11: message).
Line 12:
Line 13: Change-Id: I143cb08908db4feed84ec2b94a17d89c1e522951
....................................................
File
frontend/webadmin/modules/userportal-gwtp/src/test/java/org/ovirt/engine/ui/userportal/DuplicateKeysTest.java
Line 12: public void testkDuplicateKeys() {
Line 13: String baseDir = System.getProperty("basedir");
//$NON-NLS-1$
Line 14: assumeNotNull(baseDir);
Line 15: String fileName = "AppErrors.properties"; //$NON-NLS-1$
Line 16: File file = new File(baseDir +
"/src/main/resources/org/ovirt/engine/ui/frontend/" + fileName); //$NON-NLS-1$
see comment in the dal test
Line 17:
DuplicateKeysCheck.checkDuplicateKeys(file.getAbsolutePath());
Line 18: }
Line 19:
....................................................
File
frontend/webadmin/modules/webadmin/src/test/java/org.ovirt.engine.ui.webadmin/DuplicateKeysTest.java
Line 12: public void testkDuplicateKeys() {
Line 13: String baseDir = System.getProperty("basedir");
//$NON-NLS-1$
Line 14: assumeNotNull(baseDir);
Line 15: String fileName = "AppErrors.properties"; //$NON-NLS-1$
Line 16: File file = new File(baseDir +
"/src/main/resources/org/ovirt/engine/ui/frontend/" + fileName); //$NON-NLS-1$
see comment in the dal test
Line 17:
DuplicateKeysCheck.checkDuplicateKeys(file.getAbsolutePath());
Line 18: }
Line 19:
--
To view, visit http://gerrit.ovirt.org/11343
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I143cb08908db4feed84ec2b94a17d89c1e522951
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alissa Bonas <abo...@redhat.com>
Gerrit-Reviewer: Alissa Bonas <abo...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Laszlo Hornyak <lhorn...@redhat.com>
Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
