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.spit...@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.spit...@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


Reply via email to