RFR(M) : 8148244 : Finalize and integrate GTest implementation

[Igor Ignatyev]

http://cr.openjdk.java.net/~iignatyev///8148244-root/webrev.03
http://cr.openjdk.java.net/~iignatyev///8148244-hotspot/webrev.03

Hi Erik,

I’ve update the changes according to your comments, could you please take a look?

Thanks,
— Igor

> On May 6, 2016, at 10:05 AM, Erik Joelsson <erik.joelsson at oracle.com> wrote:
> 
> Good catch with disabling gtest in the case of the "build jdk". 
> 
> I would prefer if you introduced a new variable for it, something more obvious like BUILD_GTEST. Default it to true in spec.gmk.in and override that value to false in buildjdk-spec.gmk.in. (The TEST_IN_BUILD variable had a different purpose and is now dead, so please also remove it from buildjdk-spec.gmk.in). I would also prefer if you put the whole inclusion of CompileGtest.gmk in a conditional in CompileLibraries.gmk instead, so we avoid all of the evaluation of building it when disabled. Also, please don't forget to indent anything inside a conditional. Like this:
> 
> ifeq ($(BUILD_GTEST), true)
>   include lib/CompileGtest.gmk
> endif
> 
> 
> For completeness, there should probably also be a conditional of BUILD_GTEST like this in Main.gmk:
> ifeq ($(BUILD_GTEST), true)
>   test-image-hotspot-gtest:
> 
>         +($(CD) $(HOTSPOT_TOPDIR)/make/test && $(MAKE) $(MAKE_ARGS) -f GtestImage.gmk)
> endif
> 
> 
> /Erik
> 
> 
> On 2016-05-05 16:58, Igor Ignatyev wrote:
>> Hi,
>> 
>> I’ve rebased the changes to the latest jdk9/hs and removed the comment from JvmMapfile.gmk. Also I’ve guarded building gtest w/ if TEST_IN_BUILD != false (
>> http://cr.openjdk.java.net/~iignatyev/8148244-hotspot/webrev.02/make/lib/CompileGtest.gmk.html
>> ), so we won’t build it when we compile build jdk in case of cross-compliation.
>> 
>> new webrevs:
>> 
>> http://cr.openjdk.java.net/~iignatyev/8148244-root/webrev.02
>> http://cr.openjdk.java.net/~iignatyev/8148244-hotspot/webrev.02/
>> 
>> 
>> Thanks,
>> — Igor
>> 
>> 
>> 
>>> On May 2, 2016, at 3:55 PM, Jesper Wilhelmsson <jesper.wilhelmsson at oracle.com>
>>>  wrote:
>>> 
>>> Even though the comment on line 155 in JvmMapfile.gmk is accurate I agree with removing it.
>>> 
>>> Besides that the change looks good!
>>> /Jesper
>>> 
>>> 
>>> Den 2/5/16 kl. 14:04, skrev Erik Joelsson:
>>> 
>>>> Hello,
>>>> 
>>>> Build changes look good to me, but I also wrote most of them, except for
>>>> makefiles/lib/JvmMapfile.gmk line 155. I could not have written that. Please
>>>> remove.
>>>> 
>>>> The webrevs seem to be generated against jdk9/hs without the removal of the old
>>>> build system. I would recommend making that merge and generate new webrevs since
>>>> makefiles moved around quite a bit.
>>>> 
>>>> /Erik
>>>> 
>>>> On 2016-05-02 13:21, Igor Ignatyev wrote:
>>>> 
>>>>> http://cr.openjdk.java.net/~iignatyev/8148244-hotspot/webrev.00
>>>>>> 615 lines changed: 605 ins; 2 del; 8 mod;
>>>>>> 
>>>>> http://cr.openjdk.java.net/~iignatyev/8148244-root-wo-gtest/webrev.00/
>>>>>> 218 lines changed: 209 ins; 0 del; 9 mod;
>>>>>> 
>>>>> http://cr.openjdk.java.net/~iignatyev/8148244-root/webrev.00/
>>>>>> 32029 lines changed: 32018 ins; 0 del; 11 mod;
>>>>>> 
>>>>> Hi all,
>>>>> 
>>>>> could you please review this patch which delivers core of JEP 281: HotSpot C++
>>>>> Unit-Test Framework[1]?
>>>>> in more details, this patch
>>>>>  - integrates sources of gtest-1.7.0[2] to /test/fmw/gtest (~32k LOC)
>>>>>  - introduces TEST macros (/hotspot/test/native/unittest.hpp)
>>>>>   - TEST — a basic unit test, doesn’t require an inited JVM
>>>>>   - TEST_VM — a test which requires an inited JVM, but expected to leave JVM
>>>>> in a valid state
>>>>>   - TEST_OTHER_VM — a test which requires a clean inited JVM
>>>>>   - TEST_VM_ASSERT/TEST_VM_ASSERT_MSG - tests to verify assert, “death”[3]
>>>>> tests which require inited JVM
>>>>>  - contains two tests as smoke tests for the project and examples. new tests
>>>>> should be added to /hotspot/test/native/ using the same directories layout as
>>>>> a corresponding product source files, and have prefix ‘test_’, e.g.
>>>>> test/native/runtime/test_os.cpp w/ tests for runtime/test_os.cpp
>>>>>  - updates makefiles to build/run tests : to run tests, one should use 'make
>>>>> test TEST=hotspot_gtest’
>>>>>  - adds hotspot_gtest to jprt testset
>>>>> 
>>>>> more information on the project can be found in JEP 281[1].
>>>>> 
>>>>> please be informed, that existing unit tests (aka internal tests) will be
>>>>> ported into new framework later, this activity is tracking by JDK-8077965[4].
>>>>> 
>>>>> PS This patch relies on JDK-8149591[5] which is under review[6].
>>>>> 
>>>>> Thanks,
>>>>> — Igor
>>>>> 
>>>>> JBS: 
>>>>> https://bugs.openjdk.java.net/browse/JDK-8148244
>>>>> 
>>>>> webrevs:
>>>>>  root      : 
>>>>> http://cr.openjdk.java.net/~iignatyev/8148244-root/webrev.00/
>>>>> 
>>>>>  w/o gtest :
>>>>> 
>>>>> http://cr.openjdk.java.net/~iignatyev/8148244-root-wo-gtest/webrev.00/
>>>>> 
>>>>>  hotspot   : 
>>>>> http://cr.openjdk.java.net/~iignatyev/8148244-hotspot/webrev.00
>>>>> 
>>>>> 
>>>>> [1] 
>>>>> https://bugs.openjdk.java.net/browse/JDK-8047975
>>>>> 
>>>>> [2] 
>>>>> https://github.com/google/googletest/releases/tag/release-1.7.0
>>>>> 
>>>>> [3]
>>>>> 
>>>>> https://github.com/google/googletest/blob/master/googletest/docs/V1_7_AdvancedGuide.md#death-tests
>>>>> 
>>>>> 
>>>>> [4] 
>>>>> https://bugs.openjdk.java.net/browse/JDK-8077965
>>>>> 
>>>>> [5] 
>>>>> https://bugs.openjdk.java.net/browse/JDK-8149591
>>>>> 
>>>>> [6] 
>>>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2016-April/022937.html
>