Hi Chris, the changeset looks reasonable, reviewed.
Thanks, -- Igor > On Mar 21, 2018, at 3:26 PM, David Holmes <david.hol...@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.spit...@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.spit...@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 >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>> >>>>> >>>>> >>>> >>>> >>>
