RFR: 8355475: UNCTest should use an existing UNC path

[Mikhail Yankelevich]

On Thu, 24 Apr 2025 08:12:37 GMT, Eirik Bjørsnøs <eirbjo at openjdk.org> wrote:

> Please review this test-only PR which updates `UNCTest.java` to use a UNC path which is known to exist.
> 
> The test currently uses the file URL `file://jdk/LOCAL-JAVA/jdk1.4/win/README.txt`, but since this is unlikely to resolve to an existing UNC path on any CI server, the test doesn't really verify what it intended to.
> 
> This PR changes the test to instead use the path `file://computername/C$/Windows`, which should always be available unless Administrative Shares has been disabled. We detect this case by using File::exists for the path and simply skip the test if it does not exist.
> 
> I forced this test to run in tier1 on GHA and verified that it ran successfully, without being skipped. Meaning Administrative Shares is enabled in the Windows GHA instance.
> 
> A test run in Oracle's CI verifying a successful run (without skipping) would be welcome.
> 
> This is a test only PR, `noreg-self` has been added in the JBS.

test/jdk/java/net/URLConnection/UNCTest.java line 31:

> 29:  * @library /test/lib
> 30:  * @summary  Check that URL.openConnection() doesn't open connection to UNC
> 31:  * @run main UNCTest

Nitpick: Is this doing anything now?

test/jdk/java/net/URLConnection/UNCTest.java line 49:

> 47:         skipIfAdministrativeSharesDisabled(hostName);
> 48:         // Should always exist with Administrative Shares enabled
> 49:         URL url = new URL("file://" + hostName +"/C$/Windows");

AFAIK new URL is deprecated, do you think something like `new URI("file://" + hostName + "/C$/Windows").toURL();` might work better?

test/jdk/java/net/URLConnection/UNCTest.java line 65:

> 63:      * @param hostName the name of the local computer
> 64:      */
> 65:     private static void skipIfAdministrativeSharesDisabled(String hostName) {

Minor: Is this necessary in the separate method? Seems to me to be a bit too much to move a single if statement. Do you think changing the main method to this might be a bit easier to read? 


...
        // Get the "computer name" for this host
        String hostName = InetAddress.getLocalHost().getHostName();

        // Skip the test if Administrative Shares is disabled
        if (! new File("\\\" + hostName + "\\C$\\Windows").exists()) {
            throw new SkippedException("Administrative shares not enabled");
        }

        // Should always exist with Administrative Shares enabled
....


It is fine as it is too if you prefer it this way 😃

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24842#discussion_r2058790725
PR Review Comment: https://git.openjdk.org/jdk/pull/24842#discussion_r2058793265
PR Review Comment: https://git.openjdk.org/jdk/pull/24842#discussion_r2058805825