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

[Jiri Vanek]

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?
>
> Cheers,
> Deepak
>