On Thu, 24 Apr 2025 08:12:37 GMT, Eirik Bjørsnøs <[email protected]> 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
