Hi JC,
> On Sep 5, 2018, at 2:59 PM, JC Beyler <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
