RFR: 8343074: test/jdk/com/sun/net/httpserver/docs/test1/largefile.txt could be generated [v4]

[Volkan Yazici]

On Tue, 4 Feb 2025 10:46:33 GMT, Daniel Fuchs <dfuchs at openjdk.org> wrote:

>> Volkan Yazici has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - Assert that multiple files can be served using the same `FileServerHandler`
>>  - Remove redundant `@build` dependencies
>
> test/jdk/com/sun/net/httpserver/SelCacheTest.java line 149:
> 
>> 147:         }
>> 148:         Path tempPath = temp.toPath();
>> 149:         assert Files.mismatch(filePath, tempPath) < 0;
> 
> I would suggest using `assertEquals` with -1 here to ensure that:
> 
> 1. the test will fail if the files don't match even if asserts are disabled, and
> 2. we see at which place mismatch was detected in the exception message

In 9c7c6af013e0495b08bcd248e30d83385bcc35b5,

- Used `Asserts::assertEquals` to compare `count` and `filePath.toFile().length()` (as you requested above)
- Replaced `assert Files.mismatch(...) < 0` with a **newly added** `Asserts::assertFileContentsEqual`

I opted for a new method (using `Files::mismatch` under the hood) since it

- Provides better diagnostics (source & target paths)
- Doesn't bother call-sites with `IOException`s (in line with JUnit, AssertJ, etc. assertions)
- Saves ~11 LoC at each call-site

@jaikiran, earlier you wanted me to remove `Asserts::assertFileContentsEqual` in favor of a check against `Files::mismatch`. I switched back to the old behavior, because of the advantages I shared above, plus we import `Asserts` for `assertEquals` anyway.

@dfuch, @jaikiran, I would appreciate your input before resolving this conversation.

> test/jdk/com/sun/net/httpserver/Test1.java line 160:
> 
>> 158:         }
>> 159:         Path tempPath = temp.toPath();
>> 160:         assert Files.mismatch(filePath, tempPath) < 0;
> 
> Same suggestions WRT assertEquals here

Fixed in 9c7c6af013e0495b08bcd248e30d83385bcc35b5.

> test/jdk/com/sun/net/httpserver/Test12.java line 182:
> 
>> 180:                 }
>> 181:                 Path tempPath = temp.toPath();
>> 182:                 assert Files.mismatch(filePath, tempPath) < 0;
> 
> and here as well

Fixed in 9c7c6af013e0495b08bcd248e30d83385bcc35b5.

> test/jdk/com/sun/net/httpserver/Test13.java line 184:
> 
>> 182:                 }
>> 183:                 Path tempPath = temp.toPath();
>> 184:                 assert Files.mismatch(filePath, tempPath) < 0;
> 
> ditto

Fixed in 9c7c6af013e0495b08bcd248e30d83385bcc35b5.

> test/jdk/com/sun/net/httpserver/Test9.java line 202:
> 
>> 200:                 }
>> 201:                 Path tempPath = temp.toPath();
>> 202:                 assert Files.mismatch(filePath, tempPath) < 0;
> 
> And in all other places where this pattern appears in this PR

Fixed in 9c7c6af013e0495b08bcd248e30d83385bcc35b5.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/23401#discussion_r1941055912
PR Review Comment: https://git.openjdk.org/jdk/pull/23401#discussion_r1941056394
PR Review Comment: https://git.openjdk.org/jdk/pull/23401#discussion_r1941056606
PR Review Comment: https://git.openjdk.org/jdk/pull/23401#discussion_r1941056800
PR Review Comment: https://git.openjdk.org/jdk/pull/23401#discussion_r1941056994