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

Fri Jun 22 21:38:43 UTC 2018

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. 
>>
>