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

Tue May 10 16:13:12 UTC 2016

Jesper, Erik,

thank you both for review!

— Igor

> On May 9, 2016, at 5:26 PM, Jesper Wilhelmsson <jesper.wilhelmsson at oracle.com> wrote:
> 
> Looks good!
> /Jesper
> 
> Den 9/5/16 kl. 13:45, skrev 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
>>> 
>> 