Re: RFR(M): JDK-8165500: TestJpsJar shouldn't jar all test.classpath directories
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.spit...@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.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
Re: RFR(M): JDK-8165500: TestJpsJar shouldn't jar all test.classpath directories
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.spit...@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.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
Re: RFR(M): JDK-8165500: TestJpsJar shouldn't jar all test.classpath directories
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,
Re: RFR(M): JDK-8165500: TestJpsJar shouldn't jar all test.classpath directories
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. 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. > > 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 >> 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. > 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. > 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 -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 >>> >>>
Re: RFR(M): JDK-8165500: TestJpsJar shouldn't jar all test.classpath directories
On 10/4/16 01: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? 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. Forgot to ask about the TestJpsJarRelative. Is a relative jar path not tested anymore? Thanks, Serguei 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.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
Re: RFR(M): JDK-8165500: TestJpsJar shouldn't jar all test.classpath directories
On 10/4/16 01: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? 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.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
Re: RFR(M): JDK-8165500: TestJpsJar shouldn't jar all test.classpath directories
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.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
Re: RFR(M): JDK-8165500: TestJpsJar shouldn't jar all test.classpath directories
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. 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. 2) I combined TestJpsClass and TestJpsJar to single test to save a bit of execution time. 3) Also I fixed couple of minor issues in the code. -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 >> -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
Re: RFR(M): JDK-8165500: TestJpsJar shouldn't jar all test.classpath directories
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
RFR(M): JDK-8165500: TestJpsJar shouldn't jar all test.classpath directories
(* Resending, Any reviewers? *) 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.
PING Re: RFR(M): JDK-8165500: TestJpsJar shouldn't jar all test.classpath directories
Any comments? On 2016-09-23 12: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.
Re: RFR(M): JDK-8165500: TestJpsJar shouldn't jar all test.classpath directories
Daniel,
Thank you! I'll fix it.
-Dmitry
On 2016-09-23 12:48, Daniel Fuchs wrote:
> Hi Dmitry,
>
> There's a potential bug here. If two directories in the
> classpath contains two different versions of A.class,
> then the wrong version (the one that comes last) will
> be included in the jar:
>
> LingeredAppForJps::buildJar
>
> 109 for (String path : testClassPath.split(File.pathSeparator)) {
> 110 String classFullName = path + File.separator +
> className + ".class";
> 111 File f = new File(classFullName);
> 112 if (f.exists()) {
> 113 jarArgs.add("-C");
> 114 jarArgs.add(path);
> 115 jarArgs.add(".");
> 116 System.out.println("INFO: scheduled to jar " + path);
> 117 }
> 118 }
>
> To be more exact you should probably go through the list
> in reverse order:
>
> String[] paths = testClassPath.split(File.pathSeparator);
> for (int i = paths.length ; --i >= 0 ; ) {
> String path = paths[i];
> String classFullName = ...;
> ...
> }
>
> or alternatively, keep your loop as it is but break out of it
> as soon as you found the class you're looking for (I don't
> know whether that would be the right solution though, I am
> not familiar with these tests), or yet again keep on looping
> but throw an exception if you encounter the class twice for
> a different path.
>
> best regards,
>
> -- daniel
>
> On 23/09/16 10: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.
Re: RFR(M): JDK-8165500: TestJpsJar shouldn't jar all test.classpath directories
Hi Dmitry,
There's a potential bug here. If two directories in the
classpath contains two different versions of A.class,
then the wrong version (the one that comes last) will
be included in the jar:
LingeredAppForJps::buildJar
109 for (String path : testClassPath.split(File.pathSeparator)) {
110 String classFullName = path + File.separator +
className + ".class";
111 File f = new File(classFullName);
112 if (f.exists()) {
113 jarArgs.add("-C");
114 jarArgs.add(path);
115 jarArgs.add(".");
116 System.out.println("INFO: scheduled to jar " + path);
117 }
118 }
To be more exact you should probably go through the list
in reverse order:
String[] paths = testClassPath.split(File.pathSeparator);
for (int i = paths.length ; --i >= 0 ; ) {
String path = paths[i];
String classFullName = ...;
...
}
or alternatively, keep your loop as it is but break out of it
as soon as you found the class you're looking for (I don't
know whether that would be the right solution though, I am
not familiar with these tests), or yet again keep on looping
but throw an exception if you encounter the class twice for
a different path.
best regards,
-- daniel
On 23/09/16 10: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
RFR(M): JDK-8165500: TestJpsJar shouldn't jar all test.classpath directories
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.
