[rfc][icedtea-web] fixing failing unittests CodeBaseClassLoaderTest.java

Jiri Vanek jvanek at redhat.com
Thu Aug 9 04:31:26 PDT 2012


On 08/08/2012 10:25 PM, Deepak Bhole wrote:
> * Jiri Vanek <jvanek at redhat.com> [2012-08-08 08:35]:
>> On 08/07/2012 09:39 PM, Deepak Bhole wrote:
>>> * Jiri Vanek <jvanek at redhat.com> [2012-08-07 12:06]:
>>>> On 08/07/2012 05:53 PM, Deepak Bhole wrote:
>>>>> * Jiri Vanek <jvanek at redhat.com> [2012-08-01 14:29]:
>>>>>>
>>>>>> Those three tests have started to fail with NPE. I'm not sure if test is just badly written or
>>>>>> IcedTea web is broken (but as there are no regressions in reproducers then I'm turning to badly
>>>>>> written  test)
>>>>>>
>>>>>> Please note once more, I'm not sure if fix this test this way is correct, or we have really got
>>>>>> regression in CodeBaseClassLoader
>>>>>>
>>>>>> But IMHO we should not have jnlp file null for anything except pure applet.
>>>>>> If this is correct, then this null should be probably thrown from
>>>>>>    new SecurityDesc(HERE, SecurityDesc.SANDBOX_PERMISSIONS, null) this contcructor (after check for
>>>>>> javaws/applet) ??
>>>>>>
>>>>>
>>>>> Hi Jiri,
>>>>>
>>>>> I am not sure I fully understand. The test tries to find a class in the
>>>>> codebase classloader and then asserts that the exception is an instance
>>>>> of NPE?  Shouldn't it be an instance of CNFE when not found?
>>>>>
>>>>> findClass should never throw NPE IMO.
>>>>
>>>> The crucial difference in test is this:
>>>> -                return new SecurityDesc(null, SecurityDesc.SANDBOX_PERMISSIONS, null);
>>>> ---->>                                   -|-
>>>> +                return new SecurityDesc(this, SecurityDesc.SANDBOX_PERMISSIONS, null);
>>>>
>>>> The mock JNLPfile used in this class to test classlaoder is creating
>>>> the security description with null jnlp file. Imho it is not
>>>> possible in "real life".
>>>>
>>>> So I have replaced null by (imho correct) itself.
>>>>
>>>
>>> I am referring to this part:
>>
>> Ah, sorry.
>>>
>>>>>> +
>>>>>> +        Exception ex2=null;
>>>>>> +        try{
>>>>>> +        classLoader.findClass("foo");
>>>>>> +        }catch (Exception exx){
>>>>>> +            ex2=exx;
>>>>>> +        }
>>>>>> +        Assert.assertNotNull(ex2);
>>>>>> +        Assert.assertTrue(ex2 instanceof NullPointerException);
>>>>>> +
>>>>>> +        Exception ex=null;
>>>>>> +        try{
>>>>>> +        classLoader.findResource("net/sourceforge/jnlp/about/Main.class");
>>>>>> +        }catch (Exception exx){
>>>>>> +            ex=exx;
>>>>>> +        }
>>>>>> +        Assert.assertNotNull(ex);
>>>>>> +        Assert.assertTrue(ex instanceof NullPointerException);
>>>>>> +
>>>
>>> Why is an NPE expected and asserted when findClass should never be
>>> throwing an NPE?
>>
>> Because security manager is asked, and he is taking SecurityDesc
>> from JNLPfile. But it is null in this test.
>>
>> If you will ask me to prevent this (and so add if
>> securityDesc.getJnlpFile()==null){...) Then I must admit I'm not
>> sure what behaviour should be allowed - as jnlp file is quite
>> crucial for this.
>>
>> Does it sounds better now?
>
> If findClass() runs into any exception, it should also convert it to a
> CNFE and throw that IMO. Callers should not have to worry about
> implementation specific issues when dealing with a classloader.
>
I see your point now.

Will
  +        Assert.assertNotNull(ex);
  +        Assert.assertTrue(ex instanceof RuntimeException);

or jusr
 >>>>>> +        Assert.assertNotNull(ex);
 >>>>>> +        //Assert.assertTrue(ex instanceof NullPointerException);

Satisfy you?


I will look into catch of exception later. But I'm not sure whether  catch of NPE and rethrow as 
CNFE is an good idea. The cause is not "not found class" but the null jnlp file in security 
description. Wrapped as CNFE can be misleading.


J.




More information about the distro-pkg-dev mailing list