Serguei, Exact code executed by original version of this tests depends to what tests was run before:
1. We have to explicitly @build JpsBase to put it into jar file. 2. Couple of testlibrary classes are build because of JpsBase.java dependency. Jtreg put it to ./JTwork/classes/sun/tools/jps/jdk/testlibrary/ i.e. we have incomplete jdk.testlibrary package here. 3. If jtreg overwrites this directory processing @library tag it works. Otherwise, if some other test pre-compile testlibrary using @build testlibrary, we get classNotFound exception when attempt to run jar file or other errors, it depends to what classes is actually found. 4. If we copy multiple directories to the single jar file, we assemble complete jdk.testlibrary package from multiple *different* places that appears in classpath in unpredictable order. It works but we can't say what code exactly was executed by the test. 5. if someone occasionally put a directory containing some huge file (e.g. coredump) into classspath the test tries to jar it and timeouts. See also below. On 2016-10-04 11:36, serguei.spit...@oracle.com wrote: > Dmitry, > > > On 10/3/16 05:25, Dmitry Samersoff wrote: >> Serguei, >> >> Thank you for comments. I'll fix it. >> >>> It is completely unclear what was the exact intention with this fix. >> It's not the first time I fix this test. It starts failing again as soon >> as something is changed in the environment, testlib or jtreg. >> >> So provide a stable test is the motivation for this work. > > Ok, thanks. > > >> 1) After refactoring the test uses LingeredApp as an attach target in >> all cases. Not only for sanity check but for tests against class file >> and jar file. >> >> LingeredApp has well defined behavior and doesn't depend to other >> parts of testlib so I expect that it fixes intermittent failures and >> ClassNotFound exceptions. > > It seems, you used the LingeredApp instead of the JpsBase (which is > deleted now). > Is it correct? Correct. > > If so, then the JpsBase tested the jps options qlmvV. > Is this functionality thrown away? No. All cases that was covered by original test is still covered. see JpsHelper.runJpsVariants. I restructure the test to have two clear separated parts - (a) process to attach to (b) process that runs and verify jps against (a) For your convenience I prepared the diff between original and modified version of the key function that shuffle jps arguments and check results. See: http://cr.openjdk.java.net/~dsamersoff/JDK-8165500/webrev.02/jps_argument_processing.patch >> 2) I combined TestJpsClass and TestJpsJar to single test to save a bit >> of execution time. > > This is good, thanks. > >> >> 3) Also I fixed couple of minor issues in the code. > > What issues, could you, please, list them? > It makes sense to do as it needs to be checked specifically. Added ".invalid" TLD to invalid DomainName as it should be according rfc2606. Removed userDirSanityCheck(fullProcessName)) as it's not necessary anymore. Removed argument loop as the only argument passed to the test. > In fact, I've got lost a little bit in reviewing this fix. > It seems, it implements some test enhancement, not a fix for the > original issue. As far as *jar* tool bug that discovers the problem with our test is already fixed, we can leave the test as is. But I'm not sure it's good to have a test that loads and executes classes in random order and we can't predict/reproduce exact test run. > Also, the DKFL failure matching is going to be lost with this > refactoring, as the > test has been renamed now and all bug report failure details won't help > anymore. > Were you able to reproduce the original issue with the unaltered test? Yes. > Can you confirm that it is gone now? Yes. It's gone. I run RBT couple of time and will run it more times before push. > What about the "TestJpsJar shouldn't jar all test.classpath directories"? > Has this issue been resolved with this update? Yes. It's solved. Now the test jars only a directory containing LingeredAppForJps.class -Dmitry > > > Thanks, > Serguei > > >> >> -Dmitry >> >> >> On 2016-10-03 09:23, serguei.spit...@oracle.com wrote: >>> Hi Dmitry, >>> >>> >>> It is completely unclear what was the exact intention with this fix. >>> >>> Could you, please, explain more? >>> What was the refactoring plan? >>> Why four files are removed? >>> Are two new files a replacement of the deleted four files? >>> What functionality was deleted and why and what was moved? >>> >>> >>> http://cr.openjdk.java.net/~dsamersoff/JDK-8165500/webrev.02/_webrev/test/lib/jdk/test/lib/apps/LingeredApp.java.frames.html >>> >>> Minor comments: >>> >>> 259 * Analyze an environment and prepare a command line to 260 * run >>> theApp, app name should be added explicitlyPlease, change: theApp => the >>> app (to be consistent with the line L326. Also, it makes sense to have >>> to separate sentences with dots at the end. >>> 329 * @throws IOException >>> >>> It'd be nice to add a comment at the place where exactly this exception >>> can be thrown. 357 // startApp of derived app can throw >>> 358 // an exception before, LA actually starts The comma is not needed. >>> This would look better: 357 // The startApp() of the derived app can throw >>> 358 // an exception before the LA actually starts >>> >>> Thanks, Serguei On 9/23/16 02:01, Dmitry Samersoff wrote: >>>> Everybody, >>>> >>>> Please, review: >>>> >>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8165500/webrev.02/ >>>> >>>> I refactored the test to improve stability and simplify further >>>> debugging. >>>> >>>> Testing: rbt >>>> >>>> PS: Diffs is messy, so pleas look at raw files. >>>> >>>> -Dmitry >>>> >> > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
