RFR(S): 8198655: test/lib/jdk/test/lib/apps/LingeredApp shouldn't inherit cout/cerr

[Chris Plummer]

I looked into modifying OutputAnalyzer (actually ended up being 
ProcessTools that needed all the changes) to be more flexible so it 
could support LingeredApp. The problem I ran into is that ProcessTools 
is all static, but I needed to create and return a context. It ended up 
being too much disruption, so I instead have the 
ProcessTools.getOutput() code as part of LingeredApp.

Another thing I discovered is that you can use OutputAnalyzer with 
already generated output, so this option is still available to users of 
LingeredApp. You just need to do something like:

     OutputAnalyzer out = new 
OutputAnalyzer(lingeredApp.getOutput().getStdout(), 
lingeredApp.getOutput().getStderr());

I didn't change any test to take advantage of this, but it's there if 
someone wants it.

I've included another webrev below (completely different from the 
original). In the end, all LingeredApp stdout and stderr is dumped after 
the app exits. The old way of storing away the stdout using an 
InputGobbler is gone. Since getAppOutput() depended on this, and the new 
way of saving stdout saves it as one big string rather than a List of 
lines, getAppOutput() needed some changes to convert to the List form.

http://cr.openjdk.java.net/~cjplummer/8198655/webrev.03

thanks,

Chris

On 3/19/18 9:39 AM, Chris Plummer wrote:
> Hi David,
>
> Just to clarify one point, most of the tests that use OutputAnalyzer 
> do not display process output unless there is an error. So part of the 
> decision here with LingeredApp is when to display the output. 
> Currently the stdout is captured, but not displayed, unless the tests 
> does the work to display it, which none do. Currently stderr goes to 
> the console. Note that some negative tests actually cause some 
> expected stderr output, although the tests don't check for it.
>
> One thought I just had is to create an async option for OutputAnalyzer 
> so it doesn't block until the process exits. Basically that means 
> splitting ProcessTools.getOutput() so it doesn't block. What I 
> currently have is essentially doing that. It copies 
> ProcessTools.getOutput(), splitting it into two parts. But all this 
> logic is in LingeredApp, and of course doesn't have any of the output 
> error checking support that OutputAnalyzer, which might be useful for 
> LingeredApp. For example, the negative tests only test that launching 
> the app failed. They could be improved by checking for specific error 
> output.
>
> Chris
>
> On 3/17/18 12:11 AM, David Holmes wrote:
>> I'm afraid I'm losing track of this change.
>>
>> The key thing is that we should not have a test that launches any 
>> other process for which we can not see the output of that process.
>>
>> David
>>
>> On 17/03/2018 7:48 AM, Chris Plummer wrote:
>>> On 3/16/18 1:25 PM, serguei.spitsyn at oracle.com wrote:
>>>> Hi Chris,
>>>>
>>>> Thank you for taking care about this issue!
>>>>
>>>> On 3/16/18 11:20, Chris Plummer wrote:
>>>>> Hi,
>>>>>
>>>>> I've resolved the issues I had before with not seeing all the 
>>>>> stderr output when I tried to capture it. What I'd like to do now 
>>>>> is have us decide how the output should be handled from the 
>>>>> perspective a LingeredApp user (driver app). Currently all 
>>>>> LingeredApp stdout is captured and gets be returned the the driver 
>>>>> app by calling app.getAppOutput(). It does not appear in the .jtr 
>>>>> file, but the test would have the option of dumping it there it it 
>>>>> cared to. Only one test uses app.getAppOutput(). Currently all the 
>>>>> LingeredApp stderr is redirected to the console, so it does not 
>>>>> appear in the .jtr file.
>>>>
>>>> Just a general comment to make sure I understand it and ensure we 
>>>> are in sync.
>>>> It seems much more safe to always have both stdout and stderr 
>>>> outputs present in the .jtr automatically file independently of of 
>>>> what the test does.
>>>>
>>>>
>>>>> So how do we want this changed? Some possibilities are:
>>>>>
>>>>> (1) capture stderr just like stdout currently is, and leave is up 
>>>>> the the driver app to decide if it wants to display it (after the 
>>>>> app terminates).
>>>>
>>>> It does not look good to me (see above) but maybe I'm missing 
>>>> something important here.
>>>>
>>>>> (2) capture stderr just like stdout currently is, but have 
>>>>> LingeredApp automatically send captured output to driver app's 
>>>>> stdout and stderr (after the app terminates).
>>>>
>>>> The stdout and std err will be separated in this case, right?
>>>> Do you have a webrev for this?
>>> I currently have it working like this, although I need to fix 
>>> LingeredApp.getAppOutput(). I had to make it return a single String 
>>> instead of a List of Strings, so this breaks the one test that uses 
>>> this API. It's easily fixed. Just haven't gotten around to it yet.
>>>>
>>>>
>>>>> (3) send the LingeredApp's stdout and stderr to the driver app's 
>>>>> stdout as it is being captured (this was the original fix Igor 
>>>>> suggested and the webrev supported). A minor alternative to this 
>>>>> is to keep the two streams separated instead of sending both to 
>>>>> stdout.
>>>>>
>>>>> Let me know what you think. I'm inclined to go with 2, especially 
>>>>> since normally there is little to no output from the LingeredApp.
>>>>
>>>> The choice (2) looks good enough.
>>>> Not sure it is that important to have output from stdout and stderr 
>>>> sync'ed
>>>> but is is important to have the stderr present in the .jtr 
>>>> automatically.
>>>>
>>>> The choice (3) looks even better if it is going to work well.
>>> This is basically what the original webrev did. It sent 
>>> LingeredApp's stderr and stdout to the the driver apps stdout. It's 
>>> a 1 word change to make it send stderr to stderr. I think it has a 
>>> bug though that did not manifest itself. It seems the new copy() 
>>> code that is capturing stdout would be contending with the existing 
>>> InputGlobbler code that is doing the same. I would need to fix this 
>>> to make sure LingeredApp.getAppOutput() still returns all the apps 
>>> stdout output.
>>>
>>> Chris
>>>> Not sure, it is really necessary.
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>>>
>>>>> BTW, here's the CR and original webrev for reference:
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8198655
>>>>> http://cr.openjdk.java.net/~cjplummer/8198655/webrev.00/webrev/
>>>>>
>>>>> thanks,
>>>>>
>>>>> Chris
>>>>>
>>>>
>>>
>