@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 . 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.hol...@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.bur...@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.joels...@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
