On Thu, 24 Apr 2025 10:08:10 GMT, Daniel Fuchs <dfu...@openjdk.org> wrote:
> You will still need to update `java/net/URL/OpenStream.java` to make it pass
> with the new changes. Possibly have two `@run`, one with
> `-Djdk.net.file.ftpfallback=true` and one without and adapt the test
> expectation based on the presence (and value) of the property.
Hmm yes..
However, I find it somewhat dubious that 8202708 is crammed into this test
which originally was written to test the non-proxy aspect in 4064962. Using
that test to also verify that a non-existing `file://` UNC path leads to an
FtpURLConnection which then fails with an `UnknownHostException` seems a bit
convoluted and a mixing of concerns.
I think I would feel better if this aspect was instead verified in a new test
added to the recently introduced `NonLocalFtpFallback`:
/**
* Verify that opening a stream on a non-proxy URLConnection for a non-local
* file URL which is not an existing UNC path fails with UnknownHostException
* when the fallback FtpURLConnection attempts to connect to the non-existing
* FTP server.
*
* See bug 8202708
*/
public void verifyFtpConnectionException() throws IOException {
URL url = new URL("file://h7qbp368oix47/not-exist.txt");
assertThrows(UnknownHostException.class, () -> {
InputStream in = url.openConnection(Proxy.NO_PROXY).getInputStream();
});
}
We could then remove any FTP aspect from `OpenStream.java` by reverting 8202708
and keep any FTP fallback testing in the recently introduced tests (disable and
not disabled). This seems cleaner to me and also reduces duplicated / redundant
testing.
Any thoughts?
-------------
PR Comment: https://git.openjdk.org/jdk/pull/24657#issuecomment-2827164702
