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

[Dmitry Samersoff]

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
>>>>>>
>>


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.