gaborkaszab commented on code in PR #11837:
URL: https://github.com/apache/iceberg/pull/11837#discussion_r1993039477
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -1621,6 +1627,82 @@ public void testRetainFilesOnRetainedBranches() {
assertThat(deletedFiles).isEqualTo(expectedDeletes);
}
+ @TestTemplate
+ public void testRemoveFromTableWithBulkIO() {
+ TestBulkLocalFileIO spyFileIO = Mockito.spy(new TestBulkLocalFileIO());
Review Comment:
I think it's cleaner to use a Mockito spy on these functions and perform the
verification steps on the spy. The custom FileIO was needed to have a test
FileIO that derives from SupportsBulkOperations and has a deleteFiles()
function. For sure we could also introduce a Set in our FileIO implementation
that collects the paths received as param, also we could introduce a counter
that counts how many time the deleteFiles() function was called, etc., and also
we could separate the received paths by call of this function, but that is
unnecessarily written code as Mockito already gives this for us.
See L1700-1703: we verify that deleteFiles() was called 3 times, and we
could verify the given paths broken down by each call of the function. With
this we could see that even in a scenario where we get an exception during
deletion, we still call the deleteFiles() function for all the other file types
too.
I prefer the current implementation compared to write custom code for
verification, but I might miss something here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
