On Mon, 3 Feb 2025 19:26:03 GMT, Volkan Yazici <vyaz...@openjdk.org> wrote:
>> Adds `test.lib.Utils::createTempFileOfSize` to generate
>> `test/jdk/com/sun/net/httpserver/docs` contents at runtime. This directory
>> contains `largefile.txt` of size 2.6MiB showing up as the 4th largest file
>> tracked by git:
>>
>>
>> $ git ls-files | while read f; do echo -e $(stat -c %s "$f")"\t$f"; done
>> >/tmp/trackedFileSizes.txt
>> $ sort -n /tmp/trackedFileSizes.txt | tail -n 4
>> 2730088 test/jdk/com/sun/net/httpserver/docs/test1/largefile.txt
>> 2798680 src/java.base/share/data/unicodedata/NormalizationTest.txt
>> 3574947 test/jdk/java/foreign/libTestDowncallStack.c
>> 7128495 test/jdk/java/foreign/libTestUpcallStack.c
>>
>>
>> **Other highlights:**
>>
>> - `jdk.httpclient.test.lib.common.TestUtil` is removed in favor of similar
>> alternatives in `test.lib.Utils` and `test.lib.Asserts`
>> - `test.lib.Asserts::assertFileContentsEqual` is added
>
> 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 77:
> 75: s2 = HttpsServer.create(addr, 0);
> 76: // Assert that both files share the same parent and can be
> served from the same `FileServerHandler`
> 77: assert
> smallFilePath.getParent().equals(largeFilePath.getParent());
I see what you are doing here. On the one hand I thought that it would be
better placed right after line 66, but on the other hand we want it inside the
`try { }` so that files will be deleted on `finally` even if it fires. So LGTM.
test/jdk/com/sun/net/httpserver/SelCacheTest.java line 145:
> 143: fout.close();
> 144:
> 145: if (count != filePath.toFile().length()) {
Maybe we could use assertEquals here for better diagnosability
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
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
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
test/jdk/com/sun/net/httpserver/Test13.java line 184:
> 182: }
> 183: Path tempPath = temp.toPath();
> 184: assert Files.mismatch(filePath, tempPath) < 0;
ditto
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
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23401#discussion_r1940924335
PR Review Comment: https://git.openjdk.org/jdk/pull/23401#discussion_r1940936508
PR Review Comment: https://git.openjdk.org/jdk/pull/23401#discussion_r1940935266
PR Review Comment: https://git.openjdk.org/jdk/pull/23401#discussion_r1940941328
PR Review Comment: https://git.openjdk.org/jdk/pull/23401#discussion_r1940942729
PR Review Comment: https://git.openjdk.org/jdk/pull/23401#discussion_r1940943725
PR Review Comment: https://git.openjdk.org/jdk/pull/23401#discussion_r1940945015
