On Tue, 4 Feb 2025 10:46:33 GMT, Daniel Fuchs <dfu...@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
