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.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
