RFR: 8165600: Convert internal logging tests to GTest

Marcus Larsson marcus.larsson at oracle.com
Thu Sep 8 12:23:41 UTC 2016

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.


> 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:
>> Thanks,
>> Marcus

More information about the hotspot-runtime-dev mailing list