amogh-jahagirdar commented on code in PR #13905: URL: https://github.com/apache/iceberg/pull/13905#discussion_r2298183691
########## aws/src/test/java/org/apache/iceberg/aws/s3/TestS3InputStream.java: ########## @@ -19,30 +19,47 @@ package org.apache.iceberg.aws.s3; import static org.mockito.Mockito.any; -import static org.mockito.Mockito.anyInt; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import java.io.IOException; import java.io.InputStream; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; import software.amazon.awssdk.core.sync.ResponseTransformer; import software.amazon.awssdk.services.s3.S3Client; import software.amazon.awssdk.services.s3.model.GetObjectRequest; +@ExtendWith(MockitoExtension.class) Review Comment: I think having the separate unit test for the input stream in addition to the existing integration test is probably a good thing, since we can verify the stream reading logic independent of any behavior in the underlying client, and it'd be nice to run some tests here that don't require any environment setup. At the moment it looks like we just assert the stream is closed but we may want to see if it's possible to build a shared abstraction between the unit and integration test which allows being able to plug in whichever client is desired; in the unit test case, a mocked client is plugged in and in the integration test case, a real client is plugged in. Then if there are any other clients out there someone could easily extend the test and run that. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
