RFR(L/M) : 8210112 : remove jdk.testlibrary.ProcessTools

[JC Beyler]

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
     - 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
  - 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.ignatyev at oracle.com>
wrote:

> 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
> testing: tier1-tier3 + :jdk_svc
> JBS: https://bugs.openjdk.java.net/browse/JDK-8210112
>
> Thanks,
> -- Igor
>
>

-- 

Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180904/e8b7249f/attachment-0001.html>