RFR(M): 8205207: Port Graal unit tests under jtreg

[Erik Joelsson]

Looks ok to me.

/Erik


On 2018-06-22 15:22, Ekaterina Pavlova wrote:
> Fixed and regenerated webrev at the same location:
>  http://cr.openjdk.java.net/~epavlova//8205207/webrev.01/index.html
>
> Erik,
> thanks again for your detailed reviews!
>
> -katya
>
> On 6/22/18 2:38 PM, Erik Joelsson wrote:
>> Hello Katya,
>>
>> This looks much better, thanks!
>>
>> A few suggestions still:
>>
>> Main.gmk: instead of repeating the assignment in both if and else block:
>>
>> ifeq ($(INCLUDE_GRAAL), true)
>>    JVM_TEST_IMAGE_TARGETS += test-image-hotspot-jtreg-graal
>> endif
>>
>> I think it's fine to do that without the ?= because an alternative 
>> JVM implementation is unlikely to be using graal anyway.
>>
>> In JtregGraalUnit.gmk: Some minor style nits:
>> 54: align )) with first eval line as you have done with the other 
>> eval calls
>> 118: Since 117 is a one line parameter, please move comma to 117
>> 133: Same as for 118
>> 130-132: Please indent 4 spaces for continuation
>>
>> /Erik
>>
>> On 2018-06-22 14:16, Ekaterina Pavlova wrote:
>>> Erik, Doug,
>>>
>>> thank you a lot for your reviews and advises.
>>> I fixed everything what Erik has pointed out, please see my answers 
>>> inlined.
>>> As about moving more staff in 'updategraalinopenjdk' can we consider 
>>> this as next step?
>>> I am not quite familiar with 'updategraalinopenjdk' and didn't 
>>> contribute into Graal ws yet,
>>> so I will prefer to do this improvement/refactoring as part of 
>>> separate JDK issue.
>>>
>>> New webrev is here:
>>>   webrev: 
>>> http://cr.openjdk.java.net/~epavlova//8205207/webrev.01/index.html
>>>      JBS: https://bugs.openjdk.java.net/browse/JDK-8205207
>>>  testing: tested by building and running graal unit tests
>>>
>>>
>>> On 6/19/18 2:00 PM, Erik Joelsson wrote:
>>>> Hello,
>>>>
>>>> Please always include build-dev when reviewing build changes.
>>>>
>>>> In general this looks pretty good but there are some details that 
>>>> need fixing.
>>>>
>>>> (Aside from this particular review, I must state my reservation 
>>>> against all the special treatment graal is enjoying from a source 
>>>> and test layout and build perspective. My understanding here is 
>>>> that someone will have to regularly update the wrapper jtreg tests 
>>>> manually using the script. This in addition to having to implement 
>>>> this very convoluted redirection logic because the tests are in the 
>>>> wrong place. Wouldn't it make a lot more sense to put the 
>>>> complicated logic in the import procedure for graal source so that 
>>>> it would fit nicely into the OpenJDK source/build/test model, 
>>>> instead of adding more and more complicated workarounds in the 
>>>> OpenJDK build and test procedures?)
>>>>
>>>> On 2018-06-18 22:26, Ekaterina Pavlova wrote:
>>>>> Hi All,
>>>>>
>>>>> please review porting of Graal unit tests under jtreg. The main 
>>>>> idea of this porting is to keep Graal unit tests as is
>>>>> (located in src/jdk.internal.vm.compiler/share/classes/*.test) and 
>>>>> run them similar way they are run in Graal project.
>>>>> To achieve this 
>>>>> test/hotspot/jtreg/compiler/graalunit/common/GraalUnitTestLauncher.java 
>>>>> helper class has been written
>>>>> which simply launches com.oracle.mxtool.junit.MxJUnitWrapper to 
>>>>> run specified set of Graal unit tests. The groups of
>>>>> Graal unit tests to run are defined in auto-generated 
>>>>> test/hotspot/jtreg/compiler/graalunit/*.java jtreg tests.
>>>>>
>>>>> New make/test/JtregGraalUnit.gmk is used to build Graal unit tests 
>>>>> into jdk.vm.compiler.tests.jar and then install
>>>>> it in $(TEST_IMAGE_DIR)/hotspot/jtreg/graal/.
>>>>>
>>>> GRAALUNIT_LIB is never defined (in the open). Since this is used in 
>>>> open makefiles, we need an open configure option to set it.
>>>
>>> ok, I created open/make/autoconf/lib-tests.m4 and did put Graal 
>>> related staff there.
>>>
>>>> To make things clearer, I would rename LIB_DIR to something like 
>>>> LIB_OUTPUTDIR to signal that this is a location to put files in, 
>>>> rather than read them from.
>>>
>>> ok, renamed
>>>
>>>>
>>>> FLATTEN has no effect in the SetupCopyFiles call since all the jar 
>>>> files found by wildcard can only be in one directory anyway.
>>>
>>> ok, removed
>>>
>>>> BUILDTOOLS_OUTPUTDIR is meant for tools used during the build. 
>>>> Tests classes and jars should be built into 
>>>> $(SUPPORT_OUTPUTDIR)/test/..> Please create a suitable sub 
>>>> directory there for the output from this makefile.
>>>
>>> ok, introduced COMPILE_OUTPUTDIR := 
>>> $(SUPPORT_OUTPUTDIR)/test/graalunit to be used to build graal unit 
>>> test classes.
>>>
>>>> The all and test-image targets are never called so no need to 
>>>> declare them.
>>>>
>>>> There are some style issues too. (Please see 
>>>> http://openjdk.java.net/groups/build/doc/code-conventions.html for 
>>>> details.)
>>>> * 43 logic indent 2 spaces
>>>> * 52 we like to put the ending )) on a new line with ', \' at the 
>>>> end of the line before
>>>> * 58 continuation indent 4 spaces
>>>> * 88, 89 please break long lines
>>>> * 90 continuation indent 4 spaces
>>>> * 99 continuation indent 4 spaces
>>>> * 120 break before ))
>>>
>>> I think I fixed everything
>>>
>>>>> make/Main.gmk adds proper dependencies for 
>>>>> build-test-hotspot-jtreg-graal and test-image-hotspot-jtreg-graal.
>>>>>
>>>> The target build-test-hotspot-jtreg-graal only needs to depend on 
>>>> exploded-image-optimize.
>>>
>>> fixed
>>>
>>>> The new graal specific targets should be guarded by INCLUDE_GRAAL 
>>>> in Main.gmk. Otherwise those targets will silently do nothing if 
>>>> invoked without graal. That means adding them to 
>>>> JVM_TEST_IMAGE_TARGETS needs to be conditional too.
>>>
>>> fixed
>>>
>>>>> test/TestCommon.gmk passes TEST_IMAGE_GRAAL_DIR to jtreg so jtreg 
>>>>> knows where to find Graal tests and libs.
>>>>>
>>>> This needs to be duplicated for make/RunTest.gmk so that these 
>>>> tests work with "make run-test" as well.
>>>
>>> added
>>>
>>> thanks,
>>> -katya
>>>
>>>> /Erik
>>>>> test/hotspot/jtreg/compiler/graalunit/TestPackages.txt file 
>>>>> defines "testName -> testPrefix [requiresStatement]" mapping
>>>>> which is used by 
>>>>> test/hotspot/jtreg/compiler/graalunit/generateTests.sh script to 
>>>>> generate jtreg tests.
>>>>>
>>>>> test/hotspot/jtreg/compiler/graalunit/com.oracle.mxtool.junit was 
>>>>> ported from mx project without any changes.
>>>>>
>>>>>
>>>>>     JBS: https://bugs.openjdk.java.net/browse/JDK-8205207
>>>>>  webrev: 
>>>>> http://cr.openjdk.java.net/~epavlova//8205207/webrev.00/index.html
>>>>> testing: new tests were executed as part of automatic HS testing 
>>>>> for several months.
>>>>>
>>>>>
>>>>> thanks,
>>>>> -katya
>>>>>
>>>>> p.s.
>>>>>  Igor Ignatyev volunteered to sponsor this change. 
>>>>
>>>
>>
>