[PING] Re: RFR: 8165600: Convert internal logging tests to GTest

[Marcus Larsson]

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
>>>
>>
>