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

Tue Jun 16 14:03:28 UTC 2020

On 2020-06-16 15:06, Erik Joelsson wrote:
>
>
> 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.)
>

Ditto.

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