[rfc][icedtea-web] fixfor portalbank.no
Adam Domurad
adomurad at redhat.com
Tue Apr 30 13:24:44 PDT 2013
Commenting on tests now. Be warned this review is a little pedantic,
however I think a bit of review pain here and there forces one to write
better code :-)). I do like the patch's methodology though.
> diff -r e34db561b7b9 netx/net/sourceforge/jnlp/Version.java
> --- a/netx/net/sourceforge/jnlp/Version.java Mon Apr 29 16:24:37
> 2013 +0200
> +++ b/netx/net/sourceforge/jnlp/Version.java Tue Apr 30 16:12:59
> 2013 +0200
> @@ -308,42 +308,6 @@
>
> public String toString() {
> return versionString;
> - }
> -
> - /**
> - * Test.
> - */
> - /*
> - public static void main(String args[]) {
> - Version jvms[] = {
> - new Version("1.1* 1.3*"),
> - new Version("1.2+"),
> - };
> -
> - Version versions[] = {
> - new Version("1.1"),
> - new Version("1.1.8"),
> - new Version("1.2"),
> - new Version("1.3"),
> - new Version("2.0"),
> - new Version("1.3.1"),
> - new Version("1.2.1"),
> - new Version("1.3.1-beta"),
> - new Version("1.1 1.2"),
> - new Version("1.2 1.3"),
> - };
> -
> - for (int j = 0; j < jvms.length; j++) {
> - for (int v = 0; v < versions.length; v++) {
> - System.out.print( jvms[j].toString() + " " );
> - if (!jvms[j].matches(versions[v]))
> - System.out.print( "!" );
> - System.out.println( "matches " +
> versions[v].toString() );
> - }
> - }
> -
> - System.out.println("Test completed");
> - }
> - */
Strong +1 :-)
> + }
>
> }
> diff -r e34db561b7b9 netx/net/sourceforge/jnlp/cache/Resource.java
> --- a/netx/net/sourceforge/jnlp/cache/Resource.java Mon Apr 29
> 16:24:37 2013 +0200
> +++ b/netx/net/sourceforge/jnlp/cache/Resource.java Tue Apr 30
> 16:12:59 2013 +0200
> @@ -109,6 +109,8 @@
> synchronized (resources) {
> Resource resource = new Resource(location,
> requestVersion, updatePolicy);
>
> + //FIXME - url ignores port during its comparsion
s/comparsion/comparison/
> + //this may affect testsuites
[nit] 'test-suites'
Why don't you fix it ? Isn't it a simple matter of uncommenting
CacheUtil line 89 ?
(Although I think the true way to compare URL's is URL.toURI().equals...
And to that extent, I think we should use URI wherever possible. Out of
scope but worth considering if you do end up touching the caching code a
lot in the future.)
> int index = resources.indexOf(resource);
> if (index >= 0) { // return existing object
> Resource result = resources.get(index);
> diff -r e34db561b7b9 tests/netx/unit/net/sourceforge/jnlp/VersionTest.java
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/tests/netx/unit/net/sourceforge/jnlp/VersionTest.java Tue Apr
> 30 16:12:59 2013 +0200
> @@ -0,0 +1,99 @@
> +/*
> + Copyright (C) 2011 Red Hat, Inc.
> +
> + This file is part of IcedTea.
> +
> + IcedTea is free software; you can redistribute it and/or
> + modify it under the terms of the GNU General Public License as
> published by
> + the Free Software Foundation, version 2.
> +
> + IcedTea is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with IcedTea; see the file COPYING. If not, write to
> + the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> Boston, MA
> + 02110-1301 USA.
> +
> + Linking this library statically or dynamically with other modules is
> + making a combined work based on this library. Thus, the terms and
> + conditions of the GNU General Public License cover the whole
> + combination.
> +
> + As a special exception, the copyright holders of this library give you
> + permission to link this library with independent modules to produce an
> + executable, regardless of the license terms of these independent
> + modules, and to copy and distribute the resulting executable under
> + terms of your choice, provided that you also meet, for each linked
> + independent module, the terms and conditions of the license of that
> + module. An independent module is a module which is not derived from
> + or based on this library. If you modify this library, you may extend
> + this exception to your version of the library, but you are not
> + obligated to do so. If you do not wish to do so, delete this
> + exception statement from your version.
> + */
> +package net.sourceforge.jnlp;
> +
> +import org.junit.Assert;
> +import org.junit.Test;
> +
> +public class VersionTest {
> +
> + private static boolean[] results = {true,
> + true,
> + false,
> + true,
> + false,
> + true,
> + false,
> + true,
> + false,
> + false,
> + false,
> + false,
> + true,
> + true,
> + true,
> + true,
> + true,
> + true,
> + false,
> + true};
No :-)
> + private static Version jvms[] = {
> + new Version("1.1* 1.3*"),
> + new Version("1.2+"),};
Please instead format this into a positive (ie, should pass) & negative
(ie, shouldFail) list for both version strings.
> + private static Version versions[] = {
> + new Version("1.1"),
> + new Version("1.1.8"),
> + new Version("1.2"),
> + new Version("1.3"),
> + new Version("2.0"),
> + new Version("1.3.1"),
> + new Version("1.2.1"),
> + new Version("1.3.1-beta"),
> + new Version("1.1 1.2"),
> + new Version("1.2 1.3"),};
> +
> + @Test
> + public void iterateVersions() {
Please name this testMatches.
> +
> + int i = 0;
> + for (int j = 0; j < jvms.length; j++) {
> + for (int v = 0; v < versions.length; v++) {
> + i++;
> + String s = i + " " + jvms[j].toString() + " ";
Please name this variable.
> + if (!jvms[j].matches(versions[v])) {
> + s += "!";
> + }
> + s += "matches " + versions[v].toString();
> + //System.out.println(s);
Please remove the commented out println.
> + ServerAccess.logOutputReprint(s);
> + Assert.assertEquals(results[i - 1],
> jvms[j].matches(versions[v]));
> + }
> + }
> +
> +
> + }
> +}
> diff -r e34db561b7b9
> tests/netx/unit/net/sourceforge/jnlp/cache/ResourceTrackerTest.java
> ---
> a/tests/netx/unit/net/sourceforge/jnlp/cache/ResourceTrackerTest.java
> Mon Apr 29 16:24:37 2013 +0200
> +++
> b/tests/netx/unit/net/sourceforge/jnlp/cache/ResourceTrackerTest.java
> Tue Apr 30 16:12:59 2013 +0200
> @@ -1,54 +1,74 @@
> /* ResourceTrackerTest.java
> -Copyright (C) 2012 Red Hat, Inc.
> + Copyright (C) 2012 Red Hat, Inc.
>
> -This file is part of IcedTea.
> + This file is part of IcedTea.
>
> -IcedTea is free software; you can redistribute it and/or
> -modify it under the terms of the GNU General Public License as
> published by
> -the Free Software Foundation, version 2.
> + IcedTea is free software; you can redistribute it and/or
> + modify it under the terms of the GNU General Public License as
> published by
> + the Free Software Foundation, version 2.
>
> -IcedTea is distributed in the hope that it will be useful,
> -but WITHOUT ANY WARRANTY; without even the implied warranty of
> -MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> -General Public License for more details.
> + IcedTea is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + General Public License for more details.
>
> -You should have received a copy of the GNU General Public License
> -along with IcedTea; see the file COPYING. If not, write to
> -the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> Boston, MA
> -02110-1301 USA.
> + You should have received a copy of the GNU General Public License
> + along with IcedTea; see the file COPYING. If not, write to
> + the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> Boston, MA
> + 02110-1301 USA.
>
> -Linking this library statically or dynamically with other modules is
> -making a combined work based on this library. Thus, the terms and
> -conditions of the GNU General Public License cover the whole
> -combination.
> + Linking this library statically or dynamically with other modules is
> + making a combined work based on this library. Thus, the terms and
> + conditions of the GNU General Public License cover the whole
> + combination.
>
> -As a special exception, the copyright holders of this library give you
> -permission to link this library with independent modules to produce an
> -executable, regardless of the license terms of these independent
> -modules, and to copy and distribute the resulting executable under
> -terms of your choice, provided that you also meet, for each linked
> -independent module, the terms and conditions of the license of that
> -module. An independent module is a module which is not derived from
> -or based on this library. If you modify this library, you may extend
> -this exception to your version of the library, but you are not
> -obligated to do so. If you do not wish to do so, delete this
> -exception statement from your version.
> + As a special exception, the copyright holders of this library give you
> + permission to link this library with independent modules to produce an
> + executable, regardless of the license terms of these independent
> + modules, and to copy and distribute the resulting executable under
> + terms of your choice, provided that you also meet, for each linked
> + independent module, the terms and conditions of the license of that
> + module. An independent module is a module which is not derived from
> + or based on this library. If you modify this library, you may extend
> + this exception to your version of the library, but you are not
> + obligated to do so. If you do not wish to do so, delete this
> + exception statement from your version.
> */
> package net.sourceforge.jnlp.cache;
>
> +import java.io.ByteArrayOutputStream;
> +import java.io.File;
> +import java.io.IOException;
> +import java.io.PrintStream;
> import java.io.UnsupportedEncodingException;
> +import java.net.HttpURLConnection;
> import java.net.MalformedURLException;
> import java.net.URISyntaxException;
> import java.net.URL;
> -
> +import java.util.HashMap;
> +import net.sourceforge.jnlp.LoggingBottleneck;
> +import net.sourceforge.jnlp.ServerAccess;
> +import net.sourceforge.jnlp.ServerLauncher;
> +import net.sourceforge.jnlp.Version;
> +import net.sourceforge.jnlp.runtime.JNLPRuntime;
> import net.sourceforge.jnlp.util.UrlUtils;
> -
> +import org.junit.AfterClass;
> import org.junit.Assert;
> +import org.junit.BeforeClass;
> import org.junit.Test;
>
> -/** Test various corner cases of the parser */
> +/**
> + * Test various corner cases of the parser
> + */
Reformatting is nice, but this comment needs to go :-). (It seems copied
from ParserCornerCases, at any rate it doesn't belong here.)
> public class ResourceTrackerTest {
>
> + public static ServerLauncher s;
> + public static ServerLauncher s2;
Please name these variables. Why not serverLauncher1 & serverLauncher2 ?
> + private static PrintStream backupedStream;
'backedUpStream'
> + private static ByteArrayOutputStream nwErrorStream;
I'm not sure what 'nw' refers to. Consider renaming.
> + private static final String nameStub1 = "itw-server";
> + private static final String nameStub2 = "test-file";
> +
> @Test
> public void testNormalizeUrl() throws Exception {
> URL[] u = getUrls();
> @@ -64,7 +84,6 @@
> Assert.assertFalse("url " + i + " must be normalized (and
> so not equals) too normlaized url " + i, u[i].equals(n[i]));
normlaized -> normalized.
> }
> }
> -
> public static final int CHANGE_BORDER = 6;
>
> public static URL[] getUrls() throws MalformedURLException {
> @@ -97,4 +116,230 @@
> return n;
>
> }
> +
> + @BeforeClass
> + public static void redirectErr() {
> + if (backupedStream == null) {
> + backupedStream = System.err;
> + }
> + nwErrorStream = new ByteArrayOutputStream();
> + System.setErr(new PrintStream(nwErrorStream));
> +
> + }
> +
> + @AfterClass
> + public static void redirectErrBack() throws
> UnsupportedEncodingException {
> + ServerAccess.logErrorReprint(nwErrorStream.toString("utf-8"));
> + //System.err.println(nwErrorStream.toString("utf-8"));
Please remove this comment.
> + System.setErr(backupedStream);
> +
> + }
> +
> + @BeforeClass
> + public static void onDebug() {
> + JNLPRuntime.setDebug(true);
> + }
> +
> + @AfterClass
> + public static void offDebug() {
> + JNLPRuntime.setDebug(false);
> + }
> +
> + @BeforeClass
> + public static void startServer() throws IOException {
> + s =
> ServerAccess.getIndependentInstance(System.getProperty("java.io.tmpdir"),
> ServerAccess.findFreePort());
> + }
> +
> + @BeforeClass
> + public static void startServer2() throws IOException {
> + s2 =
> ServerAccess.getIndependentInstance(System.getProperty("java.io.tmpdir"),
> ServerAccess.findFreePort());
> + s2.setSupportsHead(false);
> + }
> +
> + @AfterClass
> + public static void stopServer() {
> + s.stop();
> + }
> +
> + @AfterClass
> + public static void stopServer2() {
> + s2.stop();
> + }
> +
> + @Test
> + public void getUrlResponseCodeTestWorkingHeadRequest() throws
> IOException {
> + redirectErr();
> + try {
> + File f = File.createTempFile(nameStub1, nameStub2);
> + int i =
> ResourceTracker.getUrlResponseCode(s.getUrl(f.getName()), new
> HashMap<String, String>(), "HEAD");
> + Assert.assertEquals(HttpURLConnection.HTTP_OK, i);
> + f.delete();
> + i =
> ResourceTracker.getUrlResponseCode(s.getUrl(f.getName()), new
> HashMap<String, String>(), "HEAD");
> + Assert.assertEquals(HttpURLConnection.HTTP_NOT_FOUND, i);
> + } finally {
> + redirectErrBack();
> + }
> + }
> +
> + @Test
> + public void getUrlResponseCodeTestNotWorkingHeadRequest() throws
> IOException {
> + redirectErr();
> + try {
> + File f = File.createTempFile(nameStub1, nameStub2);
> + int i =
> ResourceTracker.getUrlResponseCode(s2.getUrl(f.getName()), new
> HashMap<String, String>(), "HEAD");
> + Assert.assertEquals(HttpURLConnection.HTTP_NOT_IMPLEMENTED, i);
> + f.delete();
> + i =
> ResourceTracker.getUrlResponseCode(s2.getUrl(f.getName()), new
> HashMap<String, String>(), "HEAD");
> + Assert.assertEquals(HttpURLConnection.HTTP_NOT_IMPLEMENTED, i);
> + } finally {
> + redirectErrBack();
> + }
> + }
> +
> + @Test
> + public void
> getUrlResponseCodeTestGetReequestOnNotWorkingHeadRequest() throws
> IOException {
Reequest -> Request
> + redirectErr();
> + try {
> + File f = File.createTempFile(nameStub1, nameStub2);
> + int i =
> ResourceTracker.getUrlResponseCode(s2.getUrl(f.getName()), new
> HashMap<String, String>(), "GET");
> + Assert.assertEquals(HttpURLConnection.HTTP_OK, i);
> + f.delete();
> + i =
> ResourceTracker.getUrlResponseCode(s2.getUrl(f.getName()), new
> HashMap<String, String>(), "GET");
> + Assert.assertEquals(HttpURLConnection.HTTP_NOT_FOUND, i);
> + } finally {
> + redirectErrBack();
> + }
> + }
> +
> + @Test
> + public void getUrlResponseCodeTestGetRequest() throws IOException {
> + redirectErr();
> + try {
> + File f = File.createTempFile(nameStub1, nameStub2);
> + int i =
> ResourceTracker.getUrlResponseCode(s.getUrl(f.getName()), new
> HashMap<String, String>(), "GET");
> + Assert.assertEquals(HttpURLConnection.HTTP_OK, i);
> + f.delete();
> + i =
> ResourceTracker.getUrlResponseCode(s.getUrl(f.getName()), new
> HashMap<String, String>(), "GET");
> + Assert.assertEquals(HttpURLConnection.HTTP_NOT_FOUND, i);
> + } finally {
> + redirectErrBack();
> + }
> + }
> +
> + @Test
> + public void getUrlResponseCodeTestWrongRequest() throws IOException {
> + redirectErr();
> + try {
> + File f = File.createTempFile(nameStub1, nameStub2);
> + Exception exception = null;
> + try {
> + ResourceTracker.getUrlResponseCode(s.getUrl(f.getName()), new
> HashMap<String, String>(), "SomethingWrong");
> + } catch (Exception ex) {
> + exception = ex;
> + }
> + Assert.assertNotNull(exception);
> + exception = null;
> + f.delete();
> + try {
> + ResourceTracker.getUrlResponseCode(s.getUrl(f.getName()), new
> HashMap<String, String>(), "SomethingWrong");
> + } catch (Exception ex) {
> + exception = ex;
> + }
> + Assert.assertNotNull(exception);;
> + } finally {
> + redirectErrBack();
> + }
> +
> + }
> +
> + @Test
> + public void findBestUrltest() throws IOException {
> + redirectErr();
> + try {
> + File fs = File.createTempFile(nameStub1, nameStub2);
Please use 'file1' and 'file2' at the very least. 'fs' doesn't imply a
file to me.
> + File versiondFs = new File(fs.getParentFile(),
> fs.getName() + "-2.0");
> + versiondFs.createNewFile();
> +
> + File fs2 = File.createTempFile(nameStub1, nameStub2);
> + File versiondFs2 = new File(fs2.getParentFile(),
> fs2.getName() + "-2.0");
> + versiondFs2.createNewFile();
> +
> + ResourceTracker rt = new ResourceTracker();
> + Resource r1 =
> Resource.getResource(s.getUrl(fs.getName()), null, UpdatePolicy.NEVER);
> + Resource r2 =
> Resource.getResource(s2.getUrl(fs2.getName()), null, UpdatePolicy.NEVER);
> + Resource r3 =
> Resource.getResource(s.getUrl(versiondFs.getName()), new
> Version("1.0"), UpdatePolicy.NEVER);
> + Resource r4 =
> Resource.getResource(s2.getUrl(versiondFs2.getName()), new
> Version("1.0"), UpdatePolicy.NEVER);
> + asserrS1(rt.findBestUrl(r1));
> + asserrS1V1(rt.findBestUrl(r3));
> + asserrS2(rt.findBestUrl(r2));
> + asertS2V1(rt.findBestUrl(r4));
> +
> + fs.delete();
> + Assert.assertNull(rt.findBestUrl(r1));
> + asserrS1V1(rt.findBestUrl(r3));
> + asserrS2(rt.findBestUrl(r2));
> + asertS2V1(rt.findBestUrl(r4));
> +
> + versiondFs.delete();
> + Assert.assertNull(rt.findBestUrl(r1));
> + Assert.assertNull(rt.findBestUrl(r3));
> + asserrS2(rt.findBestUrl(r2));
> + asertS2V1(rt.findBestUrl(r4));
> +
> + versiondFs2.delete();
> + Assert.assertNull(rt.findBestUrl(r1));
> + Assert.assertNull(rt.findBestUrl(r3));
> + asserrS2(rt.findBestUrl(r2));
> + Assert.assertNull(rt.findBestUrl(r4));
> +
> +
> + fs2.delete();
> + Assert.assertNull(rt.findBestUrl(r1));
> + Assert.assertNull(rt.findBestUrl(r3));
> + Assert.assertNull(rt.findBestUrl(r2));
> + Assert.assertNull(rt.findBestUrl(r4));
[nit] Consider breaking this up into helper methods.
> + } finally {
> + redirectErrBack();
> + }
> +
> + }
> +
> + private void asserrS1(URL u) {
'asserr' -> 'assert'. This isn't understandable at first sight.
'assertOnServer1' or something? Same goes for below.
> + assertCommon(u);
> + assertPort(u, s.getPort());
> + }
> +
> + private void asserrS1V1(URL u) {
> + assertCommon(u);
> + assertPort(u, s.getPort());
> + assertVersion(u);
> + }
> +
> + private void asserrS2(URL u) {
> + assertCommon(u);
> + assertPort(u, s2.getPort());
> + }
> +
> + private void asertS2V1(URL u) {
> + assertCommon(u);
> + assertPort(u, s2.getPort());
> + assertVersion(u);
> + }
> +
> + private void assertCommon(URL u) {
[nit] Maybe 'assertCommonComponents'.
> + Assert.assertTrue(u.getProtocol().equals("http"));
> + Assert.assertTrue(u.getHost().equals("localhost"));
> + Assert.assertTrue(u.getPath().contains(nameStub1));
> + Assert.assertTrue(u.getPath().contains(nameStub2));
> + ServerAccess.logOutputReprint(u.toExternalForm());
> + }
> +
> + private void assertPort(URL u, int port) {
> + Assert.assertTrue(u.getPort() == port);
> + }
> +
> + private void assertVersion(URL u) {
> + Assert.assertTrue(u.getPath().contains("-2.0"));
> + Assert.assertTrue(u.getQuery().contains("version-id=1.0"));
> + }
> }
> diff -r e34db561b7b9
> tests/netx/unit/net/sourceforge/jnlp/util/HttpUtilsTest.java
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/tests/netx/unit/net/sourceforge/jnlp/util/HttpUtilsTest.java
> Tue Apr 30 16:12:59 2013 +0200
> @@ -0,0 +1,251 @@
> +/*
> + Copyright (C) 2012 Red Hat, Inc.
> +
> + This file is part of IcedTea.
> +
> + IcedTea is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 2, or (at your option)
> + any later version.
> +
> + IcedTea is distributed in the hope that it will be useful, but
> + WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with IcedTea; see the file COPYING. If not, write to the
> + Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> Boston, MA
> + 02110-1301 USA.
> +
> + Linking this library statically or dynamically with other modules is
> + making a combined work based on this library. Thus, the terms and
> + conditions of the GNU General Public License cover the whole
> + combination.
> +
> + As a special exception, the copyright holders of this library give you
> + permission to link this library with independent modules to produce an
> + executable, regardless of the license terms of these independent
> + modules, and to copy and distribute the resulting executable under
> + terms of your choice, provided that you also meet, for each linked
> + independent module, the terms and conditions of the license of that
> + module. An independent module is a module which is not derived from
> + or based on this library. If you modify this library, you may extend
> + this exception to your version of the library, but you are not
> + obligated to do so. If you do not wish to do so, delete this
> + exception statement from your version. */
> +package net.sourceforge.jnlp.util;
> +
> +import java.io.ByteArrayOutputStream;
> +import java.io.IOException;
> +import java.io.PrintStream;
> +import java.io.UnsupportedEncodingException;
> +import java.net.HttpURLConnection;
> +import java.net.URL;
> +import net.sourceforge.jnlp.ServerAccess;
> +import net.sourceforge.jnlp.ServerLauncher;
> +import org.junit.AfterClass;
> +import org.junit.Assert;
> +import org.junit.BeforeClass;
> +import org.junit.Test;
> +
> +public class HttpUtilsTest {
> +
> + private static PrintStream backupedStream;
'backedUpStream'
> + private static ByteArrayOutputStream nwErrorStream;
> +
> + public static void redirectErr() {
> + if (backupedStream == null) {
> + backupedStream = System.err;
> + }
> + nwErrorStream = new ByteArrayOutputStream();
> + System.setErr(new PrintStream(nwErrorStream));
> +
> + }
> +
> +
> + public static void redirectErrBack() throws
> UnsupportedEncodingException {
> + ServerAccess.logErrorReprint(nwErrorStream.toString("utf-8"));
> + //System.err.println(nwErrorStream.toString("utf-8"));
Please remove this comment.
> + System.setErr(backupedStream);
> +
> + }
> +
> + @Test
> + public void consumeAndCloseConnectionSilentlyTest() throws
> IOException {
> + redirectErr();
> + try{
> + Exception exception = null;
> + try {
> + HttpUtils.consumeAndCloseConnectionSilently(new
> HttpURLConnection(null) {
> + @Override
> + public void disconnect() {
> + throw new UnsupportedOperationException("Not
> supported yet.");
> + }
> +
> + @Override
> + public boolean usingProxy() {
> + throw new UnsupportedOperationException("Not
> supported yet.");
> + }
> +
> + @Override
> + public void connect() throws IOException {
> + throw new UnsupportedOperationException("Not
> supported yet.");
> + }
> + });
> + } catch (Exception ex) {
> + ServerAccess.logException(ex);
> + exception = ex;
> + }
> + Assert.assertNull("no exception expected - was" + exception,
> exception);
> +
> +
> +
> + try {
> + HttpUtils.consumeAndCloseConnectionSilently(new
> HttpURLConnection(new URL("http://localhost/blahblah")) {
> + @Override
> + public void disconnect() {
> + throw new UnsupportedOperationException("Not
> supported yet.");
> + }
> +
> + @Override
> + public boolean usingProxy() {
> + throw new UnsupportedOperationException("Not
> supported yet.");
> + }
> +
> + @Override
> + public void connect() throws IOException {
> + throw new UnsupportedOperationException("Not
> supported yet.");
> + }
> + });
> + } catch (Exception ex) {
> + ServerAccess.logException(ex);
> + exception = ex;
> + }
> + Assert.assertNull("no exception expected - was" + exception,
> exception);
> +
> + ServerLauncher s
> =ServerAccess.getIndependentInstance(System.getProperty("user.dir"),
> ServerAccess.findFreePort());
Please name this 'serverLauncher'.
> + try{
> + try {
> + HttpUtils.consumeAndCloseConnectionSilently(new
> HttpURLConnection(s.getUrl("blahblahblah")) {
> + @Override
> + public void disconnect() {
> +
> + }
> +
> + @Override
> + public boolean usingProxy() {
> + return false;
> + }
> +
> + @Override
> + public void connect() throws IOException {
> +
> + }
> + });
> + } catch (Exception ex) {
> + ServerAccess.logException(ex);
> + exception = ex;
> + }
> + Assert.assertNull("no exception expected - was" + exception,
> exception);
> + }finally{
> + try{
> + s.stop();
> + }catch(Exception ex){
> + ServerAccess.logException(ex);
> + }
> + }
> + }finally{
> + redirectErrBack();
> + }
> + }
> +
> + @Test
> + public void consumeAndCloseConnectionTest() throws IOException {
> + redirectErr();
> + try{
> + Exception exception = null;
> + try {
> + HttpUtils.consumeAndCloseConnection(new
> HttpURLConnection(null) {
> + @Override
> + public void disconnect() {
> + throw new UnsupportedOperationException("Not
> supported yet.");
> + }
> +
> + @Override
> + public boolean usingProxy() {
> + throw new UnsupportedOperationException("Not
> supported yet.");
> + }
> +
> + @Override
> + public void connect() throws IOException {
> + throw new UnsupportedOperationException("Not
> supported yet.");
> + }
> + });
> + } catch (Exception ex) {
> + ServerAccess.logException(ex);
> + exception = ex;
> + }
> + Assert.assertNotNull("exception expected - wasnt" +
> exception, exception);
> +
> +
> +
> + try {
> + HttpUtils.consumeAndCloseConnection(new
> HttpURLConnection(new URL("http://localhost/blahblah")) {
[nit] 'blahblah' isn't as clear as 'test' or 'testfolder'
> + @Override
> + public void disconnect() {
> + throw new UnsupportedOperationException("Not
> supported yet.");
> + }
> +
> + @Override
> + public boolean usingProxy() {
> + throw new UnsupportedOperationException("Not
> supported yet.");
> + }
> +
> + @Override
> + public void connect() throws IOException {
> + throw new UnsupportedOperationException("Not
> supported yet.");
> + }
Do you expect these 'Unsupported' exceptions to be the ones thrown ? I'd
prefer a custom RuntimeException in that case, with the appropriate assert.
> + });
> + } catch (Exception ex) {
> + ServerAccess.logException(ex);
> + exception = ex;
> + }
> + Assert.assertNotNull("exception expected - wasnt" +
> exception, exception);
> +
> + ServerLauncher s
> =ServerAccess.getIndependentInstance(System.getProperty("user.dir"),
> ServerAccess.findFreePort());
Please name this 'serverLauncher'.
> + try{
> + try {
> + HttpUtils.consumeAndCloseConnection(new
> HttpURLConnection(s.getUrl("blahblahblah")) {
[nit] 'blahblahblah' isn't as clear as something like 'testlocation'.
> + @Override
> + public void disconnect() {
> +
> + }
> +
> + @Override
> + public boolean usingProxy() {
> + return false;
> + }
> +
> + @Override
> + public void connect() throws IOException {
> +
> + }
> + });
[nit] You use the dummy HttpURLConnection more than once, consider
extracting it to a dummy helper class (still in same file is fine of
course).
> + } catch (Exception ex) {
> + ServerAccess.logException(ex);
> + exception = ex;
> + }
> + Assert.assertNotNull(" exception expected - wasnt" +
> exception, exception);
> + }finally{
[nit] formatting
> + try{
> + s.stop();
> + }catch(Exception ex){
> + ServerAccess.logException(ex);
> + }
> + }
> + }finally{
> + redirectErrBack();
> + }
> + }
> +}
> diff -r e34db561b7b9
> tests/netx/unit/net/sourceforge/jnlp/util/UrlUtilsTest.java
> --- a/tests/netx/unit/net/sourceforge/jnlp/util/UrlUtilsTest.java Mon
> Apr 29 16:24:37 2013 +0200
> +++ b/tests/netx/unit/net/sourceforge/jnlp/util/UrlUtilsTest.java Tue
> Apr 30 16:12:59 2013 +0200
> @@ -1,3 +1,40 @@
> +/*
> + Copyright (C) 2012 Red Hat, Inc.
> +
> +This file is part of IcedTea.
> +
> +IcedTea is free software; you can redistribute it and/or modify
> +it under the terms of the GNU General Public License as published by
> +the Free Software Foundation; either version 2, or (at your option)
> +any later version.
> +
> +IcedTea is distributed in the hope that it will be useful, but
> +WITHOUT ANY WARRANTY; without even the implied warranty of
> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> +General Public License for more details.
> +
> +You should have received a copy of the GNU General Public License
> +along with IcedTea; see the file COPYING. If not, write to the
> +Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> Boston, MA
> +02110-1301 USA.
> +
> +Linking this library statically or dynamically with other modules is
> +making a combined work based on this library. Thus, the terms and
> +conditions of the GNU General Public License cover the whole
> +combination.
> +
> +As a special exception, the copyright holders of this library give you
> +permission to link this library with independent modules to produce an
> +executable, regardless of the license terms of these independent
> +modules, and to copy and distribute the resulting executable under
> +terms of your choice, provided that you also meet, for each linked
> +independent module, the terms and conditions of the license of that
> +module. An independent module is a module which is not derived from
> +or based on this library. If you modify this library, you may extend
> +this exception to your version of the library, but you are not
> +obligated to do so. If you do not wish to do so, delete this
> +exception statement from your version. */
> +
> package net.sourceforge.jnlp.util;
>
> import static org.junit.Assert.assertEquals;
> diff -r e34db561b7b9
> tests/test-extensions/net/sourceforge/jnlp/ServerLauncher.java
> --- a/tests/test-extensions/net/sourceforge/jnlp/ServerLauncher.java
> Mon Apr 29 16:24:37 2013 +0200
> +++ b/tests/test-extensions/net/sourceforge/jnlp/ServerLauncher.java
> Tue Apr 30 16:12:59 2013 +0200
> @@ -57,7 +57,18 @@
> private final Integer port;
> private final File dir;
> private ServerSocket serverSocket;
> + private boolean supportsHead = true;
>
> + public void setSupportsHead(boolean supportsHead) {
> + this.supportsHead = supportsHead;
> + }
> +
> + public boolean isSupportsHead() {
> + return supportsHead;
> + }
> +
> +
> +
> public String getServerName() {
> return serverName;
> }
> @@ -102,10 +113,12 @@
> try {
> serverSocket = new ServerSocket(port);
> while (running) {
> - new TinyHttpdImpl(serverSocket.accept(), dir, port);
> + TinyHttpdImpl serverItself = new
> TinyHttpdImpl(serverSocket.accept(), dir, port,false);
[nit] I'm in favour of simply 'server' or 'serverImpl'.
> + serverItself.setSupportsHead(supportsHead);
> + serverItself.start();
> }
> } catch (Exception e) {
> - e.printStackTrace();
> + ServerAccess.logException(e);
> } finally {
> running = false;
> }
> diff -r e34db561b7b9
> tests/test-extensions/net/sourceforge/jnlp/TinyHttpdImpl.java
> --- a/tests/test-extensions/net/sourceforge/jnlp/TinyHttpdImpl.java
> Mon Apr 29 16:24:37 2013 +0200
> +++ b/tests/test-extensions/net/sourceforge/jnlp/TinyHttpdImpl.java
> Tue Apr 30 16:12:59 2013 +0200
> @@ -42,6 +42,7 @@
> import java.io.File;
> import java.io.FileInputStream;
> import java.io.InputStreamReader;
> +import java.net.HttpURLConnection;
> import java.net.Socket;
> import java.net.SocketException;
> import java.net.URLDecoder;
> @@ -55,25 +56,41 @@
> * When resource starts with XslowX prefix, then resouce (without XslowX)
> * is returned, but its delivery is delayed
> */
> -class TinyHttpdImpl extends Thread {
> +public class TinyHttpdImpl extends Thread {
>
> Socket c;
> private final File dir;
> private final int port;
> private boolean canRun = true;
> private static final String XSX = "/XslowX";
> -
> + private boolean supportsHead = true;
> +
> public TinyHttpdImpl(Socket s, File f, int port) {
> + this(s, f, port, true);
> + }
I think it's cleaner to set head-support in the constructor, and not
have a setter. This way you won't need a way to avoid the 'start()' method.
> + public TinyHttpdImpl(Socket s, File f, int port, boolean start) {
> c = s;
> this.dir = f;
> this.port = port;
> - start();
> + if (start){
> + start();
> + }
> }
>
> public void setCanRun(boolean canRun) {
> this.canRun = canRun;
> }
>
> + public void setSupportsHead(boolean supportsHead) {
> + this.supportsHead = supportsHead;
> + }
> +
> + public boolean isSupportsHead() {
'supportsHead' or 'doesSupportHead' are proper English. Or perhaps
'hasHeadSupport'.
> + return supportsHead;
> + }
> +
> +
> +
> public int getPort() {
> return port;
> }
> @@ -92,6 +109,11 @@
>
> boolean isGetRequest = s.startsWith("GET");
> boolean isHeadRequest = s.startsWith("HEAD");
> +
> + if (isHeadRequest && !isSupportsHead()){
> + o.writeBytes("HTTP/1.0
> "+HttpURLConnection.HTTP_NOT_IMPLEMENTED+" Not Implemented\n");
> + continue;
> + }
Ok.
>
> if (isGetRequest || isHeadRequest ) {
> StringTokenizer t = new StringTokenizer(s, " ");
> @@ -120,7 +142,7 @@
> } else if (p.toLowerCase().endsWith(".jar")) {
> content = ct + "application/x-jar\n";
> }
> - o.writeBytes("HTTP/1.0 200
> OK\nContent-Length:" + l + "\n" + content + "\n");
> + o.writeBytes("HTTP/1.0
> "+HttpURLConnection.HTTP_OK+" OK\nContent-Length:" + l + "\n" +
> content + "\n");
Ok.
>
> if (isHeadRequest) {
> continue; // Skip sending body
Thanks for testing! Sorry for so many style-related comments, but at
least for me one letter variable names (with scope more than a few
lines, and especially for static variables) require a _lot_ more time to
understand. Please consider it when writing code 8-). I won't insist too
much. I like the patch a lot from a technical standpoint.
Interesting study: http://arxiv.org/pdf/1304.5257v1.pdf
Thanks for the patch! I don't think the fix needs to wait on the tests,
although see my recommendations there.
-Adam
More information about the distro-pkg-dev
mailing list