Hi Serguei, 1s of all, thank you for your review.
since these tests have dependencies on more modules than corresponding TEST.properties file declares, we have to specify @modules tag in them, and because @modules in a test overrides modules from TEST.properties, jdk.jdi module should be mentioned in the tests. — Igor > On Mar 15, 2017, at 9:53 PM, serguei.spit...@oracle.com wrote: > > Hi Igor, > > This looks good to me. > A couple of questions below. > > > http://cr.openjdk.java.net/~iignatyev//8176176/webrev.01/test/com/sun/jdi/InvokeHangTest.java.udiff.html > > - * @modules jdk.jdi > * @library /test/lib > + * @modules java.management > + * jdk.jdi Should the jdk.jdi be removed from this com/sun/jdi test? > > > http://cr.openjdk.java.net/~iignatyev//8176176/webrev.01/test/com/sun/jdi/RedefineCrossEvent.java.udiff.html > > * @modules java.corba > * jdk.jdi > > Should the jdk.jdi be removed from this com/sun/jdi test? > > > Thanks, > Serguei > > > On 3/15/17 11:16, Igor Ignatyev wrote: >> Shura, >> >> Thank you for your review. I have update >> test/java/lang/management/PlatformLoggingMXBean/TEST.properties[1] and >> checked that there are no similar things in other TEST.properties files. >> >> Still looking for a review from a Reviewer. >> >> [1] >>> --- a/test/java/lang/management/PlatformLoggingMXBean/TEST.properties >>> Mon Mar 13 18:54:58 2017 -0700 >>> +++ b/test/java/lang/management/PlatformLoggingMXBean/TEST.properties >>> Wed Mar 15 11:09:06 2017 -0700 >>> @@ -1,3 +1,2 @@ >>> -modules = java.management \ >>> - java.logging >>> +modules = java.logging >> >> Thanks, >> — Igor >> >>> On Mar 15, 2017, at 9:56 AM, Alexandre (Shura) Iline >>> <alexandre.il...@oracle.com> wrote: >>> >>> Igor, >>> >>> I have looked through a bunch of tests where @modules is changed - that >>> looks good. >>> >>> One minor thing I noticed is that, in >>> test/java/lang/management/PlatformLoggingMXBean/TEST.properties you are >>> explicitly calling out java.management. You do not have to do that because >>> “modules” property is concatenated through the hierarchy. java.management >>> is already listed in test/java/lang/management/TEST.properties. >>> >>> Shura >>> >>>> On Mar 13, 2017, at 9:03 PM, Igor Ignatyev <igor.ignat...@oracle.com> >>>> wrote: >>>> >>>> Shura, >>>> >>>> Thank you for review, I agree that having separate bugs is more >>>> convenient. [1] is new webrev w/ changes only in the files w/ incorrect >>>> module dependency declarations. >>>> >>>> [1] http://cr.openjdk.java.net/~iignatyev//8176176/webrev.01/index.html >>>> >>>> Thanks, >>>> — Igor >>>> >>>>> On Mar 9, 2017, at 5:02 PM, Alexandre (Shura) Iline >>>>> <alexandre.il...@oracle.com> wrote: >>>>> >>>>> Igor, >>>>> >>>>> I have reviewed some number tests which change the @modules - they are >>>>> fine. >>>>> >>>>> You, however, fix more things with this than missing module dependency >>>>> declaration. There is a redesign of line-number-sensitive tests [1] and >>>>> other multiple improvements such as in [2]. Would it be more convenient >>>>> to have that as separate bugs? >>>>> >>>>> Shura >>>>> >>>>> [1] >>>>> http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/test/com/sun/jdi/ArgumentValuesTest.java.sdiff.html >>>>> [2] >>>>> http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/test/com/sun/jdi/ArrayLengthDumpTest.sh.sdiff.html >>>>> >>>>>> On Mar 7, 2017, at 1:07 PM, Igor Ignatyev <igor.ignat...@oracle.com> >>>>>> wrote: >>>>>> >>>>>> http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/index.html >>>>>>> 2586 lines changed: 669 ins; 484 del; 1433 mod; >>>>>> Hi all, >>>>>> >>>>>> Could you please review this changeset which fix @modules dependency >>>>>> declaration in jdk_svc tests? >>>>>> there are a couple issues w/ modules in jdk_svc tests: >>>>>> - some tests do not specify modules which they depend on >>>>>> - modules in TEST.properties is not used in cases there all tests >>>>>> (should) have the same @modules directive >>>>>> - @modules directive isn't placed according to current convention >>>>>> (before the 1st run directive) >>>>>> >>>>>> Since this fix has already touched lots of tests, I have decided to use >>>>>> this opportunity and reordered some of jtreg tags as well, so there >>>>>> won’t be two massive updates in the tests. >>>>>> >>>>>> Some of our tests are line number sensitive, and then I fixed jtreg >>>>>> declaration, they started to fail. It was really hard to find our all >>>>>> line number sensitive tests, so I have unified the way we declare that >>>>>> as a part of this fix. Please let me know if you prefer to have it done >>>>>> separately. >>>>>> >>>>>> There are two one-liners which, I hope, can simplify review: >>>>>> [1] shows only the changes which are not in comments. Besides obvious >>>>>> new added TEST.properties, there are changes in the following line >>>>>> number sensitive tests (which I mentioned before): >>>>>> test/com/sun/jdi/ArgumentValuesTest.java >>>>>> test/com/sun/jdi/BreakpointTest.java >>>>>> test/com/sun/jdi/FetchLocals.java >>>>>> test/com/sun/jdi/GetLocalVariables.java >>>>>> test/com/sun/jdi/GetSetLocalTest.java >>>>>> test/com/sun/jdi/LambdaBreakpointTest.java >>>>>> test/com/sun/jdi/LineNumberOnBraceTest.java >>>>>> test/com/sun/jdi/PopAndStepTest.java >>>>>> >>>>>> [2] shows changes in jtreg tags, it can help to see that almost all >>>>>> changes in jtreg tags are either moving of tags which does not affect >>>>>> execution order or @modules changes. >>>>>> >>>>>> webrev: >>>>>> http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/index.html >>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8176176 >>>>>> Testing: >>>>>> - jdk_svc on linux, windows, mac >>>>>> - checked that all tests which could be executed with full jdk before >>>>>> still can be executed with full jdk >>>>>> >>>>>> Thanks, >>>>>> — Igor >>>>>> >>>>>> [1] $ hg diff -w | grep "^[+-]" | grep -v "^[+-]\s*[*#]" >>>>>> [2] $ hg diff -w | grep -e "^\(+++ b\)\|\(--- a\)" -e "^[+-]\s*[*#]\s*@" >