[rfc][icedtea-web] fixfor portalbank.no
Jiri Vanek
jvanek at redhat.com
Thu May 2 02:49:37 PDT 2013
...
>
> 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.
agree
> Out of scope but worth considering if you do end up touching
> the caching code a lot in the future.)
I'm aware of the part making this happening. But it is out of scope of this patch.
I will refactor a lo from this for 1.5
So no op now.
Also the port confusion is really mostly related to tests...
However, thanx for hint and this will be remembered
>
>> 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 :-)
yes... I'm not going to lost more time on this.
For now imho the test extraction is enough. More work can come in close future.
>
>> + 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.
From now down.. many nits and typos.. I hope I have fixed them alll
>
>> +
>> + 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
taking this personally :DD
>
> Thanks for the patch! I don't think the fix needs to wait on the tests, although see my
> recommendations there.
> -Adam
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fixedPortalBank-tests.diff
Type: text/x-patch
Size: 35944 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20130502/0c695fbc/fixedPortalBank-tests.diff
More information about the distro-pkg-dev
mailing list