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

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR(S): 8198655: test/lib/jdk/test/lib/apps/LingeredApp shouldn't inherit cout/cerr
From:       Igor Ignatyev <igor.ignatyev () oracle ! com>
Date:       2018-03-22 4:02:54
Message-ID: F34EB7DF-B389-451C-BC32-4246EC9AA585 () oracle ! com
[Download RAW message or body]

Hi Chris,

the changeset looks reasonable, reviewed.

Thanks,
-- Igor

> On Mar 21, 2018, at 3:26 PM, David Holmes <david.holmes@oracle.com> wrote:
> 
> 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@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@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
> > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > 
> > > > > > 
> > > > > 
> > > > > 
> > > > 
> > > > 
> > > 


[prev in list] [next in list] [prev in thread] [next in thread]
Configure | About | News | Add a list | Sponsored by KoreLogic