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

[serguei.spitsyn at oracle.com]

On 10/4/16 01: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?
>
> If so, then the JpsBase tested the jps options qlmvV.
> Is this functionality thrown away?
>
>
>> 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.
>
>
> In fact, I've got lost a little bit in reviewing this fix.
Sorry, not lost but confused.

Thanks,
Serguei

> It seems, it implements some test enhancement, not a fix for the 
> original issue.
> 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?
> Can you confirm that it is gone now?
>
> What about the "TestJpsJar shouldn't jar all test.classpath directories"?
> Has this issue been resolved with this update?
>
>
> 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/20161004/b3854e31/attachment.html>