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

Wed Oct 5 16:43:51 UTC 2016

Dmitry,

Thank you for the update.
Good luck with rbt run and push.

Thanks,
Serguei

On 10/5/16 08:48, Dmitry Samersoff wrote:
> 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.spitsyn at 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.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
>>>>>>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20161005/62b262f0/attachment.html>