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