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

[Igor Ignatyev]

On 02/20/2015 03:27 PM, denis kononenko wrote:
>
> 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.
well, it's arguable; in my opinion, String.format(...) is better.
>
>>
>> 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.
it's not a simplification, it's an incorrect test. you don't expect any 
exceptions except FileNotFoundException, but catch IOException. so I'd 
recommend you to have additional catch block for IOException w/ 
rethrowing the exception as an unchecked one.
>
>> 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.
well, I can say the almost same about the whole test. the thing is that 
your test relays on a particular behavior of jtreg and an exception 
(line 93) won't contain any useful details. so it will be very hard to 
investigate in case bugs in jtreg.
>
>>
>> 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.
>>>>>>>>>
>>>>>>>>>
>>>>>
>>>>
>>>
>