Regression in itw from Tue Mar 26
Adam Domurad
adomurad at redhat.com
Wed Apr 24 06:39:04 PDT 2013
On 04/24/2013 08:34 AM, Jiri Vanek wrote:
>> If you keep NullJnlpFileException, at the very least please get rid
>> of the NetBeans stub
>> documentation & @author annotation from the NullJnlpFileException class.
>
> Keeping..... But fixed headers
OK
>>
>> But, I don't like this exception. I see it as too special-case for
>> something that is *purely* a
> Someone can do this error again. This is way how to catch it early.
To be clear, I wasn't arguing to have no exception at all. Anyway, fine
as is.
>>
>> [open for discussion] Hm. I think DummyJNLPFile is a useful enough
>> pattern that it could be a test
>> extension. Thoughts ?
>> Maybe if we had MockedEmptyJNLPFile & MockedOneJarJNLPFile ? Or,
>> maybe better is -> DummyJNLPFile
>> and DummyJNLPFileWithJar ?
>
> Yah probably good idea, done.
> Just refacrored. New features .. when they will be needed :)
Agreed
>>
>> So in summary:
> ook
>
>> - I think we need a single test in SecurityDesc for the null
>> parameter exception. A comment there
> done
>
>> explaining that tests ran into problems with a null JNLPFile in
>> SecurityDesc would be good.
> eh...O:)
OK
>
>> - I strongly dislike NullJnlpFileException
> Well not worthy of refactoring. And I do not want to remove.
Fair enough.
>
>> - I strongly dislike DummyJNLPFile#haveSecurity
> sure. Removed
>
>> - I strongly dislike testNullFileSecurityDesc &
>> testNullFileSecurityDescApplet &
>> testNullFileSecurityDescApplication.
>
> I really do not want to remove them. They already found a problem. Can
> do it even later...
No. They *were* the problem; very different. I still want them gone :-).
But I won't insist.
>> See a pattern ? I think it's less confusing for future eyes. It
>> certainly lead me to believe that
>> this was somehow a valid case (and even you forgot original reason
>> for the tests :-).
>>
>> Sorry if I come across as demanding & Feel free to disagree of course
>> :-)
>
> done ;) J
Thanks for disagreeing :-)
>
>
> J.
Comments in-line
> diff -r b912e91204b1 netx/net/sourceforge/jnlp/NullJnlpFileException.java
> --- a/netx/net/sourceforge/jnlp/NullJnlpFileException.java Tue Apr
> 23 13:59:20 2013 -0400
> +++ b/netx/net/sourceforge/jnlp/NullJnlpFileException.java Wed Apr
> 24 14:20:04 2013 +0200
> @@ -1,14 +1,42 @@
> package net.sourceforge.jnlp;
>
> -/*
> - * To change this template, choose Tools | Templates
> - * and open the template in the editor.
> +/*
> +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, 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.
> */
>
> -/**
> - *
> - * @author jvanek
> - */
> public class NullJnlpFileException extends NullPointerException {
[nit] What value does extending from NPE provide ?
>
> public NullJnlpFileException() {
> diff -r b912e91204b1 netx/net/sourceforge/jnlp/SecurityDesc.java
> --- a/netx/net/sourceforge/jnlp/SecurityDesc.java Tue Apr 23
> 13:59:20 2013 -0400
> +++ b/netx/net/sourceforge/jnlp/SecurityDesc.java Wed Apr 24
> 14:20:04 2013 +0200
> @@ -149,6 +149,9 @@
> * @param downloadHost the download host (can always connect to)
> */
> public SecurityDesc(JNLPFile file, Object type, String
> downloadHost) {
> + if (file == null) {
> + throw new NullJnlpFileException();
> + }
> this.file = file;
> this.type = type;
> this.downloadHost = downloadHost;
> diff -r b912e91204b1 tests/netx/unit/net/sourceforge/jnlp/ParserBasic.java
> --- a/tests/netx/unit/net/sourceforge/jnlp/ParserBasic.java Tue Apr
> 23 13:59:20 2013 -0400
> +++ b/tests/netx/unit/net/sourceforge/jnlp/ParserBasic.java Wed Apr
> 24 14:20:04 2013 +0200
> @@ -40,6 +40,8 @@
> import java.io.ByteArrayInputStream;
> import java.io.InputStream;
> import java.util.List;
> +import net.sourceforge.jnlp.runtime.CodeBaseClassLoaderTest;
> +import net.sourceforge.jnlp.mock.DummyJNLPFile;
>
> import org.junit.After;
> import org.junit.Assert;
> @@ -61,7 +63,7 @@
> }
> InputStream jnlpStream =
> cl.getResourceAsStream("net/sourceforge/jnlp/basic.jnlp");
> root = Parser.getRootNode(jnlpStream);
> - parser = new Parser(null, null, root, false, false);
> + parser = new Parser(new DummyJNLPFile(), null, root, false,
> false);
> }
>
> @Test
> diff -r b912e91204b1
> tests/netx/unit/net/sourceforge/jnlp/SecurityDescTest.java
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/tests/netx/unit/net/sourceforge/jnlp/SecurityDescTest.java Wed
> Apr 24 14:20:04 2013 +0200
> @@ -0,0 +1,66 @@
> +/*
> + 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, 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 net.sourceforge.jnlp.mock.DummyJNLPFile;
> +import org.junit.Assert;
> +import org.junit.Test;
> +
> +
> +public class SecurityDescTest {
> +
> + @Test
> + public void testNotNullJnlpFile(){
> + SecurityDesc securityDesc = new SecurityDesc(new
> DummyJNLPFile(), SecurityDesc.SANDBOX_PERMISSIONS, "hey!");
> + Assert.assertNotNull("securityDesc should not be ",securityDesc);
What is this, a Java sanity test ? Well, I found it a bit funny :-) I
assume you wanted to check the file, but there's no getter, so I'm in
favour of getting rid of this.
> +
> + }
> +
> + @Test
> + public void testNullJnlpFile(){
> + Exception ex = null;
> + try{
> + SecurityDesc securityDesc = new SecurityDesc(null,
> SecurityDesc.SANDBOX_PERMISSIONS, "hey!");
> + }catch(Exception eex){
> + ex=eex;
> + }
> + Assert.assertNotNull("Exception should not be null",ex);
> + Assert.assertTrue("Exception should be
> "+NullJnlpFileException.class.getName(), ex instanceof
> NullJnlpFileException);
> +
> + }
> +
> +}
> diff -r b912e91204b1
> tests/netx/unit/net/sourceforge/jnlp/runtime/CodeBaseClassLoaderTest.java
> ---
> a/tests/netx/unit/net/sourceforge/jnlp/runtime/CodeBaseClassLoaderTest.java
> Tue Apr 23 13:59:20 2013 -0400
> +++
> b/tests/netx/unit/net/sourceforge/jnlp/runtime/CodeBaseClassLoaderTest.java
> Wed Apr 24 14:20:04 2013 +0200
> @@ -1,41 +1,42 @@
> /* CodeBaseClassLoaderTest.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.runtime;
>
> +import net.sourceforge.jnlp.mock.DummyJNLPFile;
> import static org.junit.Assert.assertFalse;
> import static org.junit.Assert.assertTrue;
>
> @@ -47,6 +48,7 @@
> import net.sourceforge.jnlp.NullJnlpFileException;
> import net.sourceforge.jnlp.ResourcesDesc;
> import net.sourceforge.jnlp.SecurityDesc;
> +import net.sourceforge.jnlp.SecurityDescTest;
> import net.sourceforge.jnlp.ServerAccess;
> import net.sourceforge.jnlp.runtime.JNLPClassLoader.CodeBaseClassLoader;
> import net.sourceforge.jnlp.annotations.Bug;
> @@ -58,45 +60,6 @@
>
> public class CodeBaseClassLoaderTest {
>
> - private static final URL JAR_URL;
> - private static final URL CODEBASE_URL;
> -
> - static {
> - try {
> - JAR_URL = new
> URL("http://icedtea.classpath.org/netx/about.jar");
> - CODEBASE_URL = new URL("http://icedtea.classpath.org/netx/");
> - } catch (Exception ex) {
> - throw new RuntimeException(ex);
> - }
> - }
> -
> - private class DummyJNLPFile extends JNLPFile {
> -
> - final boolean haveSecurity;
> -
> - public DummyJNLPFile(boolean haveSecurity) {
> - this.haveSecurity = haveSecurity;
> - }
> -
> - @Override
> - public ResourcesDesc getResources() {
> - return new ResourcesDesc(null, new Locale[0], new
> String[0], new String[0]);
> - }
> -
> - @Override
> - public URL getCodeBase() {
> - return CODEBASE_URL;
> - }
> -
> - @Override
> - public SecurityDesc getSecurity() {
> - if (haveSecurity) {
> - return new SecurityDesc(this,
> SecurityDesc.SANDBOX_PERMISSIONS, null);
> - } else {
> - return new SecurityDesc(null,
> SecurityDesc.SANDBOX_PERMISSIONS, null);
> - }
> - }
> - };
> private static final String isWSA = "isWebstartApplication";
>
> static void setStaticField(Field field, Object newValue) throws
> Exception {
> @@ -159,10 +122,10 @@
> }
>
> public void testResourceCaching(String r, boolean shouldExists)
> throws Exception {
> - JNLPFile dummyJnlpFile = new DummyJNLPFile(true);
> + JNLPFile dummyJnlpFile = new DummyJNLPFile();
>
> JNLPClassLoader parent = new JNLPClassLoader(dummyJnlpFile,
> null);
> - CodeBaseClassLoader classLoader = new CodeBaseClassLoader(new
> URL[]{JAR_URL, CODEBASE_URL}, parent);
> + CodeBaseClassLoader classLoader = new CodeBaseClassLoader(new
> URL[]{DummyJNLPFile.JAR_URL, DummyJNLPFile.CODEBASE_URL}, parent);
>
> int level = 10;
> if (shouldExists) {
> @@ -228,19 +191,18 @@
> }
>
> public void testParentClassLoaderIsAskedForClasses() throws
> Exception {
> - JNLPFile dummyJnlpFile = new DummyJNLPFile(true);
> + JNLPFile dummyJnlpFile = new DummyJNLPFile();
>
> final boolean[] parentWasInvoked = new boolean[1];
>
> JNLPClassLoader parent = new JNLPClassLoader(dummyJnlpFile,
> null) {
> -
> @Override
> protected Class<?> findClass(String name) throws
> ClassNotFoundException {
> parentWasInvoked[0] = true;
> throw new ClassNotFoundException(name);
> }
> };
> - CodeBaseClassLoader classLoader = new CodeBaseClassLoader(new
> URL[]{JAR_URL, CODEBASE_URL}, parent);
> + CodeBaseClassLoader classLoader = new CodeBaseClassLoader(new
> URL[]{DummyJNLPFile.JAR_URL, DummyJNLPFile.CODEBASE_URL}, parent);
> try {
> classLoader.findClass("foo");
> assertFalse("should not happen", true);
> @@ -252,68 +214,38 @@
> @Test
> public void testNullFileSecurityDescApplication() throws Exception {
> setWSA();
> - testNullFileSecurityDesc();
> + Exception ex = null;
> + try {
> + testNullFileSecurityDesc();
> + } catch (Exception exx) {
> + ex = exx;
> + }
> + Assert.assertTrue("was expected exception", ex != null);
> + Assert.assertTrue("was expected " +
> NullJnlpFileException.class.getName(), ex instanceof
> NullJnlpFileException);
> }
>
> @Test
> @Remote
> public void testNullFileSecurityDescApplet() throws Exception {
> setApplet();
> - testNullFileSecurityDesc();
> + Exception ex = null;
> + try {
> + testNullFileSecurityDesc();
> + } catch (Exception exx) {
> + ex = exx;
> + }
> + Assert.assertTrue("was expected exception", ex != null);
> + Assert.assertTrue("was expected " +
> NullJnlpFileException.class.getName(), ex instanceof
> NullJnlpFileException);
> }
>
> public void testNullFileSecurityDesc() throws Exception {
> - JNLPFile dummyJnlpFile = new DummyJNLPFile(false);
> + JNLPFile dummyJnlpFile = new DummyJNLPFile() {
> + @Override
> + public SecurityDesc getSecurity() {
> + return new SecurityDesc(null,
> SecurityDesc.SANDBOX_PERMISSIONS, null);
> + }
> + };
> + JNLPClassLoader parent = new JNLPClassLoader(dummyJnlpFile,
> null);
>
> - JNLPClassLoader parent = new JNLPClassLoader(dummyJnlpFile,
> null);
> - CodeBaseClassLoader classLoader = new CodeBaseClassLoader(new
> URL[]{JAR_URL, CODEBASE_URL}, parent);
> -
> - Exception ex = null;
> - try {
> - classLoader.findClass("foo");
> - } catch (Exception exx) {
> - ex = exx;
> - ServerAccess.logException(ex);
> - }
> - Assert.assertNotNull(ex);
> - Assert.assertTrue(ex instanceof ClassNotFoundException);
> -
> -
> - //search dor resources is not relvant to null jnlp file for
> applets
> - ex = null;
> - URL res = null;
> - try {
> - //not cached
> - res =
> classLoader.findResource("net/sourceforge/jnlp/about/resources/notes.html");
> - } catch (Exception exx) {
> - ex = exx;
> - ServerAccess.logException(ex);
> - }
> - if (JNLPRuntime.isWebstartApplication()) {
> - Assert.assertNull(res);
> - Assert.assertNotNull(ex);
> - Assert.assertTrue(ex instanceof NullJnlpFileException);
> - } else {
> - Assert.assertNull(ex);
> - Assert.assertNotNull(res);
> - }
> -
> - ex = null;
> - res = null;
> - try {
> - //now cached
> - res =
> classLoader.findResource("net/sourceforge/jnlp/about/resources/notes.html");
> - } catch (Exception exx) {
> - ex = exx;
> - ServerAccess.logException(ex);
> - }
> - if (JNLPRuntime.isWebstartApplication()) {
> - Assert.assertNotNull(ex);
> - Assert.assertTrue(ex instanceof NullJnlpFileException);
> - Assert.assertNull(res);
> - } else {
> - Assert.assertNull(ex);
> - Assert.assertNotNull(res);
> - }
> }
> }
> diff -r b912e91204b1
> tests/netx/unit/net/sourceforge/jnlp/runtime/JNLPClassLoaderTest.java
> ---
> a/tests/netx/unit/net/sourceforge/jnlp/runtime/JNLPClassLoaderTest.java Tue
> Apr 23 13:59:20 2013 -0400
> +++
> b/tests/netx/unit/net/sourceforge/jnlp/runtime/JNLPClassLoaderTest.java Wed
> Apr 24 14:20:04 2013 +0200
> @@ -58,6 +58,7 @@
> import net.sourceforge.jnlp.LaunchException;
> import net.sourceforge.jnlp.ResourcesDesc;
> import net.sourceforge.jnlp.SecurityDesc;
> +import net.sourceforge.jnlp.SecurityDescTest;
> import net.sourceforge.jnlp.ServerAccess;
> import net.sourceforge.jnlp.Version;
> import net.sourceforge.jnlp.cache.UpdatePolicy;
> diff -r b912e91204b1
> tests/test-extensions/net/sourceforge/jnlp/mock/DummyJNLPFile.java
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++
> b/tests/test-extensions/net/sourceforge/jnlp/mock/DummyJNLPFile.java
> Wed Apr 24 14:20:04 2013 +0200
> @@ -0,0 +1,48 @@
> +/*
> + * To change this template, choose Tools | Templates
> + * and open the template in the editor.
> + */
- these lines
+ copyright header
> +package net.sourceforge.jnlp.mock;
> +
> +import java.net.URL;
> +import java.util.Locale;
> +import net.sourceforge.jnlp.JNLPFile;
> +import net.sourceforge.jnlp.ResourcesDesc;
> +import net.sourceforge.jnlp.SecurityDesc;
> +
> +/**
> + *
> + * @author jvanek
> + */
Snip these lines
Maybe you could configure NetBeans to add the Red Hat copyright header
and not add these lines ? :-))
> +public class DummyJNLPFile extends JNLPFile {
> +
> +
> + public static final URL JAR_URL;
> + public static final URL CODEBASE_URL;
> +
> + static {
> + try {
> + JAR_URL = new
> URL("http://icedtea.classpath.org/netx/about.jar");
> + CODEBASE_URL = new URL("http://icedtea.classpath.org/netx/");
> + } catch (Exception ex) {
> + throw new RuntimeException(ex);
> + }
> + }
> +
> +
> + @Override
> + public ResourcesDesc getResources() {
> + return new ResourcesDesc(null, new Locale[0], new String[0],
> new String[0]);
> + }
> +
> + @Override
> + public URL getCodeBase() {
> + return CODEBASE_URL;
> + }
> +
> + @Override
> + public SecurityDesc getSecurity() {
> + return new SecurityDesc(this,
> SecurityDesc.SANDBOX_PERMISSIONS, null);
> + }
> +
> +}
Almost acceptable :-)
Happy hacking,
-Adam
More information about the distro-pkg-dev
mailing list