Hi David, thanks for your review. re: LingeredAppTest, I agree that the test doesn't look very useful, yet I'd remind that the goal of this (and other test in /test/lib-test) is to (sanity) test testlibrary in order to easily spot bugs in testlibrary in a clear manner so one would not have to investigate failures of actual "product" tests and go thru their sometimes convoluted logic just to find out that the problem was in LingeredApp. w/ that being said, this test can do a better job in testing LingeredApp, so I'll file a JBS ticket against hotspo/svc to evaluate/improve/discuss the fate of the test.
Thanks, -- Igor > On Jun 16, 2020, at 12:14 AM, David Holmes <david.hol...@oracle.com> wrote: > > 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.hol...@oracle.com >>> <mailto: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 <mailto: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 >>> <mailto: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
