Hi Alex,
Looks good except for one minor nit:
249 if (epoch - here > (timeout)) {
No needs for the parens around "timeout". I don't need to see another
webrev.
thanks,
Chris
On 4/27/20 5:36 PM, Alex Menkov wrote:
updated webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/LingeredApp_improve/webrev.2/
Moved getStdoutAsList to OutputBuffer.
--alex
On 04/24/2020 20:38, Chris Plummer wrote:
On 4/24/20 6:52 PM, Alex Menkov wrote:
Hi Chris,
On 04/24/2020 15:42, Chris Plummer wrote:
Hi Alex,
Overall it looks good, although I'm not so sure I agree with
removing getAppOutput(). It's a generally useful utility API. Seems
it is better off in LingeredApp.java rather than in the one test
that currently uses it.
To me the method just adds noise to the class.
If you think it can be useful for some other tests, I'd move it to
to OutputBuffer (make it default method which uses
OutputBuffer.getStdout()):
default public List<String> getStdOutAsList()
I think that's a good idea. It looks like what tests are commonly
doing now is using String.split():
String[] appLines =
app.getOutput().getStdout().split("\\R");
I'm not sure of the pros and cons of producing a String[] this way vs
a List<String> using getStdOutAsList(). Maybe both have their uses.
But one thing for sure is the split() code is a lot more readable.
One reason I prefer getStdOutAsList() being placed in a library or
utility class is because TBH I find Streams code very hard to read,
and combining with chained Reader code doesn't help any, so I just
assume it be in a library that I'm less apt to look at rather than in
the test where I'm bound to find my eyes glazing over as I'm drawn in
to figure it out.
Chris
--alex
thanks,
Chris
On 4/24/20 3:17 PM, Alex Menkov wrote:
Hi all,
Please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8242522
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/LingeredApp_improve/webrev/
The fix contains minor fixes/improvements for LingeredApp and
tests which use it. See jira for details.
Tested all tests which use LingeredApp.
--alex
