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

[Erik Joelsson]

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.

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.

FLATTEN has no effect in the SetupCopyFiles call since all the jar files 
found by wildcard can only be in one directory anyway.

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.

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

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

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