On Thu, 18 Sep 2025 16:40:02 GMT, Jaikiran Pai <[email protected]> wrote:
>> Volkan Yazici has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix `FileChannelPublisherTest` failures
>
> src/java.net.http/share/classes/jdk/internal/net/http/PullPublisher.java line
> 46:
>
>> 44: private final Throwable throwable;
>> 45:
>> 46: PullPublisher(CheckedIterable<T> iterable, Throwable throwable) {
>
> Looking at the usages of this constructor, would it be better if we got rid
> of this 2-arg constructor and introduced a new
>
> PullPublisher(Throwable throwable) {
> }
>
>
> That way all we have to do is verify that the incoming argument is non-null,
> both in this constructor and the other existing
> `PullPublisher(CheckedIterable<T> iterable)`
I agree with your sentiment, which is something I raised [earlier]. I intend to
carry out this improvement in a follow-up ticket to avoid bloating this PR –
created [JDK-8368077].
[earlier]: https://github.com/openjdk/jdk/pull/26876/files#r2329782120
[JDK-8368077]: https://bugs.openjdk.org/browse/JDK-8368077
> test/jdk/java/net/httpclient/HttpRequestBodyPublishers/OfByteArraysTest.java
> line 263:
>
>> 261: * Initiates tests that depend on a custom-configured JVM.
>> 262: */
>> 263: public static void main(String[] args) throws Exception {
>
> Hello Volkan, I am not completely certain whether having a test class which
> has both a `@run main` and a `@run junit` is guaranteed to always work. What
> I mean is, I don't know if jtreg will setup the (runtime) classpath to always
> include junit libraries which will be required by this class (since it
> references those junit classes) when running the `@run main` action. It might
> be better to separate out the junit testing and the main() testing into 2
> separate test classes.
You have a very valid point. `@run main` executions should indeed fail due to
missing JUnit classes – yet it works, which is, IMHO, unexpected. Agreeing with
your sentiment, and putting aside the fact that _"it works"_, I've tried to
refactor these `@run main` tests out to separate classes, but I ended up
duplicating quite some code. Later on I've checked this issue with @sormuras
and we've found that, except `@run java`, all `@run` executions in the same
`@test` block get JUnit/TestNG added to their classpath in the presence of a
`@run junit`/`@run testng`. Since this is an established JTreg ~bug~ feature,
and helps with DRY, I will keep the code as is for the time being.
> test/jdk/java/net/httpclient/HttpRequestBodyPublishers/OfFileTest.java line
> 63:
>
>> 61: public class OfFileTest {
>> 62:
>> 63: private static final Path DEFAULT_FS_DIR =
>> Path.of(System.getProperty("java.io.tmpdir"));
>
> Is it intentional to use `java.io.tmpdir` as the Path instead of the test's
> scratch dir - `Path.of(".")`?
Fixed in 6e46e61eb9. Adopted the code in `jdk.test.lib.Utils::createTempFile`.
> test/jdk/java/net/httpclient/HttpRequestBodyPublishers/OfFileTest.java line
> 72:
>
>> 70: try {
>> 71: Path zipFile = DEFAULT_FS_DIR.resolve("file.zip");
>> 72: FileSystem zipFS = FileSystems.newFileSystem(zipFile,
>> Map.of("create", "true"));
>
> This can lead to the `FileSystem` not being closed even after the test
> completes. I think we should move out the `FileSystem` instance out of this
> method into a class level field which can then be closed when the test is
> done.
Fixed in 11a74dde85.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/26876#discussion_r2362172857
PR Review Comment: https://git.openjdk.org/jdk/pull/26876#discussion_r2362381864
PR Review Comment: https://git.openjdk.org/jdk/pull/26876#discussion_r2362233541
PR Review Comment: https://git.openjdk.org/jdk/pull/26876#discussion_r2362290580
