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

Tue Jun 26 16:08:30 UTC 2018

Hello Magnus,

On 6/26/18 12:50 AM, Magnus Ihse Bursie wrote:
> 23 juni 2018 kl. 00:22 skrev Ekaterina Pavlova <ekaterina.pavlova at oracle.com <mailto:ekaterina.pavlova at oracle.com>>:
> 
>> Fixed and regenerated webrev at the same location:
>> http://cr.openjdk.java.net/~epavlova//8205207/webrev.01/index.html
> 
> Better, but not done yet.
> 
> In JtregGraalUnit.gmk: line 52-53 should be on a single line.

will fix

> The SRC list for BUILD_VM_COMPILER_TESTS looks insane, but the root cause here is the weird layout on disk. But the .test part really surprises me, I thought we had a clear rule to have no test source in src?!

These are not jtreg tests, these are graalunit tests which are coming as part of Graal port (from Graal ws).
We can't change this layout right now.

> In lib-tests.m4: Copy paste error: AC_MSG_ERROR([You must specify the path to tonga jar])

ops, will fix

> The addition in RunTests.gmk and TestCommon.gmk should be guarded by:
>    ifeq ($(INCLUDE_GRAAL), true)

ok, will look

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