[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: JDK-8242522: Minor LingeredApp improvements
From:       Chris Plummer <chris.plummer () oracle ! com>
Date:       2020-04-29 6:49:37
Message-ID: bd009e18-96a9-d0c0-7b3c-374e4dca32a6 () oracle ! com
[Download RAW message or body]

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
>>>>
>>
>>


[prev in list] [next in list] [prev in thread] [next in thread]