[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: [PING] Re: RFR: 8165600: Convert internal logging tests to GTest
From:       Rachel Protacio <rachel.protacio () oracle ! com>
Date:       2016-09-26 20:00:22
Message-ID: a972f610-8fa2-8b4c-0993-4c70da0b2242 () oracle ! com
[Download RAW message or body]

Looks good to me!

Rachel


On 9/26/2016 10:27 AM, Marcus Larsson wrote:
> Looking for a second Reviewer for these (small and simple!) patches.
>
> Thanks,
> Marcus
>
>
> On 09/08/2016 04:00 PM, Marcus Larsson wrote:
>> Hi again,
>>
>> Patch has been split up into smaller parts, see separate email 
>> threads. Reusing this issue for the last few conversions.
>>
>> Updated webrev:
>> http://cr.openjdk.java.net/~mlarsson/8165600/webrev.01/
>>
>> Thanks,
>> Marcus
>>
>>
>> On 09/08/2016 02:23 PM, Marcus Larsson wrote:
>>> Hi Kirill,
>>>
>>> Thanks for looking at this.
>>>
>>>
>>> On 09/08/2016 02:02 PM, Kirill Zhaldybin wrote:
>>>> Marcus,
>>>>
>>>> Thank you for taking responsibility to convert logging tests to GTest!
>>>>
>>>> The change you made is pretty large. Would you consider to separate 
>>>> it to several smaller ones?
>>>> It would make review much simpler.
>>>
>>> Will do.
>>>
>>>>
>>>> A couple of fast comments:
>>>> 1. Naming: I think we have a naming convention to name tests 
>>>> <Tested Class>, <scenarion_in_snake_case>.
>>>
>>> Oh right, I've forgotten to rename them properly. Will fix!
>>>
>>>> 2. You used EXPECT_GT in TEST(logging, 
>>>> LogTagLevelExpression_combination_limit). As far as understand 
>>>> EXPECT_* checks do not abort current routine but I am unsure why 
>>>> you used EXPECT_GT in the last line of test.  I also see EXPECT_* 
>>>> as last line in several other places. Could you please let me know 
>>>> why you used this macro not ASSERT_*?
>>>
>>> It's just a habit. I've preferred to use EXPECT_* except in those 
>>> cases it makes no sense. Changing from EXPECT_ to ASSERT_ just 
>>> because it's the last assertion in a test seems pointless to me, and 
>>> it would cause more noise when/if we add more assertions to a test 
>>> in follow up patches.
>>>
>>> I'll post updated webrevs soon.
>>>
>>> Thanks,
>>> Marcus
>>>
>>>>
>>>> Thank you.
>>>>
>>>> Regards, Kirill
>>>>
>>>>
>>>> On 08.09.2016 10:47, Marcus Larsson wrote:
>>>>> Hi,
>>>>>
>>>>> Please review the following patch to convert existing internal VM 
>>>>> tests for UL to the GTest framework.
>>>>>
>>>>> Summary:
>>>>> Most of it is just a simple conversion from assert() to ASSERT_*() 
>>>>> macros. Tests which log to file, or otherwise change the log 
>>>>> configuration, have been modified to use the LogTestFixture for 
>>>>> automated log file handling and configuration cleanup.
>>>>>
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~mlarsson/8165600/webrev.00/
>>>>>
>>>>> Issue:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8165600
>>>>>
>>>>> Testing:
>>>>> JPRT
>>>>>
>>>>> Thanks,
>>>>> Marcus
>>>>
>>>
>>
>

[prev in list] [next in list] [prev in thread] [next in thread]