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

[Erik Joelsson]

Looks good!

/Erik

On 2016-05-09 13:45, Igor Ignatyev wrote:
> 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