[rfc][icedtea-web] fixing failing unittests CodeBaseClassLoaderTest.java
Deepak Bhole
dbhole at redhat.com
Thu Aug 9 06:29:00 PDT 2012
* Jiri Vanek <jvanek at redhat.com> [2012-08-09 07:30]:
> 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?
>
Close :) I think this will be better:
Assert.assertNotNull(ex);
Assert.assertTrue(ex instanceof ClassNotFoundException);
And we fix JNLPCLassLoader to throw the proper exception first and then
commit the test.
>
> 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.
>
>From the callers perspective, they don't know anything about JNLP, they
only know that it is a classloader and the class is either found or
not.. the caller should never have to worry about a RuntimeException
generally (i.e. for all cases where we know there is an RTE, we should
throw a CNFE). Depending on the code, the caller may be able to work
around a CFNE (a class may not be required) and throwing an NPE in such
a case would unnecessarily crash the caller.
Cheers,
Deepak
>
> J.
>
More information about the distro-pkg-dev
mailing list