RFR(XS): 8165613: Convert TestKlass_test to Gtest

[Coleen Phillimore]

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