RFR(XS): 8165613: Convert TestKlass_test to Gtest

[Kirill Zhaldybin]

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/

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