From 5fc011cb7de4f953e2298e4b3eb0505a925e5bd7 Mon Sep 17 00:00:00 2001 From: Abid Mohammed Date: Wed, 20 Jul 2022 12:05:36 -0700 Subject: [PATCH 1/5] Test should verify even if there are no exceptions --- .../org/apache/iceberg/aws/s3/TestS3OutputStream.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/aws/src/test/java/org/apache/iceberg/aws/s3/TestS3OutputStream.java b/aws/src/test/java/org/apache/iceberg/aws/s3/TestS3OutputStream.java index c9b6043ccd61..a53ed60266d8 100644 --- a/aws/src/test/java/org/apache/iceberg/aws/s3/TestS3OutputStream.java +++ b/aws/src/test/java/org/apache/iceberg/aws/s3/TestS3OutputStream.java @@ -119,22 +119,26 @@ public void testWrite() { @Test public void testAbortAfterFailedPartUpload() { - doThrow(new RuntimeException()).when(s3mock).uploadPart((UploadPartRequest) any(), (RequestBody) any()); + doThrow(new RuntimeException("mock uploadPart failure")).when(s3mock).uploadPart((UploadPartRequest) any(), (RequestBody) any()); try (S3OutputStream stream = new S3OutputStream(s3mock, randomURI(), properties, nullMetrics())) { stream.write(randomData(10 * 1024 * 1024)); } catch (Exception e) { + // ignore failure for testing + } finally { verify(s3mock, atLeastOnce()).abortMultipartUpload((AbortMultipartUploadRequest) any()); } } @Test public void testAbortMultipart() { - doThrow(new RuntimeException()).when(s3mock).completeMultipartUpload((CompleteMultipartUploadRequest) any()); + doThrow(new RuntimeException("mock completeMultipartUpload failure")).when(s3mock).completeMultipartUpload((CompleteMultipartUploadRequest) any()); try (S3OutputStream stream = new S3OutputStream(s3mock, randomURI(), properties, nullMetrics())) { stream.write(randomData(10 * 1024 * 1024)); } catch (Exception e) { + // ignore failure for testing + } finally { verify(s3mock).abortMultipartUpload((AbortMultipartUploadRequest) any()); } } From 93a3981ee32aacc20cdab33bda26d2bf6463338d Mon Sep 17 00:00:00 2001 From: Abid Mohammed Date: Wed, 20 Jul 2022 12:57:58 -0700 Subject: [PATCH 2/5] refactor as suggested in PR comments --- .../iceberg/aws/s3/TestS3OutputStream.java | 37 ++++++++++--------- 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/aws/src/test/java/org/apache/iceberg/aws/s3/TestS3OutputStream.java b/aws/src/test/java/org/apache/iceberg/aws/s3/TestS3OutputStream.java index a53ed60266d8..5ad851270dd5 100644 --- a/aws/src/test/java/org/apache/iceberg/aws/s3/TestS3OutputStream.java +++ b/aws/src/test/java/org/apache/iceberg/aws/s3/TestS3OutputStream.java @@ -40,6 +40,7 @@ import org.junit.Before; import org.junit.ClassRule; import org.junit.Test; +import org.junit.jupiter.api.Assertions; import org.junit.runner.RunWith; import org.mockito.ArgumentCaptor; import org.mockito.junit.MockitoJUnitRunner; @@ -119,28 +120,28 @@ public void testWrite() { @Test public void testAbortAfterFailedPartUpload() { - doThrow(new RuntimeException("mock uploadPart failure")).when(s3mock).uploadPart((UploadPartRequest) any(), (RequestBody) any()); - - try (S3OutputStream stream = new S3OutputStream(s3mock, randomURI(), properties, nullMetrics())) { - stream.write(randomData(10 * 1024 * 1024)); - } catch (Exception e) { - // ignore failure for testing - } finally { - verify(s3mock, atLeastOnce()).abortMultipartUpload((AbortMultipartUploadRequest) any()); - } + doThrow(new RuntimeException("mock uploadPart failure")) + .when(s3mock).uploadPart((UploadPartRequest) any(), (RequestBody) any()); + + Assertions.assertThrows(RuntimeException.class, ()->{ + try (S3OutputStream stream = new S3OutputStream(s3mock, randomURI(), properties, nullMetrics())) { + stream.write(randomData(10 * 1024 * 1024)); + } + }); + verify(s3mock, atLeastOnce()).abortMultipartUpload((AbortMultipartUploadRequest) any()); } @Test public void testAbortMultipart() { - doThrow(new RuntimeException("mock completeMultipartUpload failure")).when(s3mock).completeMultipartUpload((CompleteMultipartUploadRequest) any()); - - try (S3OutputStream stream = new S3OutputStream(s3mock, randomURI(), properties, nullMetrics())) { - stream.write(randomData(10 * 1024 * 1024)); - } catch (Exception e) { - // ignore failure for testing - } finally { - verify(s3mock).abortMultipartUpload((AbortMultipartUploadRequest) any()); - } + doThrow(new RuntimeException("mock completeMultipartUpload failure")) + .when(s3mock).completeMultipartUpload((CompleteMultipartUploadRequest) any()); + + Assertions.assertThrows(RuntimeException.class, ()->{ + try (S3OutputStream stream = new S3OutputStream(s3mock, randomURI(), properties, nullMetrics())) { + stream.write(randomData(10 * 1024 * 1024)); + } + }); + verify(s3mock).abortMultipartUpload((AbortMultipartUploadRequest) any()); } @Test From bfb82abd3da795987081e982e4eeb8114a7f2d26 Mon Sep 17 00:00:00 2001 From: Abid Mohammed Date: Wed, 20 Jul 2022 13:13:08 -0700 Subject: [PATCH 3/5] refactor as suggested in PR comments --- .../iceberg/aws/s3/TestS3OutputStream.java | 37 +++++++++++-------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/aws/src/test/java/org/apache/iceberg/aws/s3/TestS3OutputStream.java b/aws/src/test/java/org/apache/iceberg/aws/s3/TestS3OutputStream.java index 5ad851270dd5..0f9386bd2535 100644 --- a/aws/src/test/java/org/apache/iceberg/aws/s3/TestS3OutputStream.java +++ b/aws/src/test/java/org/apache/iceberg/aws/s3/TestS3OutputStream.java @@ -40,7 +40,7 @@ import org.junit.Before; import org.junit.ClassRule; import org.junit.Test; -import org.junit.jupiter.api.Assertions; +import org.assertj.core.api.Assertions; import org.junit.runner.RunWith; import org.mockito.ArgumentCaptor; import org.mockito.junit.MockitoJUnitRunner; @@ -120,27 +120,34 @@ public void testWrite() { @Test public void testAbortAfterFailedPartUpload() { - doThrow(new RuntimeException("mock uploadPart failure")) - .when(s3mock).uploadPart((UploadPartRequest) any(), (RequestBody) any()); + RuntimeException mockException = new RuntimeException("mock uploadPart failure"); + doThrow(mockException).when(s3mock).uploadPart((UploadPartRequest) any(), (RequestBody) any()); + + Assertions.assertThatThrownBy(() -> { + try (S3OutputStream stream = new S3OutputStream(s3mock, randomURI(), properties, nullMetrics())) { + stream.write(randomData(10 * 1024 * 1024)); + } + }) + .isInstanceOf(mockException.getClass()) + .hasMessageContaining(mockException.getMessage()); - Assertions.assertThrows(RuntimeException.class, ()->{ - try (S3OutputStream stream = new S3OutputStream(s3mock, randomURI(), properties, nullMetrics())) { - stream.write(randomData(10 * 1024 * 1024)); - } - }); verify(s3mock, atLeastOnce()).abortMultipartUpload((AbortMultipartUploadRequest) any()); } @Test public void testAbortMultipart() { - doThrow(new RuntimeException("mock completeMultipartUpload failure")) - .when(s3mock).completeMultipartUpload((CompleteMultipartUploadRequest) any()); + RuntimeException mockException = new RuntimeException("mock completeMultipartUpload failure"); + doThrow(mockException).when(s3mock) + .completeMultipartUpload((CompleteMultipartUploadRequest) any()); + + Assertions.assertThatThrownBy(() -> { + try (S3OutputStream stream = new S3OutputStream(s3mock, randomURI(), properties, nullMetrics())) { + stream.write(randomData(10 * 1024 * 1024)); + } + }) + .isInstanceOf(mockException.getClass()) + .hasMessageContaining(mockException.getMessage()); - Assertions.assertThrows(RuntimeException.class, ()->{ - try (S3OutputStream stream = new S3OutputStream(s3mock, randomURI(), properties, nullMetrics())) { - stream.write(randomData(10 * 1024 * 1024)); - } - }); verify(s3mock).abortMultipartUpload((AbortMultipartUploadRequest) any()); } From 547cd56258dbe0be69577b1dcdfd71d34f09aad0 Mon Sep 17 00:00:00 2001 From: Abid Mohammed Date: Wed, 20 Jul 2022 16:18:56 -0700 Subject: [PATCH 4/5] fix formatting --- .../iceberg/aws/s3/TestS3OutputStream.java | 50 ++++++++++--------- 1 file changed, 27 insertions(+), 23 deletions(-) diff --git a/aws/src/test/java/org/apache/iceberg/aws/s3/TestS3OutputStream.java b/aws/src/test/java/org/apache/iceberg/aws/s3/TestS3OutputStream.java index 0f9386bd2535..def2e0334b00 100644 --- a/aws/src/test/java/org/apache/iceberg/aws/s3/TestS3OutputStream.java +++ b/aws/src/test/java/org/apache/iceberg/aws/s3/TestS3OutputStream.java @@ -36,11 +36,11 @@ import java.util.stream.Stream; import org.apache.iceberg.aws.AwsProperties; import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; +import org.assertj.core.api.Assertions; import org.junit.After; import org.junit.Before; import org.junit.ClassRule; import org.junit.Test; -import org.assertj.core.api.Assertions; import org.junit.runner.RunWith; import org.mockito.ArgumentCaptor; import org.mockito.junit.MockitoJUnitRunner; @@ -73,7 +73,6 @@ import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; - @RunWith(MockitoJUnitRunner.class) public class TestS3OutputStream { private static final Logger LOG = LoggerFactory.getLogger(TestS3OutputStream.class); @@ -124,12 +123,12 @@ public void testAbortAfterFailedPartUpload() { doThrow(mockException).when(s3mock).uploadPart((UploadPartRequest) any(), (RequestBody) any()); Assertions.assertThatThrownBy(() -> { - try (S3OutputStream stream = new S3OutputStream(s3mock, randomURI(), properties, nullMetrics())) { - stream.write(randomData(10 * 1024 * 1024)); - } - }) - .isInstanceOf(mockException.getClass()) - .hasMessageContaining(mockException.getMessage()); + try (S3OutputStream stream = new S3OutputStream(s3mock, randomURI(), properties, nullMetrics())) { + stream.write(randomData(10 * 1024 * 1024)); + } + }) + .isInstanceOf(mockException.getClass()) + .hasMessageContaining(mockException.getMessage()); verify(s3mock, atLeastOnce()).abortMultipartUpload((AbortMultipartUploadRequest) any()); } @@ -138,15 +137,15 @@ public void testAbortAfterFailedPartUpload() { public void testAbortMultipart() { RuntimeException mockException = new RuntimeException("mock completeMultipartUpload failure"); doThrow(mockException).when(s3mock) - .completeMultipartUpload((CompleteMultipartUploadRequest) any()); + .completeMultipartUpload((CompleteMultipartUploadRequest) any()); Assertions.assertThatThrownBy(() -> { - try (S3OutputStream stream = new S3OutputStream(s3mock, randomURI(), properties, nullMetrics())) { - stream.write(randomData(10 * 1024 * 1024)); - } - }) - .isInstanceOf(mockException.getClass()) - .hasMessageContaining(mockException.getMessage()); + try (S3OutputStream stream = new S3OutputStream(s3mock, randomURI(), properties, nullMetrics())) { + stream.write(randomData(10 * 1024 * 1024)); + } + }) + .isInstanceOf(mockException.getClass()) + .hasMessageContaining(mockException.getMessage()); verify(s3mock).abortMultipartUpload((AbortMultipartUploadRequest) any()); } @@ -180,7 +179,8 @@ private void writeTest() { writeAndVerify(s3mock, randomURI(), data, arrayWrite); ArgumentCaptor putObjectRequestArgumentCaptor = ArgumentCaptor.forClass(PutObjectRequest.class); - verify(s3mock, times(1)).putObject(putObjectRequestArgumentCaptor.capture(), + verify(s3mock, times(1)).putObject( + putObjectRequestArgumentCaptor.capture(), (RequestBody) any()); checkPutObjectRequestContent(data, putObjectRequestArgumentCaptor); checkTags(putObjectRequestArgumentCaptor); @@ -190,7 +190,8 @@ private void writeTest() { data = randomData(6 * 1024 * 1024); writeAndVerify(s3mock, randomURI(), data, arrayWrite); putObjectRequestArgumentCaptor = ArgumentCaptor.forClass(PutObjectRequest.class); - verify(s3mock, times(1)).putObject(putObjectRequestArgumentCaptor.capture(), + verify(s3mock, times(1)).putObject( + putObjectRequestArgumentCaptor.capture(), (RequestBody) any()); checkPutObjectRequestContent(data, putObjectRequestArgumentCaptor); checkTags(putObjectRequestArgumentCaptor); @@ -201,7 +202,8 @@ private void writeTest() { writeAndVerify(s3mock, randomURI(), data, arrayWrite); ArgumentCaptor uploadPartRequestArgumentCaptor = ArgumentCaptor.forClass(UploadPartRequest.class); - verify(s3mock, times(2)).uploadPart(uploadPartRequestArgumentCaptor.capture(), + verify(s3mock, times(2)).uploadPart( + uploadPartRequestArgumentCaptor.capture(), (RequestBody) any()); checkUploadPartRequestContent(data, uploadPartRequestArgumentCaptor); reset(s3mock); @@ -211,7 +213,8 @@ private void writeTest() { writeAndVerify(s3mock, randomURI(), data, arrayWrite); uploadPartRequestArgumentCaptor = ArgumentCaptor.forClass(UploadPartRequest.class); - verify(s3mock, times(5)).uploadPart(uploadPartRequestArgumentCaptor.capture(), + verify(s3mock, times(5)).uploadPart( + uploadPartRequestArgumentCaptor.capture(), (RequestBody) any()); checkUploadPartRequestContent(data, uploadPartRequestArgumentCaptor); reset(s3mock); @@ -268,7 +271,7 @@ private String getDigest(byte[] data, int offset, int length) { return null; } - private void writeAndVerify(S3Client client, S3URI uri, byte [] data, boolean arrayWrite) { + private void writeAndVerify(S3Client client, S3URI uri, byte[] data, boolean arrayWrite) { try (S3OutputStream stream = new S3OutputStream(client, uri, properties, nullMetrics())) { if (arrayWrite) { stream.write(data); @@ -296,14 +299,15 @@ private void writeAndVerify(S3Client client, S3URI uri, byte [] data, boolean ar private byte[] readS3Data(S3URI uri) { ResponseBytes data = - s3.getObject(GetObjectRequest.builder().bucket(uri.bucket()).key(uri.key()).build(), - ResponseTransformer.toBytes()); + s3.getObject( + GetObjectRequest.builder().bucket(uri.bucket()).key(uri.key()).build(), + ResponseTransformer.toBytes()); return data.asByteArray(); } private byte[] randomData(int size) { - byte [] result = new byte[size]; + byte[] result = new byte[size]; random.nextBytes(result); return result; } From e34cf82d2fa3b634058ddd268b85acbf7ff6eb32 Mon Sep 17 00:00:00 2001 From: Abid Mohammed Date: Wed, 20 Jul 2022 17:37:03 -0700 Subject: [PATCH 5/5] rever unnecessary formatting --- .../iceberg/aws/s3/TestS3OutputStream.java | 22 ++++++++----------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/aws/src/test/java/org/apache/iceberg/aws/s3/TestS3OutputStream.java b/aws/src/test/java/org/apache/iceberg/aws/s3/TestS3OutputStream.java index def2e0334b00..d4ce92b2a7da 100644 --- a/aws/src/test/java/org/apache/iceberg/aws/s3/TestS3OutputStream.java +++ b/aws/src/test/java/org/apache/iceberg/aws/s3/TestS3OutputStream.java @@ -73,6 +73,7 @@ import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; + @RunWith(MockitoJUnitRunner.class) public class TestS3OutputStream { private static final Logger LOG = LoggerFactory.getLogger(TestS3OutputStream.class); @@ -179,8 +180,7 @@ private void writeTest() { writeAndVerify(s3mock, randomURI(), data, arrayWrite); ArgumentCaptor putObjectRequestArgumentCaptor = ArgumentCaptor.forClass(PutObjectRequest.class); - verify(s3mock, times(1)).putObject( - putObjectRequestArgumentCaptor.capture(), + verify(s3mock, times(1)).putObject(putObjectRequestArgumentCaptor.capture(), (RequestBody) any()); checkPutObjectRequestContent(data, putObjectRequestArgumentCaptor); checkTags(putObjectRequestArgumentCaptor); @@ -190,8 +190,7 @@ private void writeTest() { data = randomData(6 * 1024 * 1024); writeAndVerify(s3mock, randomURI(), data, arrayWrite); putObjectRequestArgumentCaptor = ArgumentCaptor.forClass(PutObjectRequest.class); - verify(s3mock, times(1)).putObject( - putObjectRequestArgumentCaptor.capture(), + verify(s3mock, times(1)).putObject(putObjectRequestArgumentCaptor.capture(), (RequestBody) any()); checkPutObjectRequestContent(data, putObjectRequestArgumentCaptor); checkTags(putObjectRequestArgumentCaptor); @@ -202,8 +201,7 @@ private void writeTest() { writeAndVerify(s3mock, randomURI(), data, arrayWrite); ArgumentCaptor uploadPartRequestArgumentCaptor = ArgumentCaptor.forClass(UploadPartRequest.class); - verify(s3mock, times(2)).uploadPart( - uploadPartRequestArgumentCaptor.capture(), + verify(s3mock, times(2)).uploadPart(uploadPartRequestArgumentCaptor.capture(), (RequestBody) any()); checkUploadPartRequestContent(data, uploadPartRequestArgumentCaptor); reset(s3mock); @@ -213,8 +211,7 @@ private void writeTest() { writeAndVerify(s3mock, randomURI(), data, arrayWrite); uploadPartRequestArgumentCaptor = ArgumentCaptor.forClass(UploadPartRequest.class); - verify(s3mock, times(5)).uploadPart( - uploadPartRequestArgumentCaptor.capture(), + verify(s3mock, times(5)).uploadPart(uploadPartRequestArgumentCaptor.capture(), (RequestBody) any()); checkUploadPartRequestContent(data, uploadPartRequestArgumentCaptor); reset(s3mock); @@ -271,7 +268,7 @@ private String getDigest(byte[] data, int offset, int length) { return null; } - private void writeAndVerify(S3Client client, S3URI uri, byte[] data, boolean arrayWrite) { + private void writeAndVerify(S3Client client, S3URI uri, byte [] data, boolean arrayWrite) { try (S3OutputStream stream = new S3OutputStream(client, uri, properties, nullMetrics())) { if (arrayWrite) { stream.write(data); @@ -299,15 +296,14 @@ private void writeAndVerify(S3Client client, S3URI uri, byte[] data, boolean arr private byte[] readS3Data(S3URI uri) { ResponseBytes data = - s3.getObject( - GetObjectRequest.builder().bucket(uri.bucket()).key(uri.key()).build(), - ResponseTransformer.toBytes()); + s3.getObject(GetObjectRequest.builder().bucket(uri.bucket()).key(uri.key()).build(), + ResponseTransformer.toBytes()); return data.asByteArray(); } private byte[] randomData(int size) { - byte[] result = new byte[size]; + byte [] result = new byte[size]; random.nextBytes(result); return result; }