On Thu, 24 Apr 2025 16:12:17 GMT, Mikhail Yankelevich
<[email protected]> wrote:
>> Eirik Bjørsnøs has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains three additional
>> commits since the last revision:
>>
>> - Cleanups suggested in review
>> - Merge branch 'master' into unc-test-existing-path
>> - UNCTest should use an existing UNC path
>
> 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 😃
Thanks for your suggestions @myankelev. I think I've adopted all of them, so
rather than commenting on each, can you take another look at the last state and
see if I maybe missed something?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24842#discussion_r2058938472
