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 Thanks, -- Igor > On Sep 4, 2018, at 8:01 PM, JC Beyler <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
