Serguei, > This comments above became out-dated now.
fixed. (in-place, press shift-reload) > Nice. Consider it reviewed. Thanks, Serguei Thank you. I'll run rbt job and proceed with push if everything is OK. -Dmitry On 2016-10-05 16:02, serguei.spit...@oracle.com wrote: > Hi Dmitry, > > > > On 10/4/16 05:59, Dmitry Samersoff wrote: >> 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. > > Ok, I understand the original issues with the dependencies. > > >> 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. > > Thanks. > > >>> 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 >> > > > Good, thank you for the explanation. > > I see, testing of the jps options is moved from the JpsBase to the > JpsHelper.runJpsVariants(). > Also, the getManifest() and buildJar() is moved from the JpsHelper to > the LingeredAppForJps. > > A couple of comments. > > http://cr.openjdk.java.net/~dsamersoff/JDK-8165500/webrev.02/jdk_webrev/test/sun/tools/jps/JpsHelper.java.frames.html > > > 229 // 30673 /tmp/jtreg/jtreg-workdir/scratch/JpsBase.jar ... > 236 // 30673 JpsBase monkey ... 242 // 30673 JpsBase -Xmx512m > -XX:+UseParallelGC -XX:Flags=/tmp/jtreg/jtreg-workdir/scratch/vmflags > ... 250 // 30673 JpsBase +DisableExplicitGC ... > > > This comments above became out-dated now. >> >>>> 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. > Ok, thanks. >>> 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. > Your explanations helped, thanks. So, now the fix looks Ok to me as it > is more cleaner this way. >>> 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 > Nice. Consider it reviewed. Thanks, Serguei >> >> >> -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.
