[9 + 8u] RFR - 8066612: Add a test that will call getDeclaredFields() on all classes and try to set them accessible.

[Daniel Fuchs]

On 04/12/14 14:02, Seán Coffey wrote:
> Thanks for driving efforts in this area Daniel. I think it's a very
> useful test and is bound to help test code coverage. If it's currently
> passing on all JPRT platforms, it's a good measure.

It seems to :-)

> Eventually I think we can bulk up the tests that can be run on the
> Iterable returned from your class search.
> At moment you just test Field.setAccessible.

Yes. If we change it later to test more, we will probably need to
change its name. I was already in lack of inspiration:
FieldSetAccessibleTest is not really a great name - but hopefully
it can do for now.

> In the future, I don't see any harm in adding all simple Field method
> calls so that any corner cases in custom classes like the original issue
> are caught. e.g Field.getDeclaredAnnotations(), Field.getModifiers(),
> Field.isEnumConstant() etc., etc. Some methods won't be much value add
> but they're not a cost either.

Agreed.

> Same argument for running through all Class methods, e.g
> Class.getDeclaredClasses(), Class.getDeclaredMethods(). As a result this
> test might eventually become more general in test goal and might live
> better one level up in "test/java/lang/Class/" - it can be moved when
> the time comes.

Agreed as well :-)

Here is a new revision of the webrev for 9 in which I have
incorporated David's comment:

http://cr.openjdk.java.net/~dfuchs/webrev_8066612/webrev-jdk9.01/

best regards,

-- daniel

>
> regards,
> Sean.
>
> On 04/12/2014 12:25, Daniel Fuchs wrote:
>> On 04/12/14 13:06, David Holmes wrote:
>>> Hi Daniel,
>>>
>>> On 4/12/2014 9:38 PM, Daniel Fuchs wrote:
>>>> Hi David,
>>>>
>>>> In fact I could use 'null' on JDK 9 as well.
>>>> My first version of the JDK 9 test was parsing over all the .jimage
>>>> files under <jdk>/lib/modules - which explain why I needed to
>>>> use the System class loader.
>>>>
>>>> Then I switched to only parsing the bootmodules.jimage - because
>>>> I noticed that the results where more coherent with what I had
>>>> observed on 8 & 7 - but I kept using the System class loader.
>>>>
>>>> I am not sure whether we want the test for 9 should iterate over
>>>> the three .jimage - or continue to test only the boot .jimage.
>>>>
>>>> I have updated the JDK 9 test (refreshed the webrev in place)
>>>> http://cr.openjdk.java.net/~dfuchs/webrev_8066612/webrev-jdk9.00/
>>>> and added support for possibly running the test in the two modes
>>>> (I added a 'test.boot.only' system property, true by default)
>>>> as well as additional traces to print the loaded classes by
>>>> defining loader at the end (test.list.classes, false by default).
>>>
>>> A couple of initial comments:
>>>
>>>   104     static ClassLoader getClassLoaderFor(String classFileName) {
>>>   105         if (restrictToBoot) return null; // only bootmodules
>>>   106         return ClassLoaders.systemClassLoader;
>>>   107     }
>>>
>>> I'm not clear the intent here. If it is to return a loader for which
>>> loadClass could be invoked then you can always just return the system
>>> loader - or just Class.forName. If it is meant to the return the
>>> expected defining loader then it isn't doing that as the extensions
>>> loader is not allowed for.
>>
>> The intent is to return the class loader that will be passed to
>> Class.forName( ). I've been fiddling & experimenting with this
>> test over 3 different platforms while trying to minimize the
>> differences - so that was my attempt at having a good place to
>> experiment with different strategy for loading classes.
>>
>>> Similarly for:
>>>
>>>   128         static ClassLoader getFor(String classFileName) {
>>>   129             return systemClassLoader;
>>>   130         }
>>
>> Oh - that's my mistake. In fact ClassLoader getClassLoaderFor(..)
>> was supposed to simply return ClassLoaders.getFor(...);
>> and all the code should be in ClassLoaders.getFor - my bad.
>>
>>> Minor nit - In:
>>>
>>> 135                     System.err.println("Unexpected loader for "+c+":
>>> "+c.getClassLoader());
>>>
>>> c.getClassLoader() can be replaced by cl. Also put spaces around the +
>>> operator.
>>
>> Good catch.
>>
>> Thanks a lot David! Have a good night! (that's quite late - isn't it?)
>>
>> -- daniel
>>
>>>
>>> David
>>> (signing off for the night)
>>>
>>>> Thanks for your question, it triggered me into looking deeper
>>>> into what was happening :-)
>>>>
>>>> best regards,
>>>>
>>>> -- daniel
>>>>
>>>> On 04/12/14 10:05, Daniel Fuchs wrote:
>>>>>>> The differences between 8 & 9 are limited to:
>>>>>>>
>>>>>>>     - ClassLoader:
>>>>>>>          - on 8 we use 'null' (BCL)
>>>>>>>          - on 9 we use the system class loader.
>>>>>>
>>>>>> I haven't seen anything in JEP 220, regarding modules, that indicates
>>>>>> that classes currently loaded  by the boot-loader will now be loaded
>>>>>> by the system classloader ???
>>>>>
>>>>> In [1] towards the end:
>>>>>
>>>>> [1] http://openjdk.java.net/jeps/220
>>>>>
>>>>> "The defining class loader of the types in some existing packages
>>>>>   will change. Existing code that makes assumptions about the class
>>>>>   loaders of these types might not work correctly."
>>>>>   (then there is a list of specific changes).
>>>>>
>>>>> This test looks up all class names in the image files and attempt
>>>>> to load the corresponding class. But as indicated in [1]
>>>>> some of these classes are now in the Boot CL, some in the
>>>>> Extension CL, and some in the Application CL.
>>>>>
>>>>> So the test uses the System CL to load each class - which ensures
>>>>> that the loading will be delegated to the appropriate ClassLoader.
>>>>>
>>>>> best regards,
>>>>>
>>>>> -- daniel
>>>>
>>
>