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

[JC Beyler]

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
  -> 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
   There is now only a 1-liner for this method and it is called only once,
should we inline and remove the method?

- 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/lib/jdk/test/lib/process/OutputAnalyzer.java.udiff.html
    "There is no lines" -> "There are no lines"
    - 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

Thanks for the incremental webrev, that made looking at the changes so much
easier!
Jc

On Wed, Sep 5, 2018 at 2:32 PM Igor Ignatyev <igor.ignatyev at 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
>
> Thanks,
> -- Igor
>
> On Sep 4, 2018, at 8:01 PM, JC Beyler <jcbeyler at 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
>      - 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
>
>
>

-- 

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