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

[Igor Ignatyev]

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

1. wrong indent on 53.
2. could you pleas use Asserts.assertEquals to replace lines 56--61, 
67--71? lines 63--66 could be also replaced by Asserts.assertTrue
3. since you expect FileNotFoundException, shouldn't you use it in catch 
clause?
4. what if 'non-existing.file' exists? could you please check it in the 
very begging of test case?

OutputAnalyzerTest:
0. the same as FileOutputAnalyzerTest#2

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