RFR(XS): 8165613: Convert TestKlass_test to Gtest

[Kirill Zhaldybin]

Coleen,

Thank you for review!

Regards, Kirill

On 21.09.2016 16:57, Coleen Phillimore wrote:
>
> This looks great!   Thank you for making the changes and doing this 
> work to convert these tests.
>
> Coleen
>
>
> On 9/16/16 1:02 PM, Kirill Zhaldybin wrote:
>> Coleen,
>>
>> Thank you for comments.
>>
>> Here are a new WebRev: 
>> http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8165613/webrev.03/
>>
>> Could you please let me know your opinion?
>>
>> Regards, Kirill
>>
>> On 15.09.2016 20:34, Coleen Phillimore wrote:
>>>
>>>
>>> On 9/15/16 1:31 PM, Kirill Zhaldybin wrote:
>>>> Coleen,
>>>>
>>>> Thank you for detailed research!
>>>>
>>>> We already have test_instanceClass.cpp.
>>>> I added converted tests to this file.
>>>>
>>>> Here are a new WebRev: 
>>>> http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8165613/webrev.02/
>>>
>>> I think this looks good except that
>>>
>>>
>>>     test/native/runtime/test_instanceKlass.cpp
>>>
>>> (sorry for the bold) belongs in
>>>
>>> test/native/oops/test_instanceKlass.cpp because instanceKlass.cpp is 
>>> in directory oops.
>>>
>>> Also, why does it sometimes have a capital I and this one has a 
>>> lower case i ?
>>>
>>>   TEST_VM(instanceKlass, null_symbol) {
>>> and your new tests:
>>>
>>> +TEST_VM(InstanceKlass, class_loader_class) {
>>>
>>> thanks,
>>> Coleen
>>>
>>> <javascript:print()>
>>>> Could you please let me know your opinion?
>>>>
>>>> Thank you.
>>>>
>>>> Regards, Kirill
>>>>
>>>>
>>>>
>>>> On 15.09.2016 19:06, Coleen Phillimore wrote:
>>>>>
>>>>>
>>>>> On 9/15/16 10:53 AM, Kirill Zhaldybin wrote:
>>>>>> Coleen,
>>>>>>
>>>>>> Thank you for reviewing the fix!
>>>>>>
>>>>>> Here are a new WebRev: 
>>>>>> http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8165613/webrev.01/
>>>>>>
>>>>>> I changed the test name and moved it to the appropriate location.
>>>>>>
>>>>>> Could you please let me know your opinion?
>>>>>
>>>>> I think I figured out the point of this test.  It's apparently 
>>>>> trying to add a test for the 
>>>>> InstanceKlass::is_class_loader_instance_klass() function.
>>>>>
>>>>> So I have to admit that I was wrong to have you move it to 
>>>>> systemDictionary, since the function that it's trying to test is 
>>>>> in InstanceKlass, so it should belong to InstanceKlass.
>>>>>
>>>>>    28 TEST_VM(SystemDictionary, class_loader_class) {
>>>>>    29   Klass* klass = SystemDictionary::ClassLoader_klass();
>>>>>    30   ASSERT_TRUE(klass->is_instance_klass());
>>>>>    31   ASSERT_TRUE(InstanceKlass::cast(klass)->is_class_loader_instance_klass());
>>>>>    32 }
>>>>>    33
>>>>>    34 TEST_VM(SystemDictionary, string_klass) {
>>>>>    35   Klass* klass = SystemDictionary::String_klass();
>>>>>    36   ASSERT_TRUE(!klass->is_instance_klass() ||
>>>>>    37           !InstanceKlass::cast(klass)->is_class_loader_instance_klass());
>>>>>
>>>>> Since SystemDictionary::x_klass() now returns InstanceKlass, this can be written as:
>>>>>
>>>>>    28 TEST_VM(InstanceKlass, class_loader_class) {
>>>>>    29   InstanceKlass* klass = SystemDictionary::ClassLoader_klass();
>>>>>    31   ASSERT_TRUE(klass->is_class_loader_instance_klass());
>>>>>    32 }
>>>>>    33
>>>>>    34 TEST_VM(InstanceKlass, string_klass) {
>>>>>    35   InstanceKlass* klass = SystemDictionary::String_klass();
>>>>>    36   ASSERT_TRUE(!klass->is_class_loader_instance_klass());
>>>>> And then add a comment that this is the point of the test since I 
>>>>> had to go back to history to figure this out. I'm sorry that I 
>>>>> didn't research this when I made my original comments. Thanks, Coleen
>>>>>> Thank you. Regards, Kirill On 12.09.2016 15:28, Coleen Phillimore 
>>>>>> wrote:
>>>>>>> This looks good but I'm confused about something: 
>>>>>>> http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8165613/webrev.00/test/native/oops/test_klass.cpp.html 
>>>>>>>   28 TEST_VM(SystemDictionary, class_loader_class) { It seems 
>>>>>>> like making the tests test the SystemDictionary is more 
>>>>>>> appropriate than InstanceKlass, which I think this does.  If so, 
>>>>>>> can you move the test file to the directory 
>>>>>>> test/native/classfile/test_systemDictionary.cpp (or whatever the 
>>>>>>> convention is), if it exists? Thanks, Coleen On 9/12/16 8:15 AM, 
>>>>>>> Kirill Zhaldbybin wrote:
>>>>>>>> Robert, Thank you for review! Regards, Kirill On 09/12/2016 
>>>>>>>> 11:35 AM, Robbin Ehn wrote:
>>>>>>>>> Looks good. Thanks! /Robbin (not a 'R'eviewer) On 09/08/2016 
>>>>>>>>> 04:48 PM, Kirill Zhaldybin wrote:
>>>>>>>>>> Dear all, Could you please review this fix for 8165613? 
>>>>>>>>>> WebRev: 
>>>>>>>>>> http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8165613/webrev.00/ 
>>>>>>>>>> CR: https://bugs.openjdk.java.net/browse/JDK-8165613 Thank 
>>>>>>>>>> you. Regards, Kirill