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

Wed Jun 27 15:36:51 UTC 2018


On 2018-06-27 00:29, Ekaterina Pavlova wrote:
>
> well, INCLUDE_GRAAL is not defined at the time we run tests.
> I can try to guard by something like
>  ifeq ($(OPENJDK_TARGET_OS)-$(OPENJDK_TARGET_CPU), $(filter 
> $(OPENJDK_TARGET_OS)-$(OPENJDK_TARGET_CPU),linux-x64 macosx-x64 
> windows-x64))
>
> but I am not sure we should proceed this way. It looks too much 
> complicated.
> It is safe to pass -e:TEST_IMAGE_GRAAL_DIR to jtreg even it will not 
> be used.
> We also do similar way for example for -e:JDK8_HOME
>
I agree, no need to guard the addition of the env variable pass through.
> I have uploaded new webrev at the same location:
>  http://cr.openjdk.java.net/~epavlova//8205207/webrev.01/index.html
>
Looks good to me now. (Magnus is on vacation so you probably don't want 
to wait for him to respond)

/Erik
>
> thanks for reviewing,
> -katya
>
>>> /Magnus
>>>
>>>>
>>>> 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.
>>>>>>>
>>>>>>
>>>>
>>
>