[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