[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR(L) : 8176176 : fix @modules in jdk_svc tests
From: "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date: 2017-03-16 5:10:21
Message-ID: a648254c-f5a3-40e3-def3-1d91ab083ea9 () oracle ! com
[Download RAW message or body]
Igor,
It was my guess but it is nice to have it explicitly explained.
Thanks!
Serguei
On 3/15/17 22:07, Igor Ignatyev wrote:
> 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.spitsyn@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.iline@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.ignatyev@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.iline@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.ignatyev@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: \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > 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*@"
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic