RFR(L) : 8176176 : fix @modules in jdk_svc tests
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*@"
Re: RFR: 8159523. Fix tests depending on absence of -limitmods in VM arguments.
Hi Shura,
> Quite recently the suggested improvement was discussed with a few folks from
> hotspot team where the same or a similar implementation could be reused. I am
> CCing Igor and Dima to comment.
Right, Dima and I would really love to reuse part of this library and that’s
more important to have this library designed in the way we will be able to use,
extend and change it without massive update of the tests. After series of our
discussion, we came to the agreement that there are 4 different abstractions,
below I’m explaining what they are and why we need them.
- CommandLineBuilder {
ProccesBuilder build();
}
- ProcessLauncher {
ProcessResult launch(ProccesBuilder);
}
- ProcessResult {
// blocks if it’s still running
int exitCode();
long pid();
boolean isAlive();
TextAnalyzer stdout();
TextAnalyzer stderr();
analyze(String artifactName, Analyzer::);
Set artifacts(); // names of created files
}
- TextAnalyzer extends Analyzer
TextAnalyzer contains();
TextAnalyzer matches();
...
}
1. Command line builder, which is a huge part of jdk.testlibrary.tasks.JavaTask
class from your patch. We assume that there will be several different
implementation of this depending on an type of a spawning process and practical
needs. This can be described as a domain specific extension for
j.l.ProcessBuilder. At the end, all such extensions should return something
which is not a started process yet, an instance of ProcessBuilder sounds like a
good candidate. This is needed to have possibility to have multiple ways to
start a process using different Process launchers. For the time being, your
current patch does not need any changes, later we will introduce either a new
method to create a ProccessBuilder in Task and refactor run method to use it or
a run() method which takes a process launcher.
2. Process launcher defines a way to run a process, it includes several things
such as redirecting/copying output/error, running on a remote agent. We will
also be able to run a process inside a different kinds of debugger using a
decorator pattern. Again, there is no needs to change you current patch, since
such changes seem to be backward compatible.
3. Process results object contains information about a process, such as PID,
exit code, methods to get access to artifacts, including stdout, stderr and
different files created by a process. We expect to have several different
process results depending on a spawn process. For example if we start javac, we
know what files it will create and where, when we start jvm with particular
flags, we know where log files will be created or where core dump file can be
found, all this artifacts should become available thru methods of process
results object. We’d also like to have a possibility to work with still running
processes, so we need methods like isAlive(), kill(). Basically, this object
are domain specific extension of j.l.Process. The closes thing in your
changeset is Task.Result, but it represents only completed processes and has a
couple things which we believe should be a part of Analyzer. Hence you will
have to update your fix a little bit.
4. Analyzer classes will be used to define different checks. The most common
and obvious analyzer is text analyzer, which has a number of method to check if
text data(such as stdout) contains strings, matches regexp and so on. Other
possible analyzers are ClassFileAnalyzer which has methods to assert on
structure and data of classfile, e.g. class file version, class pool entries,
etc; HsErrAnalyzer which knows about structure of hs_err file and has methods
to assert on its content. In order to prevent later changes in tests, I’d
suggest you to adopt your current fix to this as well.
In order to make possible to use builder/fluent API w/ ProcessResult, I suggest
to have smth similar to the next signatures for analyze methods:
ProcessResult analyze(String, Analyzer::, Consumer>);
ProcessResult stdout(Consumer< TextAnalyzer >);
ProcessResult stderr(Consumer< TextAnalyzer >);
So in tests we will have:
Task
.addFlagA()
.changeB(“C”)
.run()
.stdout(
out -> out
.contains(“X”)
.doesNotContain(“Y”)
.matches(“.*”));
Thanks,
— Igor
> On Mar 6, 2017, at 5:27 PM, Alexandre (Shura) Iline
> wrote:
>
> Hi,
>
> Could you please review the suggested fox for:
> https://bugs.openjdk.java.net/browse/JDK-8159523
>
> There has been a bit of discussion on jigsaw-dev, but it perhaps make sense
> to include core-libs-dev.
>
> There have been some fixes since the review was published, so we are now at
> revision #4:
> http://cr.openjdk.java.net/~shurailine/8159523/webrev.04/
>
> The background for this fix is sufficiently captured in the original e-mail:
>> 1. An attempt was made to enhance jdk.testlibrary.ProcessTools class to
>> support some filtering java and VM options. That implementation came out
>> really overloaded (check executeModularTest(...) in [1]).
>>
Re: RFR(L) : 8176176 : fix @modules in jdk_svc tests
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 > 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 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 >>> 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 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*@" >>> >> >
Re: RFR(L) : 8176176 : fix @modules in jdk_svc tests
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.jdiShould 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 >>> 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 >>>> 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 >>>>> 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 >>>>>> 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 >&
RFR(S) : 8177374 : fix module dependency declaration in jdk_svc tests
http://cr.openjdk.java.net/~iignatyev/8177374/webrev.00/index.html > 40 lines changed: 26 ins; 13 del; 1 mod; Hi all, could you please review this changeset which fixes in a few jdk_svc tests which were missed by JDK-8176176[1]? testing: :jdk_svc tests webrev: http://cr.openjdk.java.net/~iignatyev/8177374/webrev.00/index.html JBS: https://bugs.openjdk.java.net/browse/JDK-8177374 [1] https://bugs.openjdk.java.net/browse/JDK-8176176 Thanks, -- Igor
Re: RFR(S) : 8177374 : fix module dependency declaration in jdk_svc tests
Hi Mandy, I've (re)sorted @modules in all the tests which I touched -- http://cr.openjdk.java.net/~iignatyev/8177374/webrev.01/index.html <http://cr.openjdk.java.net/~iignatyev/8177374/webrev.01/index.html> > test/java/lang/management/PlatformLoggingMXBean/LoggingMXBeanTest.java > This test should need java.management. Where is it declared? it's declared in test/java/lang/management/TEST.properties which has been pushed by JDK-8176176. Thanks, -- Igor > On Mar 22, 2017, at 2:01 PM, Mandy Chung wrote: > > >> On Mar 22, 2017, at 1:09 PM, Igor Ignatyev wrote: >> >> http://cr.openjdk.java.net/~iignatyev/8177374/webrev.00/index.html >>> 40 lines changed: 26 ins; 13 del; 1 mod; >> >> Hi all, >> >> could you please review this changeset which fixes in a few jdk_svc tests >> which were missed by JDK-8176176[1]? >> >> testing: :jdk_svc tests >> webrev: http://cr.openjdk.java.net/~iignatyev/8177374/webrev.00/index.html >> JBS: https://bugs.openjdk.java.net/browse/JDK-8177374 > > > test/java/lang/management/PlatformLoggingMXBean/LoggingMXBeanTest.java > This test should need java.management. Where is it declared? > Does the webrev miss some new file? > > Can you keep @modules sorted? e.g. the following in a few files: > 42 * @modules jdk.management.agent/jdk.internal.agent > 43 * java.management > 44 * jdk.attach > Mandy >
Re: RFR(S) : 8177374 : fix module dependency declaration in jdk_svc tests
Mandy/Daniel, yes, jdk.management.agent does require java.management, but not transitively. Shura and I have discussed that and agreed that in such cases a test should declare dependency explicitly, otherwise it can start to fail when some of transitive requires (which are not a part of the contract) are changed. I used jdeps with the post-proceccing which makes reduction similar to list-reduced-deps. I have run 'jdeps --list-reduced-deps' on classes from sun/management/jmxremote/bootstrap/CustomLauncherTest.java test run, and it showed the same: >java.management >jdk.attach >jdk.management.agent/jdk.internal.agent >unnamed module: > /tmp/run/jdk/sun/management/jmxremote/bootstrap/JTwork-sun-management-jmxremote-bootstrap-CustomLauncherTest-java_0/classes Thanks, -- Igor > On Mar 23, 2017, at 9:39 AM, Mandy Chung wrote: > > >> On Mar 23, 2017, at 7:33 AM, Daniel Fuchs wrote: >> >> Hi Igor, >> >> small nit: >> >> 42 * @modules java.management >> 43 * jdk.attach >> 44 * jdk.management.agent/jdk.internal.agent >> >> I don't think java.management needs to be specified as >> a dependency when the test requires jdk.management.agent, >> because jdk.management.agent already requires java.management. > > That’s true. > > Igor - How do you analyze the dependency? Are you using jdeps > —-list-reduced-deps? > > Mandy >
