RFR: 8159523. Fix tests depending on absence of -limitmods in VM arguments.

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8159523. Fix tests depending on absence of -limitmods in VM arguments.

Alexandre (Shura) Iline
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]).
> 2. It was suggested that a builder API could be introduced instead. “toolbox” classes from langtools test library were suggested as a start [2].
> 3. Further on, it was agreed that the classes need to be copied into the JDK test library rather than updated in place or somehow else shared between langtools and jdk test libraries.

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.

Thank you

Shura


> Hi,
>
> Please review a fix for https://bugs.openjdk.java.net/browse/JDK-8159523
>
> To recap what has happened in the past.
>
> 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]).
> 2. It was suggested that a builder API could be introduced instead. “toolbox” classes from langtools test library were suggested as a start [2].
> 3. Further on, it was agreed that the classes need to be copied into the JDK test library rather than updated in place or somehow else shared between langtools and jdk test libraries.
>
> I have started porting a few tasks (JavaTask, JavacTask and JarTask) only to realize that there are many questions which may cause design discussions. So, instead, I have rolled back to one class (JavaTask) and the superclasses. If the design is acceptable, more tasks could be added as needed.
>
> The webrev: http://cr.openjdk.java.net/~shurailine/8159523/webrev.01
>
> Thank you.
>
> Shura
>
> [1] http://cr.openjdk.java.net/~shurailine/8159523/webrev.00/test/lib/testlibrary/jdk/testlibrary/ProcessTools.java.sdiff.html
>
> [2] http://hg.openjdk.java.net/jdk9/jdk9/langtools/file/8e9e1a2373a4/test/tools/lib/toolbox

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8159523. Fix tests depending on absence of -limitmods in VM arguments.

Mandy Chung

> On Mar 6, 2017, at 5:27 PM, Alexandre (Shura) Iline <[hidden email]> 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/
>

I reviewed webrev.04 which is in a good shape.  Here are my comments:

W.r.t. the JavaTask methods:

110         new JavaTask()
111             .addExports("java.base", "jdk.internal.reflect")
112             .vmOptions("-version")
113             .run();

I prefer it to use <module>/<package> to represent the source in the same way as the command-line argument to `—-add-exports` option, as Alan suggests.  Also specifying the target explicitly makes the test clearer what it does.

  .addExports(“java.base/jdk.internal.reflect”, “ALL-UNNAMED”)

Similiarly, module(String mid) takes the argument passed to `—-module`.

addReads(String source, String… targets)
  I suggest to require a non-empty targets.  i.e. ALL-UNNAMED must be specified.

`—-add-modules` is a repeating option and JavaTask::addModules does not support that.

148             .addModules("jdk.naming.dns,jdk.compiler”)

This should be “addModules(“jdk.naming.dns”, “jdk.compiler”)”

JavaTask should be extended for the new `-—add-opens` option

classArgs(String… args) might be better to rename to mainArgs(String… args).

modulePath, upgradeModulePath, classPath - maybe just take Path arguments.  

136             .shouldContain(Task.OutputKind.STDERR, "IllegalAccessError");
164             .shouldContain(Task.OutputKind.STDOUT, "specified more than once”);

It may be useful to add “stdoutShouldContain” and “stderrShouldContain” method, like what ProcessTools provides.

ignoreStandardOptions()
ignoreStandardModuleOptions()
 It might be confusing what “standard options” are.  They are the options configured by jtreg and inherited from the test run.

What about “doNotInheritTestOptions” or “doNotInheritTestRuntimeOptions”?

It seems better for JavaTask constructor to take a boolean/enum flag. Instead of a public JavaTask constructor, what about JavaTask.newTask(Enum) or JavaTask.newTaskWithInheritTestRuntimeOptions() explicit factory methods? Either way is fine with me.

public JavaTask ignoreStandardOption(String option, int parameterCount) {
public JavaTask filterStandardOption(Function<List<String>, List<String>> filter) {
  - should these methods be private?

what if classArgs and module are both called?  The builder should detect that and throw IAE to help troubleshooting since the main class should either be in a named module or just class name.

TaskError - Alan suggests to be TaskException extends RuntimeException instead.

AbstractTask.java

System.out.println("Running " + pb.command().stream().map(s -> "\"" + s + "\"").collect(Collectors.joining(",")));

- it would be useful to print the command-line that I can cut-n-paste to a shell and rerun.

i.e. no need to wrap with double quotes except -classpath and --module-path etc.  Use space rather than “,” as separator.

Task.Mode.* is defined to launch via different mechanism.  For java launcher, it has only one way.  For jar, jlink, jmod and other tools, we can now use java.util.ToolProvider.  It might be useful if we had a JlinkTask in the future.  We should either drop Task.Mode or update its comments.

Should we remove the use of JDKToolFinder since I assume jdk.testlibrary.task will replace jdk.testlibrary.* helper classes in the future.

I suggest the package name be “jdk.testlibrary.task” singular rather than plural.

Mandy

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8159523. Fix tests depending on absence of -limitmods in VM arguments.

Alexandre (Shura) Iline
Thank you, Mandy!

Comments inline.

> On Mar 6, 2017, at 6:00 PM, Mandy Chung <[hidden email]> wrote:
>
>
>> On Mar 6, 2017, at 5:27 PM, Alexandre (Shura) Iline <[hidden email]> 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/
>>
>
> I reviewed webrev.04 which is in a good shape.  Here are my comments:
>
> W.r.t. the JavaTask methods:
>
> 110         new JavaTask()
> 111             .addExports("java.base", "jdk.internal.reflect")
> 112             .vmOptions("-version")
> 113             .run();
>
> I prefer it to use <module>/<package> to represent the source in the same way as the command-line argument to `—-add-exports` option, as Alan suggests.  

I see a point in doing that. At the same time I want to point out that there will be cases when the user will be forced to do a concatenation such as
.addExports(JAVA_BASE + “/“ + JDK_MISC, ALL_UNNAMED)
i.e. JAVA_BASE is a constant which is used multiple times in the code.

This not a deal breaker - I will change it in the next iteration.

> Also specifying the target explicitly makes the test clearer what it does.
>
>  .addExports(“java.base/jdk.internal.reflect”, “ALL-UNNAMED”)

Makes sense.

I hope though that you are not against having a constant for “ALL-UNNAMED”, such as Task.ALL_UNNAMED. A lot of people, myself included prefer usage of constants over the copy-pasted value.

>
> Similiarly, module(String mid) takes the argument passed to `—-module`.
>
> addReads(String source, String… targets)
>  I suggest to require a non-empty targets.  i.e. ALL-UNNAMED must be specified.
>
> `—-add-modules` is a repeating option and JavaTask::addModules does not support that.
OK.
>
> 148             .addModules("jdk.naming.dns,jdk.compiler”)
>
> This should be “addModules(“jdk.naming.dns”, “jdk.compiler”)”
>
> JavaTask should be extended for the new `-—add-opens` option
Yes.
>
> classArgs(String… args) might be better to rename to mainArgs(String… args).
OK.
>
> modulePath, upgradeModulePath, classPath - maybe just take Path arguments.  
>
> 136             .shouldContain(Task.OutputKind.STDERR, "IllegalAccessError");
> 164             .shouldContain(Task.OutputKind.STDOUT, "specified more than once”);
>
> It may be useful to add “stdoutShouldContain” and “stderrShouldContain” method, like what ProcessTools provides.

There will be a further discussion on this - Igor will be summarizing it in a separate e-mail. What he will be suggesting is
.stderr().shouldContain(“IllegalAccessError”)
or something similar.

>
> ignoreStandardOptions()
> ignoreStandardModuleOptions()
> It might be confusing what “standard options” are.  They are the options configured by jtreg and inherited from the test run.
>
> What about “doNotInheritTestOptions” or “doNotInheritTestRuntimeOptions”?

Sounds good to me.

>
> It seems better for JavaTask constructor to take a boolean/enum flag. Instead of a public JavaTask constructor, what about JavaTask.newTask(Enum) or JavaTask.newTaskWithInheritTestRuntimeOptions() explicit factory methods? Either way is fine with me.
>
> public JavaTask ignoreStandardOption(String option, int parameterCount) {
> public JavaTask filterStandardOption(Function<List<String>, List<String>> filter) {
>  - should these methods be private?

Yes for filterStandardOption(…).
I meant  ignoreStandardOption(String, int)  as a public API. Some options are supported by JavaOptions, but there could be others which are not. But I agree that we can hide it for now - there may never be a case for it.

>
> what if classArgs and module are both called?  The builder should detect that and throw IAE to help troubleshooting since the main class should either be in a named module or just class name.

Yes.

>
> TaskError - Alan suggests to be TaskException extends RuntimeException instead.

OK.

>
> AbstractTask.java
>
> System.out.println("Running " + pb.command().stream().map(s -> "\"" + s + "\"").collect(Collectors.joining(",")));
>
> - it would be useful to print the command-line that I can cut-n-paste to a shell and rerun.

Makes sense.

>
> i.e. no need to wrap with double quotes except -classpath and --module-path etc.  Use space rather than “,” as separator.
>
> Task.Mode.* is defined to launch via different mechanism.  For java launcher, it has only one way.  For jar, jlink, jmod and other tools, we can now use java.util.ToolProvider.  It might be useful if we had a JlinkTask in the future.  We should either drop Task.Mode or update its comments.
>
> Should we remove the use of JDKToolFinder since I assume jdk.testlibrary.task will replace jdk.testlibrary.* helper classes in the future.

Probably also make it private for now. Or remove - we can re-introduce it later.

>
> I suggest the package name be “jdk.testlibrary.task” singular rather than plural.
ok

It will also need to go to the top-level test library, to allow HS team to use it.

Shura
>
> Mandy
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8159523. Fix tests depending on absence of -limitmods in VM arguments.

Mandy Chung

> On Mar 8, 2017, at 5:21 PM, Alexandre (Shura) Iline <[hidden email]> wrote:
>> Also specifying the target explicitly makes the test clearer what it does.
>>
>> .addExports(“java.base/jdk.internal.reflect”, “ALL-UNNAMED”)
>
> Makes sense.
>
> I hope though that you are not against having a constant for “ALL-UNNAMED”, such as Task.ALL_UNNAMED. A lot of people, myself included prefer usage of constants over the copy-pasted value.

I am not against defining a constant variable (I see the benefit of doing that to avoid typo).  However, Task is not the right class to define ALL_UNNAMED.  Another alternative is to introduce a new Modules class to define those strings which seems overkill.  A typo will be caught when the test is run and it isn’t too bad.

>>
>> 136             .shouldContain(Task.OutputKind.STDERR, "IllegalAccessError");
>> 164             .shouldContain(Task.OutputKind.STDOUT, "specified more than once”);
>>
>> It may be useful to add “stdoutShouldContain” and “stderrShouldContain” method, like what ProcessTools provides.
>
> There will be a further discussion on this - Igor will be summarizing it in a separate e-mail. What he will be suggesting is
> .stderr().shouldContain(“IllegalAccessError”)
> or something similar.
>

That may be fine.

>
> Yes for filterStandardOption(…).
> I meant  ignoreStandardOption(String, int)  as a public API. Some options are supported by JavaOptions, but there could be others which are not. But I agree that we can hide it for now - there may never be a case for it.

Sounds good.  Add it when we identify a need for it.

>
>>
>> Should we remove the use of JDKToolFinder since I assume jdk.testlibrary.task will replace jdk.testlibrary.* helper classes in the future.
>
> Probably also make it private for now. Or remove - we can re-introduce it later.

I suggest to remove it as a clean start.

Mandy
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8159523. Fix tests depending on absence of -limitmods in VM arguments.

Igor Ignatyev
In reply to this post by Alexandre (Shura) Iline
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::<init>);
   Set<String> 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<E>::<init>, Consumer<Analyze<E>>);
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 <[hidden email]> 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]).
>> 2. It was suggested that a builder API could be introduced instead. “toolbox” classes from langtools test library were suggested as a start [2].
>> 3. Further on, it was agreed that the classes need to be copied into the JDK test library rather than updated in place or somehow else shared between langtools and jdk test libraries.
>
> 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.
>
> Thank you
>
> Shura
>
>
>> Hi,
>>
>> Please review a fix for https://bugs.openjdk.java.net/browse/JDK-8159523
>>
>> To recap what has happened in the past.
>>
>> 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]).
>> 2. It was suggested that a builder API could be introduced instead. “toolbox” classes from langtools test library were suggested as a start [2].
>> 3. Further on, it was agreed that the classes need to be copied into the JDK test library rather than updated in place or somehow else shared between langtools and jdk test libraries.
>>
>> I have started porting a few tasks (JavaTask, JavacTask and JarTask) only to realize that there are many questions which may cause design discussions. So, instead, I have rolled back to one class (JavaTask) and the superclasses. If the design is acceptable, more tasks could be added as needed.
>>
>> The webrev: http://cr.openjdk.java.net/~shurailine/8159523/webrev.01
>>
>> Thank you.
>>
>> Shura
>>
>> [1] http://cr.openjdk.java.net/~shurailine/8159523/webrev.00/test/lib/testlibrary/jdk/testlibrary/ProcessTools.java.sdiff.html
>>
>> [2] http://hg.openjdk.java.net/jdk9/jdk9/langtools/file/8e9e1a2373a4/test/tools/lib/toolbox
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8159523. Fix tests depending on absence of -limitmods in VM arguments.

Dmitry Fazunenko
Hi Igor,

thanks for the good summary of our discussion.

Shura,

I agree with Igor position. To implement VM Launching specific (Verundy
project) we don't need any Task functionality, so I don't have any
comments on that.

VM and JDK have a lot of common in dealing with output. Representation
and utilities for analysis could be certainly shared across all JDK
components. The way it's currently implemented in your change doesn't
satisfy VM requirements and we hope it could be updated to meet our
needs. I think, if we spend a bit more time now on better designing of
the process output representation interface and API for analysis, we
will safe much time avoiding mass test update in the future.

Thanks,
Dima



On 10.03.2017 4:50, Igor Ignatyev wrote:

> 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::<init>);
>     Set<String> 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<E>::<init>, Consumer<Analyze<E>>);
> 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 <[hidden email]> 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]).
>>> 2. It was suggested that a builder API could be introduced instead. “toolbox” classes from langtools test library were suggested as a start [2].
>>> 3. Further on, it was agreed that the classes need to be copied into the JDK test library rather than updated in place or somehow else shared between langtools and jdk test libraries.
>> 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.
>>
>> Thank you
>>
>> Shura
>>
>>
>>> Hi,
>>>
>>> Please review a fix for https://bugs.openjdk.java.net/browse/JDK-8159523
>>>
>>> To recap what has happened in the past.
>>>
>>> 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]).
>>> 2. It was suggested that a builder API could be introduced instead. “toolbox” classes from langtools test library were suggested as a start [2].
>>> 3. Further on, it was agreed that the classes need to be copied into the JDK test library rather than updated in place or somehow else shared between langtools and jdk test libraries.
>>>
>>> I have started porting a few tasks (JavaTask, JavacTask and JarTask) only to realize that there are many questions which may cause design discussions. So, instead, I have rolled back to one class (JavaTask) and the superclasses. If the design is acceptable, more tasks could be added as needed.
>>>
>>> The webrev: http://cr.openjdk.java.net/~shurailine/8159523/webrev.01
>>>
>>> Thank you.
>>>
>>> Shura
>>>
>>> [1] http://cr.openjdk.java.net/~shurailine/8159523/webrev.00/test/lib/testlibrary/jdk/testlibrary/ProcessTools.java.sdiff.html
>>>
>>> [2] http://hg.openjdk.java.net/jdk9/jdk9/langtools/file/8e9e1a2373a4/test/tools/lib/toolbox

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8159523. Fix tests depending on absence of -limitmods in VM arguments.

Alexandre (Shura) Iline
In reply to this post by Igor Ignatyev
Igor, let me capture our offline conversation.

The design you are suggesting is aimed to allow to chain methods which check different outputs: stderr, stdout and other possible artifacts of a process execution. To that end,
1. the method which you call stdout(Consumer<TextOutput>) should really the called analyzeStdout(…) or somehow similar.
2. originally suggested design seems a little bit more straightforward for simple cases. Compare
new JavaTask(…)…stdout().shouldContain(“X”)
to
new JavaTask(…)…analyzeStdout(out -> out.shouldContain(“X”));

The other point which your design was suggesting was that stderr and stdout is similar to other artifacts, which, after the conversation we have had may not be the best assumption. The rest of artifacts are files and so do not necessarily have to have same or similar API to be provided to work with them.

Witt that, under “Making Easy Things Easy & Hard Things Possible” paradigm, it makes sense to proceed with my original design and think more on introducing API for handling file artifacts.

Shura

> On Mar 9, 2017, at 5:50 PM, Igor Ignatyev <[hidden email]> wrote:
>
> 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::<init>);
>   Set<String> 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<E>::<init>, Consumer<Analyze<E>>);
> 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 <[hidden email]> 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]).
>>> 2. It was suggested that a builder API could be introduced instead. “toolbox” classes from langtools test library were suggested as a start [2].
>>> 3. Further on, it was agreed that the classes need to be copied into the JDK test library rather than updated in place or somehow else shared between langtools and jdk test libraries.
>>
>> 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.
>>
>> Thank you
>>
>> Shura
>>
>>
>>> Hi,
>>>
>>> Please review a fix for https://bugs.openjdk.java.net/browse/JDK-8159523
>>>
>>> To recap what has happened in the past.
>>>
>>> 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]).
>>> 2. It was suggested that a builder API could be introduced instead. “toolbox” classes from langtools test library were suggested as a start [2].
>>> 3. Further on, it was agreed that the classes need to be copied into the JDK test library rather than updated in place or somehow else shared between langtools and jdk test libraries.
>>>
>>> I have started porting a few tasks (JavaTask, JavacTask and JarTask) only to realize that there are many questions which may cause design discussions. So, instead, I have rolled back to one class (JavaTask) and the superclasses. If the design is acceptable, more tasks could be added as needed.
>>>
>>> The webrev: http://cr.openjdk.java.net/~shurailine/8159523/webrev.01
>>>
>>> Thank you.
>>>
>>> Shura
>>>
>>> [1] http://cr.openjdk.java.net/~shurailine/8159523/webrev.00/test/lib/testlibrary/jdk/testlibrary/ProcessTools.java.sdiff.html
>>>
>>> [2] http://hg.openjdk.java.net/jdk9/jdk9/langtools/file/8e9e1a2373a4/test/tools/lib/toolbox
>>
>