RFR: JDK-8232092 (fs) Files::isWritable returns false on a writeable root directory (win) [v6]

[Nhat Nguyen]

On Thu, 24 Sep 2020 09:49:33 GMT, Alan Bateman <alanb at openjdk.org> wrote:

>> Nhat Nguyen has updated the pull request incrementally with three additional commits since the last revision:
>> 
>>  - Remove usage of stream
>>  - Remove unused import
>>  - Remove duplicate import
>
> The update to WindowsFileStore looks okay.
> 
> The test has good coverage but I worry that creating X: will not be reliable in some CI environments, esp. if X: exists
> already or permissions are needed. I'll try your test in our CI and would be good if others did the same.
> One terminology nit is that the file system API uses the term "directory". The test uses a mix of "directory" and
> "folder" would be good to keep it consistent if possible.
> I don't think the addition to TestUtil is needed, the createRootTempDirectory method can call Files.createTmpDirectory.
> 
> Many of the existing TestNG tests import statically Asserts and could be done here too.
> 
> substList should return Stream<Srring>, we try to avoid raw types.
> 
> No need to import java.lang.ProcessBuilder, also would be good to clean import the imports, no need for a space between
> each java.* package.

> _Mailing list message from [Alan Bateman](mailto:Alan.Bateman at oracle.com) on
> [nio-dev](mailto:nio-dev at openjdk.java.net):_
> On 24/09/2020 10:52, Alan Bateman wrote:
> 
> > :
> > The test has good coverage but I worry that creating X: will not be reliable in some CI environments, esp. if X: exists
> > already or permissions are needed. I'll try your test in our CI and would be good if others did the same.
> 
> I tried it in our CI and testRealPath fails consistently on all Windows
> clients.? This is JDK-8213216 rather than JDK-8232092. I know Brian was
> looking at JDK-8213216 at one point but we didn't come to a conclusion
> on supporting the scenario without creating inconsistencies. So maybe
> this test should be dropped from SubstDrive.
> 
> -Alan

Thank you Alan for catching this. This is an oversight on my end. I wrote `testRealPath` with the intention that if
Brian ever fixes jdk-8213216, he can use this test; unfortunately I forgot to disable it. I have dropped this test case
from the test altogether to prevent any future confusion.

> One terminology nit is that the file system API uses the term "directory". The test uses a mix of "directory" and
> "folder" would be good to keep it consistent if possible.

Fixed!

> I don't think the addition to TestUtil is needed, the createRootTempDirectory method can call Files.createTmpDirectory.

Fixed!
 
> Many of the existing TestNG tests import statically Asserts and could be done here too.

Fixed!

> No need to import java.lang.ProcessBuilder, also would be good to clean import the imports, no need for a space between
> each java.* package.

Fixed!

> substList should return Stream, we try to avoid raw types.

I'm sorry I'm a bit confused by this. Did you mean to say "should not return stream"? substList returns Stream<String>
and not raw Stream. If the convention is try not to return Stream<> at all, I have converted the method to return
List<> instead (I made substList return Stream in the first place because I wanted to call anyMatch() on the return
result). Please let me know if this is what you expected. ��

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

PR: https://git.openjdk.java.net/jdk/pull/287