Regression in itw from Tue Mar 26

[Adam Domurad]

[..snip..]
> oook so something like this?
>

Thanks for taking up the task :-)

> diff -r 0f85f68370a4 netx/net/sourceforge/jnlp/SecurityDesc.java
> --- a/netx/net/sourceforge/jnlp/SecurityDesc.java    Fri Apr 19 
> 16:41:33 2013 +0200
> +++ b/netx/net/sourceforge/jnlp/SecurityDesc.java    Tue Apr 23 
> 13:51:46 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();

If you keep NullJnlpFileException, at the very least please get rid of 
the NetBeans stub documentation & @author annotation from the 
NullJnlpFileException class.

But, I don't like this exception. I see it as too special-case for 
something that is *purely* a programmer error.
I am in favour of NullPointerException plainly, IllegalArgumentException 
or something that describes the intent rather than the specific mistake, 
eg "IllegalSecurityDescArgumentException" ,

> +        }
>          this.file = file;
>          this.type = type;
>          this.downloadHost = downloadHost;
> diff -r 0f85f68370a4 tests/netx/unit/net/sourceforge/jnlp/ParserBasic.java
> --- a/tests/netx/unit/net/sourceforge/jnlp/ParserBasic.java    Fri Apr 
> 19 16:41:33 2013 +0200
> +++ b/tests/netx/unit/net/sourceforge/jnlp/ParserBasic.java    Tue Apr 
> 23 13:51:46 2013 +0200
> @@ -40,6 +40,7 @@
>  import java.io.ByteArrayInputStream;
>  import java.io.InputStream;
>  import java.util.List;
> +import net.sourceforge.jnlp.runtime.CodeBaseClassLoaderTest;
>
>  import org.junit.After;
>  import org.junit.Assert;
> @@ -61,7 +62,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 
> CodeBaseClassLoaderTest.DummyJNLPFile(true), null, root, false, false);


[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 ?

>      }
>
>      @Test
> diff -r 0f85f68370a4 
> tests/netx/unit/net/sourceforge/jnlp/runtime/CodeBaseClassLoaderTest.java
> --- 
> a/tests/netx/unit/net/sourceforge/jnlp/runtime/CodeBaseClassLoaderTest.java 
>  Fri Apr 19 16:41:33 2013 +0200
> +++ 
> b/tests/netx/unit/net/sourceforge/jnlp/runtime/CodeBaseClassLoaderTest.java 
>  Tue Apr 23 13:51:46 2013 +0200
> @@ -70,7 +70,7 @@
>          }
>      }
>
> -    private class DummyJNLPFile extends JNLPFile {
> +    public static  class DummyJNLPFile extends JNLPFile {
>
>          final boolean haveSecurity;

Please get rid of this 'haveSecurity' flag. To me it indicates 
full-privileges/sandbox. We need to cover up the mistaken belief :-))

>
> @@ -252,68 +252,33 @@
>      @Test
>      public void testNullFileSecurityDescApplication() throws Exception {

I am in favour of *exactly one* test that has any mention of null-files. 
It should be a unit test for SecurityDesc. Please remove all trace of it 
otherwise, so not to confuse future maintainers.

>          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();

See above.

> +        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);

Step in the right direction, but see above.

> +        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);
> -        }
>      }
>  }

So in summary:
- I think we need a single test in SecurityDesc for the null parameter 
exception. A comment there explaining that tests ran into problems with 
a null JNLPFile in SecurityDesc would be good.
- If you agree, maybe move out DummyJNLPFile to test extension, 
otherwise what you did is OK (but some mocking helpers would be nice)
- I strongly dislike NullJnlpFileException
- I strongly dislike DummyJNLPFile#haveSecurity
- I strongly dislike testNullFileSecurityDesc & 
testNullFileSecurityDescApplet & testNullFileSecurityDescApplication.
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 :-)

Happy hacking,
-Adam