JC, thanks for your review! Core-libs team, as the majority of changed tests are core-libs tests, I'd really appreciate if someone from core-libs (preferably a Reviewer) could review the patch.
for the record, [1] is the latest version of webrev. [1] http://cr.openjdk.java.net/~iignatyev//8210112/webrev.02/index.html <http://cr.openjdk.java.net/~iignatyev//8210112/webrev.02/index.html> > 2433 lines changed: 334 ins; 1677 del; 422 mod; Cheers, -- Igor > On Sep 5, 2018, at 5:03 PM, JC Beyler <jcbey...@google.com> wrote: > > 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.ignat...@oracle.com > <mailto:igor.ignat...@oracle.com>> wrote: > Hi JC, > > >> On Sep 5, 2018, at 2:59 PM, JC Beyler <jcbey...@google.com >> <mailto:jcbey...@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.ignat...@oracle.com >> <mailto:igor.ignat...@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 <jcbey...@google.com >>> <mailto:jcbey...@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.ignat...@oracle.com >>> <mailto:igor.ignat...@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 > > > > -- > > Thanks, > Jc
