Regression in itw from Tue Mar 26
Jiri Vanek
jvanek at redhat.com
Wed Apr 24 05:34:53 PDT 2013
On 04/23/2013 03:52 PM, Adam Domurad wrote:
> [..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.
Keeping..... But fixed headers
>
> 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.
> programmer error.
> I am in favour of NullPointerException plainly, IllegalArgumentException or something that describes
> the intent rather than the specific mistake, eg "IllegalSecurityDescArgumentException" ,
The own type of excepytion is best recognition of it. Thats why I kept it, and also why I do not
want to rename it... No actual reason for it...
>
>> + }
>> 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 ?
Yah probably good idea, done.
Just refacrored. New features .. when they will be needed :)
>
>> }
>>
>> @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 :-))
done. To much true! to much pain!
>
>>
>> @@ -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:
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:)
> - If you agree, maybe move out DummyJNLPFile to test extension, otherwise what you did is OK (but
> some mocking helpers would be nice)
done
> - I strongly dislike NullJnlpFileException
Well not worthy of refactoring. And I do not want to remove.
> - 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...
> 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fixedRegression_2.patch
Type: text/x-patch
Size: 18965 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20130424/b4677342/fixedRegression_2.patch
More information about the distro-pkg-dev
mailing list