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

Wed Mar 21 16:31:49 UTC 2018

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