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

[Alan Bateman]

On Mon, 5 Oct 2020 18:59:10 GMT, Nhat Nguyen <github.com+8022952+nhat-nguyen at openjdk.org> wrote:

>> 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. ��

The updated test looks much better and passes in our infrastructure since the remove of the toRealPath test. I'm still
bothered by the hardcoded "X:". Does subst (and hence substCreate) fail if that volume already exists?

substList can be changed to return List<String>.

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

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