RFR(S) : 8211977 : move testlibrary tests into one place

Tue Jun 16 13:06:49 UTC 2020

On 2020-06-15 17:39, Igor Ignatyev wrote:
> @David, Erik, Magnus,
>
> please find the answers to your comments at the bottom of this email.
>
> @all,
>
> David's and Erik's comments made me realize that some parts of the 
> original patch were stashed away and didn't make it to webrev.00. I'm 
> truly sorry for the confusion and inconvenience. I also agree w/ David 
> that it's hard to follow/review, and my gaffe just made it worse, 
> therefore I'd like to start it over again from a clean sate
>
> http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02

Build changes look good to me now, except for a missing newline in 
Main.gmk. (No need for new review.)

/Erik

>> 833 lines changed: 228 ins; 591 del; 14 mod; 
>
> could you please review the patch which puts all tests for common 
> testlibrary classes (which is ones located in /test/lib) into one 
> location under /test/lib-test? please note this intentionally doesn't 
> move all "testlibrary" tests, e.g. tests for CTW, hotspot-specific 
> testlibrary, are left in /test/hotspot/jtreg/testlibrary_tests/ctw .
>
> to simplify review, the patch has been split into several webrevs, 
> with each of them accomplishes a more-or-less distinct thing, but is 
> not necessary self-contained:
>
> 0. trivial move of tests from test/jdk and test/hotspot/jtreg test 
> suites to test/lib-test:
> http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.00-move/
>
> 1. removal of hotspot's AssertsTest, OutputAnalyzerReportingTest and 
> "merge" of hotspot's and jdk's OutputAnalyzerTest:
> http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.01-merge/
>
> 2. simplification of TestNativeProcessBuilder tests: converts Test 
> class used by TestNativeProcessBuilder into a static nested class:
> http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.02-TestNativeProcessBuilder
>
> 3. makefiles changes to build native parts of lib-test tests. in past, 
> there were no tests w/ native parts in this test suite, 
> TestNativeProcessBuilder is the 1st such test; this part also removes 
> now unneeded lines from hotspot test suite makefile:
> http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.03-NativeLibTest
>
> 4. clean ups in hotspot test suite: adjusted location 
> of SimpleClassFileLoadHookTest test, which is a test for 
> hotspot-specific test library (located in 
> /test/hotspot/jtreg/testlibrary/jvmti) and hence should not be moved 
> to /test/lib-test; removed TestMutuallyExclusivePlatformPredicates 
> from TEST.groups:
> http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.04-hotspot/
>
> 5. LingeredAppTest fix (which apparently has never been run before):
> http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.05-LingeredAppTest/
>
> 6. changes to make test/lib-test a fully supported and working test 
> suite, and add in into tier1,  includes adding ProblemList, 
> TEST.groups file, and 'randomness' k/w:
> http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.06-TESTROOT/
>
> webrev: http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02
> JBS: https://bugs.openjdk.java.net/browse/JDK-8211977
> testing:
>  - make test TEST=tier1 locally on macosx
>  - test/lib-test on  {windows,linux,macosx}-x64
>  - tier1 job (in progress)
>
> Thanks,
> -- Igor
>
>
>> On Jun 14, 2020, at 11:23 PM, David Holmes <david.holmes at oracle.com 
>> <mailto:david.holmes at oracle.com>> wrote:
>> <...>
>> This doesn't seem to move everything under 
>> test/hotspot/jtreg/testlibrary_tests/ e.g. ctw directory.
> this is intentionally, ctw test-library is hotspot specific, hence its 
> tests are to reside in hotspot test suite (until we decide to move the 
> library to /test/lib), the same is true for SimpleClassFileLoadHookTest.
>>
>> You did not modify hotspot/jtreg/TEST.groups but it refers to:
>> testlibrary_tests/TestMutuallyExclusivePlatformPredicates.java \
>> Plus it should probably refer to the new native_sanity tests somewhere.
> the actual version of the patch removed reference to 
> TestMutuallyExclusivePlatformPredicates from hotspot's TEST.groups and 
> had TestNativeProcessBuilder moved to /test/test-lib, so no updates 
> w.r.t. native_sanity are needed
>
>> The newly added copyright notice needs to have the correct initial 
>> copyright year based on when the file was first added to the repo.
>  /test/lib-test/TEST.ROOT file was created by JDK-8243010 on 
> 2020-04-16, hence the added copyright has 2020 as the initial 
> copyright year.
>
>> You used the wrong license header - makefiles don't use the classpath 
>> exception.
> JtregNativeLibTest.gmk is based on make/test/JtregNativeJdk.gmk which 
> also have classpath exception. quick grep showed that make directory 
> has 794 files which has '"Classpath" exception', out of 850 which 
> contain 'GNU General Public License version 2' string. And although I 
> agree that makefiles shouldn't have the classpath exception, I'd 
> prefer to keep JtregNativeLibTest.gmk in sync w/ the its siblings and 
> would leave it up to build team to decide how it's best to handle.
>
>
>> On Jun 15, 2020, at 8:12 AM, Magnus Ihse Bursie 
>> <magnus.ihse.bursie at oracle.com 
>> <mailto:magnus.ihse.bursie at oracle.com>> wrote:
>>
>> A few comments:
>>
>> This seems like code copied from elsewhere:
>>    57 # This evaluation is expensive and should only be done if this target was
>>    58 # explicitly called.
>>    59 ifneq ($(filter build-test-libtest-jtreg-native, $(MAKECMDGOALS)), )
>> I don't agree that this is an expensive evaluation. Furthermore, the 
>> makefile is only called for building the testlib and for making 
>> images, so in worst case it's just the image part that would get a 
>> penalty (although I highly doubt there is any).
> right, JtregNativeLibTest.gmk is based on make/test/JtregNativeJdk.gmk 
> which has this comment, I don't know enough details to say if it's 
> expensive or not, yet if you insist and there is a consensus within 
> build team, I can remove the comment.
>
>>
>>    82 $(eval $(call SetupCopyFiles,COPY_LIBTEST_JTREG_NATIVE, \
>> Please use space after comma.
> this again was preexisting in JtregNativeJdk.gmk, added the space 
> nevertheless.
>
>
>> On Jun 15, 2020, at 6:16 AM, Erik Joelsson <erik.joelsson at oracle.com 
>> <mailto:erik.joelsson at oracle.com>> wrote:
>>
>> In JtretNativeLibTest.gmk, lines 51-55 should probably be removed (or 
>> adjusted if linking to libjvm is actually needed).
> the lines were updated to define 
> BUILD_LIBTEST_JTREG_EXECUTABLES_LIBS_exejvm-test-launcher
>
>