[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR(M): JDK-8165500: TestJpsJar shouldn't jar all test.classpath directories
From: "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date: 2016-10-05 16:43:51
Message-ID: 8b891773-43df-672c-207f-0c5621b4c4cb () oracle ! com
[Download RAW message or body]
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@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@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@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
> > > > > > >
>
[Attachment #3 (text/html)]
<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix"><font size="-1">Dmitry,<br>
<br>
Thank you for the update.<br>
Good luck with rbt run and push.<br>
<br>
Thanks,<br>
Serguei</font><br>
<br>
<br>
On 10/5/16 08:48, Dmitry Samersoff wrote:<br>
</div>
<blockquote
cite="mid:745b4be5-42c7-5011-47e1-be54aab70d8a@oracle.com"
type="cite">
<pre wrap="">Serguei,
</pre>
<blockquote type="cite">
<pre wrap=""> This comments above became out-dated now.
</pre>
</blockquote>
<pre wrap="">
fixed. (in-place, press shift-reload)
</pre>
<blockquote type="cite">
<pre wrap="">Nice. Consider it reviewed. Thanks, Serguei
</pre>
</blockquote>
<pre wrap="">
Thank you.
I'll run rbt job and proceed with push if everything is OK.
-Dmitry
On 2016-10-05 16:02, <a class="moz-txt-link-abbreviated" \
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a> wrote: </pre>
<blockquote type="cite">
<pre wrap="">Hi Dmitry,
On 10/4/16 05:59, Dmitry Samersoff wrote:
</pre>
<blockquote type="cite">
<pre wrap="">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.
</pre>
</blockquote>
<pre wrap="">
Ok, I understand the original issues with the dependencies.
</pre>
<blockquote type="cite">
<pre wrap="">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, <a class="moz-txt-link-abbreviated" \
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a> wrote: </pre>
<blockquote type="cite">
<pre wrap="">Dmitry,
On 10/3/16 05:25, Dmitry Samersoff wrote:
</pre>
<blockquote type="cite">
<pre wrap="">Serguei,
Thank you for comments. I'll fix it.
</pre>
<blockquote type="cite">
<pre wrap="">It is completely unclear what was the exact intention \
with this fix. </pre>
</blockquote>
<pre wrap="">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.
</pre>
</blockquote>
<pre wrap="">Ok, thanks.
</pre>
<blockquote type="cite">
<pre wrap="">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.
</pre>
</blockquote>
<pre wrap="">It seems, you used the LingeredApp instead of the JpsBase \
(which is deleted now).
Is it correct?
</pre>
</blockquote>
<pre wrap="">Correct.
</pre>
</blockquote>
<pre wrap="">
Thanks.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">If so, then the JpsBase tested the jps options qlmvV.
Is this functionality thrown away?
</pre>
</blockquote>
<pre wrap="">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:
<a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/~dsamersoff/JDK-8165500/webrev.02/jps_argument_proces \
sing.patch">http://cr.openjdk.java.net/~dsamersoff/JDK-8165500/webrev.02/jps_argument_processing.patch</a>
</pre>
</blockquote>
<pre wrap="">
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.
<a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/~dsamersoff/JDK-8165500/webrev.02/jdk_webrev/test/sun \
/tools/jps/JpsHelper.java.frames.html">http://cr.openjdk.java.net/~dsamersoff/JDK-8165500/webrev.02/jdk_webrev/test/sun/tools/jps/JpsHelper.java.frames.html</a>
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.
</pre>
<blockquote type="cite">
<pre wrap="">
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">2) I combined TestJpsClass and TestJpsJar to single test \
to save a bit of execution time.
</pre>
</blockquote>
<pre wrap="">This is good, thanks.
</pre>
<blockquote type="cite">
<pre wrap="">3) Also I fixed couple of minor issues in the code.
</pre>
</blockquote>
<pre wrap="">What issues, could you, please, list them?
It makes sense to do as it needs to be checked specifically.
</pre>
</blockquote>
<pre wrap="">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.
</pre>
</blockquote>
<pre wrap="">Ok, thanks.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">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.
</pre>
</blockquote>
<pre wrap="">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.
</pre>
</blockquote>
<pre wrap="">Your explanations helped, thanks. So, now the fix looks Ok to me \
as it is more cleaner this way.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">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?
</pre>
</blockquote>
<pre wrap="">Yes.
</pre>
<blockquote type="cite">
<pre wrap="">Can you confirm that it is gone now?
</pre>
</blockquote>
<pre wrap="">Yes. It's gone. I run RBT couple of time and will run it more \
times before push.
</pre>
<blockquote type="cite">
<pre wrap="">What about the "TestJpsJar shouldn't jar all test.classpath
directories"?
Has this issue been resolved with this update?
</pre>
</blockquote>
<pre wrap="">Yes. It's solved. Now the test jars only a directory \
containing LingeredAppForJps.class
</pre>
</blockquote>
<pre wrap="">Nice. Consider it reviewed. Thanks, Serguei
</pre>
<blockquote type="cite">
<pre wrap="">
-Dmitry
</pre>
<blockquote type="cite">
<pre wrap="">
Thanks,
Serguei
</pre>
<blockquote type="cite">
<pre wrap="">-Dmitry
On 2016-10-03 09:23, <a class="moz-txt-link-abbreviated" \
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a> wrote: </pre>
<blockquote type="cite">
<pre wrap="">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?
<a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/~dsamersoff/JDK-8165500/webrev.02/_webrev/test/lib/jd \
k/test/lib/apps/LingeredApp.java.frames.html">http://cr.openjdk.java.net/~dsamersoff/J \
DK-8165500/webrev.02/_webrev/test/lib/jdk/test/lib/apps/LingeredApp.java.frames.html</a>
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:
</pre>
<blockquote type="cite">
<pre wrap="">Everybody,
Please, review:
<a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/~dsamersoff/JDK-8165500/webrev.02/">http://cr.openjdk.java.net/~dsamersoff/JDK-8165500/webrev.02/</a>
I refactored the test to improve stability and simplify further
debugging.
Testing: rbt
PS: Diffs is messy, so pleas look at raw files.
-Dmitry
</pre>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<pre wrap="">
</pre>
</blockquote>
</blockquote>
<pre wrap="">
</pre>
</blockquote>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic