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 <alexandre.iline at oracle.com> 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