RFR (S): 8072687: Update OutputAnalyzer to work with files

[denis kononenko]

Hi Igor,

On 20.02.2015 0:07, Igor Ignatyev wrote:
> Hi Denis,
>
> FileOutputAnalyzerTest:
> 0. why don't you use Utils.NEW_LINE or String.format(...%n...) instead of
>>   46         String expected =
>>   47             "line1" + System.lineSeparator()
>>   48             + "line2" + System.lineSeparator()
>>   49             + "line3";

It has more clear visual representation of the expected content. In 
contrast to Utils.NEW_LINE the System.lineSeparator is a standard 
library method. Ideally I'd prefer to generate random content but Dima 
told me this would complicate the test.

>
> 1. wrong indent on 53.

Thanks.

> 2. could you pleas use Asserts.assertEquals to replace lines 56--61, 
> 67--71? lines 63--66 could be also replaced by Asserts.assertTrue

It makes sense.

> 3. since you expect FileNotFoundException, shouldn't you use it in 
> catch clause?

In such case I should have two catch clauses, one for 
FileNotFoundException and another for IOException. For purpose of 
simplicity I'd prefer to catch more generic exception.

> 4. what if 'non-existing.file' exists? could you please check it in 
> the very begging of test case?

It shouldn't be possible as Jtreg ensures that the test is running in a 
newly created and clean scratch directory.

>
> OutputAnalyzerTest:
> 0. the same as FileOutputAnalyzerTest#2

Thank you,
Denis.

>
> Thanks,
> Igor
>
> On 02/19/2015 05:17 PM, Staffan Larsen wrote:
>> Looks good!
>>
>> Thanks,
>> /Staffan
>>
>>> On 19 feb 2015, at 13:42, denis kononenko 
>>> <denis.kononenko at oracle.com> wrote:
>>>
>>>
>>> Hi Dima, Staffan,
>>>
>>> I've fixed the review findings, here's a new webrev link:
>>>
>>> http://cr.openjdk.java.net/~dfazunen/dkononen/8072687/webrev.01/
>>>
>>> I found that CompressedClassSpaceSizeInJmapHeap.java could be 
>>> improved without use of the new proposed functionality. The 
>>> OutputAnalyzer works perfectly with Process objects and there's no 
>>> need to capture stderr/stdout into two separate files as it's 
>>> already done by the OutputAnalyzer. So I simply removed all stuff 
>>> which duplicated this functionality.
>>> All necessary tests were performed.
>>>
>>> Thank you,
>>> Denis.
>>>
>>> On 18.02.2015 12:56, Dmitry Fazunenko wrote:
>>>>
>>>> On 18.02.2015 12:34, Staffan Larsen wrote:
>>>>> I can only see one usage of the OutputAnalyzer(String) constructor 
>>>>> and that is in 
>>>>> test/gc/metaspace/CompressedClassSpaceSizeInJmapHeap.java. That 
>>>>> usage does not require both stdout and stderr to be set.
>>>>>
>>>>> In fact, that usage would benefit from the new constructor that 
>>>>> takes a File.
>>>>>
>>>>> I suggest we do:
>>>>>
>>>>> - Change OutputAnalyzer(String) to set stdout to the input string 
>>>>> and stderr to the empty string ""
>>>>> - Update CompressedClassSpaceSizeInJmapHeap.java to use the new 
>>>>> OutputAnalyzer(File) constructor
>>>>
>>>> sounds good to me.
>>>>
>>>> -- Dima
>>>>
>>>>>
>>>>> Thanks,
>>>>> /Staffan
>>>>>
>>>>>
>>>>>> On 17 feb 2015, at 19:56, Dmitry Fazunenko 
>>>>>> <Dmitry.Fazunenko at oracle.com> wrote:
>>>>>>
>>>>>> Hi Staffan,
>>>>>>
>>>>>> For me it's not clear why the constructor taking a String 
>>>>>> argument sets both stderr and stdout to the same value...
>>>>>> But I don't see any reason to update this behavior and break 
>>>>>> compatibility.
>>>>>> So I would agree, that the new constructor should be consistent 
>>>>>> with the existing one and set both out and err to the same string.
>>>>>>
>>>>>> Thanks,
>>>>>> Dima
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 17.02.2015 20:39, Staffan Larsen wrote:
>>>>>>> There is constructor that takes a String argument. That 
>>>>>>> constructor sets both stderr and stdout to that same String. 
>>>>>>> Your constructor behaves differently and only sets stdout to the 
>>>>>>> contents of the file. I have no idea why the existing 
>>>>>>> constructor does what it does, but it would be good if the new 
>>>>>>> and old had the same behavior. I haven’t looked at the use cases 
>>>>>>> for the existing constructor.
>>>>>>>
>>>>>>> /S
>>>>>>>
>>>>>>>> On 17 feb 2015, at 16:58, denis kononenko 
>>>>>>>> <denis.kononenko at oracle.com> wrote:
>>>>>>>>
>>>>>>>> Hi All,
>>>>>>>>
>>>>>>>> Could you please review a small change of OutputAnalyzer class 
>>>>>>>> from testlibrary.
>>>>>>>>
>>>>>>>> Webrev link: 
>>>>>>>> http://cr.openjdk.java.net/~dfazunen/dkononen/8072687/webrev.00/
>>>>>>>> Bug id: https://bugs.openjdk.java.net/browse/JDK-8072687
>>>>>>>> Testing: automated
>>>>>>>> Description:
>>>>>>>>
>>>>>>>> The purpose of this change is to extend the functionality of 
>>>>>>>> OutputAnalyzer by adding ability to read output from a file. 
>>>>>>>> Sometimes we have to analyze output written to a file (test 
>>>>>>>> data, logs, etc.). In that case we have to read it as a string 
>>>>>>>> and pass it into OutputAnalyzer's constructor (it could be done 
>>>>>>>> in several different ways). It would be very convenient to put 
>>>>>>>> this code directly into OutputAnalyzer. This would allow to 
>>>>>>>> make further tests code more cleaner and less error prone.
>>>>>>>>
>>>>>>>> The change consist of two parts:
>>>>>>>> 1) The change: OutputAnalyzer.java, added a new constructor 
>>>>>>>> that accepts File;
>>>>>>>> 2) The tests: FileOutputAnalyzerTests.java, very basic tests on 
>>>>>>>> reading functionality.
>>>>>>>>
>>>>>>>> Thank you,
>>>>>>>> Denis.
>>>>>>>>
>>>>>>>>
>>>>
>>>
>>