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
