On Wed, 16 Apr 2025 00:42:22 GMT, Brian Burkhalter <b...@openjdk.org> wrote:
>> In `java.io.WinNTFileSystem::isInvalid`, replace an insufficient test for
>> file path validity with a sufficient test for file path invalidity. Also,
>> add a new test.
>
> Brian Burkhalter has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8354450: Add specific conditional for trailing space; adjust test
test/jdk/java/io/File/WinTrailingSpace.java line 28:
> 26:
> 27: import org.junit.jupiter.api.Test;
> 28: import static org.junit.jupiter.api.Assertions.*;
nitpick: wildcard import
test/jdk/java/io/File/WinTrailingSpace.java line 61:
> 59: f.delete();
> 60: f.createNewFile();
> 61: assertFalse(f.exists()); // should not reach here
Do you think this might be easier to debug if there was a message in there
rather than an `assertFalse`? In case of an error this will just say failed.
However in the failed case where the file wasn't actually created and the error
wasn't triggered either it should pass in the current state. This will be more
confusing when debugging, especially in case of an intermittent issue.
My feeling is that for the long run it might be better to have something like
```java
assertFalse("File was created. File exists", f.exists());
assertEquals("No error thrown", "Error Thrown");
What do you think?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24635#discussion_r2046493405
PR Review Comment: https://git.openjdk.org/jdk/pull/24635#discussion_r2046509227
