RFR(S) : 8157728 : Covert GCTimer_test to GTest

[JC Beyler]

Hi Igor,

I was looking at this and it looks good to me (not a reviewer of course)
but I noticed a few nits:

-
http://cr.openjdk.java.net/~iignatyev//8157728/webrev.00/src/hotspot/share/gc/shared/gcTimer.cpp.udiff.html
   - Seems like you could remove a few more lines there because now you
have empty lines at the end of the file

-
http://cr.openjdk.java.net/~iignatyev//8157728/webrev.00/test/hotspot/gtest/gc/shared/test_gcTimer.cpp.html
   - Should the copyright year 2001 be here?

   - Indentation is a bit off:
       - GCTimerTest has 2 spaces for public and 4 for methods
       - TimePartitionPhasesIteratorTest has 1 and 2
       - Tests afterwards have 2

 - The code could be simplified if we removed the
TimePartitionPhasesIteratorTest
entirely and just put it in an anonymous namespace
      - hotspot/gtest/utilities/test_bitMap_search.cpp uses anonymous
namespaces so gtest does support it
      Then lines such as:

 219   EXPECT_NO_FATAL_FAILURE(TimePartitionPhasesIteratorTest::validate_gc_phase(iter.next(),
1, "SubPhase3", 15, 16));

become:

 219   EXPECT_NO_FATAL_FAILURE(validate_gc_phase(iter.next(), 1,
"SubPhase3", 15, 16));


Apart from that, it looks good to me :)

Jc



On Tue, Oct 9, 2018 at 10:24 AM Igor Ignatyev <igor.ignatyev at oracle.com>
wrote:

> http://cr.openjdk.java.net/~iignatyev//8157728/webrev.00/index.html
> > 450 lines changed: 238 ins; 211 del; 1 mod;
>
> Hi all,
>
> could you please review this small (and hopefully trivial) patch which
> converts internal GCTimer_test to gtest?
>
> webrev:
> http://cr.openjdk.java.net/~iignatyev//8157728/webrev.00/index.html
> JBS: https://bugs.openjdk.java.net/browse/JDK-8157728
> testing:
>  - converted tests on linux-x64, windows-x64, macosx-x64, solaris-sparcv9
> in product and fastdebug variants
>  - build w/ precompiled-headers enabled and disabled
>
> PS the patch has been originally created by Kirill Zh, but hasn't been
> sent out for official review
>
> Thanks,
> -- Igor
>



-- 

Thanks,
Jc