RFR(JDK 12/NIO) 8202285: (fs) Add a method to Files for comparing file contents

[Joe Wang]

On 10/17/18, 1:08 PM, Roger Riggs wrote:
> Hi Joe,
>
> A few comments about the test.
>
> 45: the summary should mention it is testing the Files.mismatch method

Files.mismatch it is.
>
> I think the @bug line should be empty, this is not a test for a bug.

Consider along with Stuart's comment, that it points to the bug under 
which the changeset was pushed, would it be ok to keep it?

> I don't think you need or use the testng group= functionality; if you 
> don't have a need for it omit it.

Removed.
>
>
> On the test files, I think I would have used testMismatch DataProvider to
> create the files.  It would avoid having to maintain parallel lists of 
> matching parameters.
> The @beforeSetup might support the dataProvider directly,
> but might have to iterate over the array itself, creating the files if 
> they do not exist.
> And keeping the mapping from name to path in a Map instead of statics.

Removed the class fields. They were converted from an array that was 
used for @AfterClass cleanup, now that I've added a temporary test 
directory, cleanup won't need them anymore. Not much sharing among the 
DataProviders either. So now they are created inside each DataProvider 
as needed.
>
> The code should use Assert.assertEquals(actual, expected, msg) so that 
> if there is a difference
> it is printed.  (instead of assertTrue ( n==m)...)

Done.
>
>
> The file sizes all seem to be multiples of 1024 or the buffer size.
> There might be a  good case for using a more varied sizes.

Yeah, added a couple of more test groups with arbitrary sizes.
>
> 340: testMismatchNotExists:  can the expected exception be more 
> specific:  FileNotFoundException
> instead of IOException.

It appears isSameFile actually throws java.nio.file.NoSuchFileException 
rather than FileNotFoundException. Since isSameFile, as well as this 
spec, defined IOException, the expected exception spec-wise and 
impl-wise (it uses isSameFile) is indeed IOException.
>
> 370: This condition for zero made me wonder if there was a test case 
> for files that differ at 0.

Added tests for a mismatch at 0, and also at size - 1 (at the end).
>
> 397: fillArray could probably make good use of System.arraycopy().

Sure.

Current version:
http://cr.openjdk.java.net/~joehw/jdk12/8202285/webrev_v06/

Previous:
http://cr.openjdk.java.net/~joehw/jdk12/8202285/webrev_v05/

Thanks,
Joe

>
> Regards, Roger
>
>
> On 10/17/2018 03:33 PM, Joe Wang wrote:
>>
>> On 10/17/18, 7:16 AM, Roger Riggs wrote:
>>> Hi Joe,
>>>
>>> Looks good, straightforward and easy to understand.
>>
>> Glad to hear that,  while it's been back and forth a couple of times, 
>> it seems "straightforward and easy" won :-)
>>>
>>> The spec text in the CSR need to be updated to match. (At least the 
>>> paragraph about reflexive and atomic).
>>
>> CSR: update the text and attachment
>> https://bugs.openjdk.java.net/browse/JDK-8202302
>>
>> Also added coverage to spec case 5 -  reflexive (no new test cases), 
>> spec case 6 - symmetric with tests selected from case 3.
>> webrevs: http://cr.openjdk.java.net/~joehw/jdk12/8202285/webrev_v05/
>>
>> Thanks,
>> Joe
>>
>>>
>>> Thanks, Roger
>>>
>>> On 10/16/2018 02:10 PM, Joe Wang wrote:
>>>> Hi Daniel,
>>>>
>>>> @linkplain it is!  Thanks!  And yes, int totalRead was fixed in the 
>>>> previous webrevs.
>>>>
>>>> Current version:
>>>> specdiff: 
>>>> http://cr.openjdk.java.net/~joehw/jdk12/8202285/specdiff_v04/java/nio/file/Files.html 
>>>>
>>>> webrevs: http://cr.openjdk.java.net/~joehw/jdk12/8202285/webrev_v04/
>>>>
>>>> Previous version:
>>>> specdiff: 
>>>> http://cr.openjdk.java.net/~joehw/jdk12/8202285/specdiff_v03/java/nio/file/Files.html 
>>>>
>>>> webrevs: http://cr.openjdk.java.net/~joehw/jdk12/8202285/webrev_v03/
>>>>
>>>> Best regards,
>>>> Joe
>>>>
>>>> On 10/16/18, 2:11 AM, Daniel Fuchs wrote:
>>>>> Hi Joe,
>>>>>
>>>>> There are a few places where {@linkplain } should be used instead
>>>>> of {@link }. For instance:
>>>>>
>>>>> 1545      * <li> The two paths locate the {@link #isSameFile(Path, 
>>>>> Path) same file},
>>>>>
>>>>> This should be {@linkplain } - for the record {@link } will format 
>>>>> the
>>>>> text as code, {@linkplain } will format it as regular text. Since the
>>>>> text of the link is "same file" then it should be formatted as 
>>>>> regular
>>>>> text - not as code.
>>>>>
>>>>> The link on the next line should probably be {@linkplain } as well.
>>>>>
>>>>> Otherwise looks good to me (except for the declaration of
>>>>> int totalRead that should be long but Alan already mentioned it).
>>>>>
>>>>> best regards,
>>>>>
>>>>> -- daniel
>>>>>
>>>>>
>>>>> On 12/10/2018 20:16, Joe Wang wrote:
>>>>>> Here's an update based on all of the great reviews and comments 
>>>>>> (thanks all!):
>>>>>>
>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8202285
>>>>>> CSR: https://bugs.openjdk.java.net/browse/JDK-8202302
>>>>>>
>>>>>> Current version:
>>>>>> specdiff: 
>>>>>> http://cr.openjdk.java.net/~joehw/jdk12/8202285/specdiff_02/java/nio/file/Files.html 
>>>>>>
>>>>>> webrevs: http://cr.openjdk.java.net/~joehw/jdk12/8202285/webrev_v02/
>>>>>>
>>>>>> Previous version:
>>>>>> specdiff: 
>>>>>> http://cr.openjdk.java.net/~joehw/jdk12/8202285/specdiff_v01/java/nio/file/Files.html 
>>>>>>
>>>>>> webrevs: http://cr.openjdk.java.net/~joehw/jdk12/8202285/webrev_v01/
>>>>>>
>>>>>> It's been a while, so here's a summary of what's changed:
>>>>>>
>>>>>> Spec: Alan's patch to fix inconsistencies in wording/terminology
>>>>>> Impl: s/mismatchByAttrs and etc/isSameFile;
>>>>>>           Stick with the simple implementation 
>>>>>> (InputStream/Buffer) for now. Further improvement may be done if 
>>>>>> there's a need;
>>>>>>           The impl is smaller than the previous version, merged 
>>>>>> the code into Files, removed FilesMismatch.java;
>>>>>>
>>>>>> Test: more readable variables instead of the array of test files
>>>>>>           more comments on the testcases
>>>>>>           improved the private methods used for generating the 
>>>>>> test files
>>>>>>
>>>>>> Thanks,
>>>>>> Joe
>>>>>
>>>
>
> -- 
> Thanks, Roger