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

Wed Mar 21 18:24:55 UTC 2018

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