On Wed, 16 Apr 2025 09:21:41 GMT, Mikhail Yankelevich
<myankelev...@openjdk.org> wrote:
>> 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
I am not fond of wildcard imports either, but it is common practice for static
imports like this. I will change it though.
> 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?
I agree that something like you suggest would be better. I'll take a closer
look.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24635#discussion_r2047397807
PR Review Comment: https://git.openjdk.org/jdk/pull/24635#discussion_r2047399125
