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 <igor.ignat...@oracle.com> 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.il...@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
>>
>
