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

JC Beyler jcbeyler at google.com
Thu Sep 6 00:03:29 UTC 2018


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

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

-- 

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


More information about the serviceability-dev mailing list