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

Tue Jun 16 07:14:59 UTC 2020

Hi Igor,

On 16/06/2020 10:39 am, 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
>> 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 .

Ok.

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

Many thanks for doing this!

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

Looks good.

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

Looks good.

> 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

Looks good.

> 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

Hmm okay. Not sure this needed its own category of build logic but ...

Aside: Makefiles should not have the classpath exception version of the 
license header. But they all do for some reason. I'll check if we need 
to do a separate cleanup of that.

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

Looks good. Took me a while to understand the connection to the test 
library here.

> 5. LingeredAppTest fix (which apparently has never been run before):
> http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.05-LingeredAppTest/

Someone from serviceability should evaluate this test. It may not be needed.

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

Seems okay.

Thanks,
David
-----

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