Frank Kobzik has posted comments on this change. Change subject: tools: Validate WebSocketProxy config value ......................................................................
Patch Set 1: -Verified (6 comments) http://gerrit.ovirt.org/#/c/37498/1/backend/manager/tools/src/main/java/org/ovirt/engine/core/config/entity/helper/WebSocketProxyLocationValueHelper.java File backend/manager/tools/src/main/java/org/ovirt/engine/core/config/entity/helper/WebSocketProxyLocationValueHelper.java: Line 23: } Line 24: } Line 25: Line 26: private boolean matchesHostColonPort(String s) { Line 27: return (s == null ? false : s.matches("\\S+:\\d+")); //$NON-NLS-1$ > Well, this allows too many false positives. If you really want to validate In the new PS I used URI class for validation host:port as both 'Engine' and 'Host' pass URI validation. Tell me if you want to validate them separately. Line 28: } http://gerrit.ovirt.org/#/c/37498/1/backend/manager/tools/src/test/java/org/ovirt/engine/core/config/entity/helper/WebSocketProxyLocationValueHelperTest.java File backend/manager/tools/src/test/java/org/ovirt/engine/core/config/entity/helper/WebSocketProxyLocationValueHelperTest.java: > If you write a test, then please test all valid inputs and also at least so Done Line 1: package org.ovirt.engine.core.config.entity.helper; Line 2: Line 3: import org.junit.Test; Line 4: import org.omg.CORBA.WStringValueHelper; Line 1: package org.ovirt.engine.core.config.entity.helper; Line 2: Line 3: import org.junit.Test; Line 4: import org.omg.CORBA.WStringValueHelper; > Please remove this import Done Line 5: Line 6: Line 7: import static org.junit.Assert.*; Line 8: Line 3: import org.junit.Test; Line 4: import org.omg.CORBA.WStringValueHelper; Line 5: Line 6: Line 7: import static org.junit.Assert.*; > 1. According to our conventions static imports should be placed before norm Done Line 8: Line 9: public class WebSocketProxyLocationValueHelperTest { Line 10: Line 11: @Test Line 22: Line 23: @Test Line 24: public void testValidateIncomplete() throws Exception { Line 25: WebSocketProxyLocationValueHelper helper = new WebSocketProxyLocationValueHelper(); Line 26: assertFalse(helper.validate(null, "myengine:").isOk()); > If user uses /etc/hosts on all machines, then "myengine" is a valid hostnam yes, but the this case is invalid since user must specify port as well. Line 27: } Line 28: Line 29: @Test Line 30: public void testValidateHostPort() throws Exception { Line 31: WebSocketProxyLocationValueHelper helper = new WebSocketProxyLocationValueHelper(); Line 32: assertTrue(helper.validate(null, "myengine.com:6100").isOk()); Line 33: } Line 34: Line 35: } > According to Checkstyle, you don't have a newline at the end of file. Pleas Done -- To view, visit http://gerrit.ovirt.org/37498 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I84b02d25a15762ba24544bd678328e6532d360ac Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Frank Kobzik <fkob...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Frank Kobzik <fkob...@redhat.com> Gerrit-Reviewer: Martin Peřina <mper...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <yzasl...@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
