RFR(M): JDK-8165500: TestJpsJar shouldn't jar all test.classpath directories

Wed Oct 5 13:02:30 UTC 2016

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.spitsyn at 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.spitsyn at 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
>>>>>
>