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

[David Holmes]

Sorry Chris I just don't have time to try and figure this one out. If it 
works uses it.

David

On 22/03/2018 4:24 AM, Chris Plummer wrote:
> Yeah, this was all new to me. Before this I didn't know anything about 
> jtreg IO other than the use of OutputAnalyzer for capture and verification.
> 
> Thanks for reviewing.
> 
> Chris
> 
> On 3/21/18 11:08 AM, serguei.spitsyn at oracle.com wrote:
>> Hi Chris,
>>
>> It looks good to me.
>> It is a little bit more complicated than one would expect but reasonable.
>>
>> Thanks,
>> Serguei
>>
>>
>> On 3/21/18 09:31, Chris Plummer wrote:
>>> Ping. I still need a couple of reviews for this.
>>>
>>> thanks,
>>>
>>> Chris
>>>
>>> On 3/19/18 3:50 PM, Chris Plummer wrote:
>>>> 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
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>
> 
>