RFR: 8276994: java/nio/channels/Channels/TransferTo.java leaves multi-GB files in /tmp

[Markus KARG]

On Sun, 14 Nov 2021 11:32:27 GMT, Lance Andersen <lancea at openjdk.org> wrote:

>> This PR proposes to cleanup *all* temp files of the `TransferTo` test, not just the 2GB files. Also it separates the actual test logic from the cleanup logic, so the test cases are simple to understand again as they are not interwoven with cleanup code anymore.
>
> test/jdk/java/nio/channels/Channels/TransferTo.java line 78:
> 
>> 76: 
>> 77:     private static Collection<Path> tempFiles;
>> 78: 
> 
> Is there a reason you chose a LinkedList over a List here?  As mentioned below, you could initialize  tempFiles here.
> 
> Please change Collection -> LinkedList or if you use a List  initialize with an ArrayList()

If you change from an interface to a particular implementation you give the reader the impression that the code will only work with this particular implementation, and you give the impression that the code will only work in a particular sequence when using List or LinkedList, which both is not true. The Collection interface was chosen deliberately to clearly tell the reader that both is completely irrelevant and may be changed at any time if needed. If I do what you want, others will fear to break something. So i would like to stay with the code as-is.

> test/jdk/java/nio/channels/Channels/TransferTo.java line 147:
> 
>> 145:     @Test
>> 146:     public void testMoreThanTwoGB() throws IOException {
>> 147:         // preparing two temporary files which will be compared at the end of the test
> 
> I would prefer to keep the previous changes to this test.  We went through a few iterations off line.  We would also prefer to delete these files at the end of the test, not just the over all test run due to their size.

As all the *other* tests in this test class run rather quick (within seconds) I wonder what you expect to *actually* gain? The difference would just be some seconds of earlier deletion for the sake of worse readability, actually. If the majority is for keeping your original solution, this is fine for me. I just like to understand the driver behind this.

> test/jdk/java/nio/channels/Channels/TransferTo.java line 285:
> 
>> 283:     @BeforeClass
>> 284:     public static void createTempFileRegistry() {
>> 285:         tempFiles = new LinkedList();
> 
> This could be initialized as part of the declaration.

Yes I could do that but I think it is better to understand for the reader as it clearly explains that this code is part of the test management, not part of the test logic.

> test/jdk/java/nio/channels/Channels/TransferTo.java line 303:
> 
>> 301:      * Creates a temporary file and registers it for later cleanup after tests.
>> 302:      */
>> 303:     private static Path newTempFile() throws IOException {
> 
> You might consider passing in the prefix to make it easier to differentiate the temp file (InputStreamProvider vs OutputSteamProvider...)

Is this really worth the extra complexity? What is the use case where this plays a vital role?

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

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