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

[serguei.spitsyn at oracle.com]

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