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

[serguei.spitsyn at oracle.com]

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.
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/d028fcec/attachment.html>