RFR(L/M) : 8210112 : remove jdk.testlibrary.ProcessTools
Igor Ignatyev
igor.ignatyev at oracle.com
Wed Sep 5 22:20:35 UTC 2018
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 <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>)
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 <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 <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 <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 <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 <mailto: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 <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 <mailto: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 <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.ignatyev at oracle.com <mailto:igor.ignatyev at 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
>
>
>
> --
>
> Thanks,
> Jc
More information about the core-libs-dev
mailing list