On Sep 5, 2018, at 5:03 PM, JC Beyler <jcbey...@google.com> wrote:
Hi Igor,
Looks good to me then. I agree most are nits, personal preferences, and just I
noticed them as you were cleaning them up!
Awesome clean up :-)
Jc
On Wed, Sep 5, 2018 at 3:20 PM Igor Ignatyev <igor.ignat...@oracle.com
<mailto:igor.ignat...@oracle.com>> wrote:
Hi JC,
On Sep 5, 2018, at 2:59 PM, JC Beyler <jcbey...@google.com
<mailto:jcbey...@google.com>> wrote:
Hi Igor,
I like this much better! A few more comments:
-
http://cr.openjdk.java.net/~iignatyev//8210112/webrev.0-1/test/jdk/lib/testlibrary/OutputAnalyzerTest.java.udiff.html
<http://cr.openjdk.java.net/~iignatyev//8210112/webrev.0-1/test/jdk/lib/testlibrary/OutputAnalyzerTest.java.udiff.html>
-> If the shouldMatch call fails, it throws an exception, why not just let that
fail test, why are you catching and then rethrowing (like you do for
http://cr.openjdk.java.net/~iignatyev//8210112/webrev.0-1/test/jdk/sun/tools/jcmd/TestJcmdDefaults.java.udiff.html
<http://cr.openjdk.java.net/~iignatyev//8210112/webrev.0-1/test/jdk/sun/tools/jcmd/TestJcmdDefaults.java.udiff.html>)
just to follow the style used by this tests, e.g.
54 try {
55 output.shouldContain(stdout);
56 output.stdoutShouldContain(stdout);
57 output.shouldContain(stderr);
58 output.stderrShouldContain(stderr);
59 } catch (RuntimeException e) {
60 throw new Exception("shouldContain() failed", e);
61 }
86 try {
87 output.shouldNotContain(nonExistingString);
88 output.stdoutShouldNotContain(nonExistingString);
89 output.stderrShouldNotContain(nonExistingString);
90 } catch (RuntimeException e) {
91 throw new Exception("shouldNotContain() failed", e);
92 }
93
-
http://cr.openjdk.java.net/~iignatyev//8210112/webrev.0-1/test/jdk/sun/tools/jcmd/TestJcmdDefaults.java.udiff.html
<http://cr.openjdk.java.net/~iignatyev//8210112/webrev.0-1/test/jdk/sun/tools/jcmd/TestJcmdDefaults.java.udiff.html>
There is now only a 1-liner for this method and it is called only once,
should we inline and remove the method?
we can, but I ain't sure we should. from my point of view
80 output.shouldHaveExitValue(0);
81 output.shouldContain("sun.tools.jcmd.JCmd");
82 matchListedProcesses(output);
is a bit easier to understand than
80 output.shouldHaveExitValue(0);
81 output.shouldContain("sun.tools.jcmd.JCmd");
82 output.shouldMatchByLine(JCMD_LIST_REGEX);
however, I don't have strong preference here and if serviceability team wants,
I can inline matchListedProcesses.
- Same for (we could inline):
http://cr.openjdk.java.net/~iignatyev//8210112/webrev.0-1/test/jdk/sun/tools/jcmd/TestJcmdSanity.java.udiff.html
<http://cr.openjdk.java.net/~iignatyev//8210112/webrev.0-1/test/jdk/sun/tools/jcmd/TestJcmdSanity.java.udiff.html>same
here
-
http://cr.openjdk.java.net/~iignatyev//8210112/webrev.0-1/test/lib/jdk/test/lib/process/OutputAnalyzer.java.udiff.html
<http://cr.openjdk.java.net/~iignatyev//8210112/webrev.0-1/test/lib/jdk/test/lib/process/OutputAnalyzer.java.udiff.html>
"There is no lines" -> "There are no lines"
fixed. thanks for spotting.
- What is the advantage of having the return at all now for the
shouldMatch methods, if it fails it throws, the test fails; otherwise it
doesn't return anything, the test can move on, no? I saw no moment when you get
the return to do something more with it
OutputAnalazyer is supposed to be a fluent interface, and in some cases you
might find it used that way, so I'd prefer to have possibility to use these
methods in a method chain, as w/ we already have for the the most of other
should* method. I've fixed a few more should* methods to return this.
Thanks for the incremental webrev, that made looking at the changes so much
easier!
here is the next one --
http://cr.openjdk.java.net/~iignatyev//8210112/webrev.1-2/index.html
<http://cr.openjdk.java.net/~iignatyev//8210112/webrev.1-2/index.html>
-- Igor
Jc
On Wed, Sep 5, 2018 at 2:32 PM Igor Ignatyev <igor.ignat...@oracle.com
<mailto:igor.ignat...@oracle.com>> wrote:
Hi JC,
thanks for reviewing this! I agree w/ both your comments and have updated the
code very similarly to your suggestion.
I've also noticed that j.t.l.p.OutputAnalyzer::shouldMatchByLine method family is a bit
different from other should* (and strange), besides checking that the lines match the
pattern, shouldMatchByLine methods do not check that it's greater than zero and return
number of matched lines instead. however all users of these methods do check that the
return results is non zero. I have updated these methods to check that there are lines to
match and updated all their users correspondingly. Doing that, I also made some harmless
refactoring, like moving Pattern::compile from loops, using "\R" as end-of-line
pattern.
incremental webrev:
http://cr.openjdk.java.net/~iignatyev//8210112/webrev.0-1/index.html
<http://cr.openjdk.java.net/~iignatyev//8210112/webrev.0-1/index.html>
Thanks,
-- Igor
On Sep 4, 2018, at 8:01 PM, JC Beyler <jcbey...@google.com
<mailto:jcbey...@google.com>> wrote:
Hi Igor,
I reviewed the webrev but I noticed two things:
- Small nit:
- In
http://cr.openjdk.java.net/~iignatyev//8210112/webrev.00/test/lib/jdk/test/lib/process/ProcessTools.java.udiff.html
<http://cr.openjdk.java.net/~iignatyev//8210112/webrev.00/test/lib/jdk/test/lib/process/ProcessTools.java.udiff.html>
- I thought we don't have to flush as the stream gets closed and by
closing flushes the stream, isn't that redundant then?
-
http://cr.openjdk.java.net/~iignatyev//8210112/webrev.00/test/lib/jdk/test/lib/process/OutputBuffer.java.udiff.html
<http://cr.openjdk.java.net/~iignatyev//8210112/webrev.00/test/lib/jdk/test/lib/process/OutputBuffer.java.udiff.html>
- Seems we could refactor a bit this no?
- If we put the Future and ByteArrayOutputStream in a separate class (ex
TaskStream), then the constructor and the getters could be factorized:
class TaskStream {
private final ByteArrayOutputStream buffer;
private Future<Void> task;
public TaskStream(InputStream stream) {
buffer = new ByteArrayOutputStream();
task = new StreamPumper(stream, buffer).process();
}
public String getBuffer() {
try {
task.get();
return buffer.toString();
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
throw new OutputBufferException(e);
} catch (ExecutionException | CancellationException e) {
throw new OutputBufferException(e);
}
}
}
+ class LazyOutputBuffer implements OutputBuffer {
+ private final TaskStream stderr;
+ private final TaskStream stdout;
+ private final Process p;
+
+ private LazyOutputBuffer(Process p) {
+ this.p = p;
+ stderr = new TaskStream(p.getInputStream());
+ stdout = new TaskStream(p.getErrorStream());
+ }
+
+ @Override
+ public String getStdout() {
+ return stdout.getBuffer();
+ }
+ @Override
+ public String getStderr() {
+ return stderr.getBuffer()
+ }
I think it is more clear, what do you think?
Apart from those two elements, it looks good to me :), nice refactor!
Jc
On Tue, Sep 4, 2018 at 6:33 PM Igor Ignatyev <igor.ignat...@oracle.com
<mailto:igor.ignat...@oracle.com>> wrote:
http://cr.openjdk.java.net/~iignatyev//8210112/webrev.00/index.html
<http://cr.openjdk.java.net/~iignatyev//8210112/webrev.00/index.html>
2375 lines changed: 322 ins; 1662 del; 391 mod
Hi all,
could you please review the patch which removes jdk.testlibrary.ProcessTools
and its friends and replaces all theirs usages w/ corresponding classes from
jdk.test.lib.process?
there were a few differences b/w implementations which are addressed by the
patch:
- j.t.l.p.ProcessTools missed executeProcess(ProcessBuilder, String) method
- j.t.l.p.OutputAnalyzer didn't have shouldMatchByLine methods family
- j.t.l.p.OutputBuffer was a very rudimentary and didn't serve any purposes,
while j.t.OutputBuffer provided lazy access to a process's cout, cerr and
exitcode. I have changed j.t.l.p.OutputBuffer to be an interface w/ two
implementations LazyOutputBuffer and EagerOutputBuffer, and updated
j.t.l.p.OutputAnalyzer to get values from an OutputBuffer instead of storing
them.
- j.t.l.p.ProcessTools::createJavaProcessBuilder always adds '-cp', but
j.t.ProcessTools::createJavaProcessBuilder did not. I have identified tests
which really depend on absence of '-cp' and updated them to create
ProcessBuilder directly, namely JavaClassPathTest and
AppendToClassPathModuleTest.
the rest of the patch is straightforward change of used classes w/ adding
@library /test/lib if necessary and removing @library /lib/testlibrary if
possible.
webrev: http://cr.openjdk.java.net/~iignatyev//8210112/webrev.00/index.html
<http://cr.openjdk.java.net/~iignatyev//8210112/webrev.00/index.html>
testing: tier1-tier3 + :jdk_svc
JBS: https://bugs.openjdk.java.net/browse/JDK-8210112
<https://bugs.openjdk.java.net/browse/JDK-8210112>
Thanks,
-- Igor
--
Thanks,
Jc
--
Thanks,
Jc
--
Thanks,
Jc
