On Fri, 16 May 2025 06:17:32 GMT, Alan Bateman <[email protected]> wrote:
>> Brian Burkhalter 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 five additional
>> commits since the last revision:
>>
>> - 8355954: Simplify test
>> - Merge
>> - 8355954: Fix HashedPasswordFileTest failure due to obsolete read-only
>> attribute being set
>> - 8355954: Address comments on naming in the PR
>> - 8355954: File.delete removes read-only files (win)
>
> test/jdk/java/io/File/DeleteReadOnly.java line 47:
>
>> 45:
>> 46: private static final File DIR = new File(".", "dir");
>> 47: private static final File FILE = new File(DIR, "file");
>
> If one of the tests fails then it will impact the test(s) that execute later.
> It might be better to just isolate them. deleteReadOnlyDirectory creates a
> directory that is used solely for that test, and deleteReadOnlyRegularFile
> creates a file that is used solely for that test.
Agreed. That actually happened when negative testing against code without the
fix.
> test/jdk/java/io/File/DeleteReadOnly.java line 62:
>
>> 60:
>> 61: boolean deleted = FILE.delete();
>> 62: boolean shouldBeDeleted = !Platform.isWindows() ||
>> DELETE_READ_ONLY;
>
> The behavior and system property is Windows specific and might make things
> simpler to make it a Windows only test.
I guess collapsing the tests made them more ambiguous. Will fix.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24977#discussion_r2092413253
PR Review Comment: https://git.openjdk.org/jdk/pull/24977#discussion_r2092412505
