test: framework for testing rpc #454

dmitry-fa · 2020-08-05T10:36:17Z

The suggested approach allows us to add new Storage RPC calls covered by unit tests.
The existing RPC calls could be also covered by unit tests.

Fixes #450

codecov · 2020-08-05T10:44:20Z

Codecov Report

Merging #454 (73b6f6e) into master (0ce82cc) will increase coverage by 2.00%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master     #454      +/-   ##
============================================
+ Coverage     63.12%   65.12%   +2.00%     
- Complexity      610      618       +8     
============================================
  Files            32       32              
  Lines          5133     5144      +11     
  Branches        490      492       +2     
============================================
+ Hits           3240     3350     +110     
+ Misses         1729     1630      -99     
  Partials        164      164

Impacted Files	Coverage Δ	Complexity Δ
...src/main/java/com/google/cloud/storage/Bucket.java	`82.20% <0.00%> (+0.12%)`	`34.00% <0.00%> (ø%)`
...main/java/com/google/cloud/storage/BucketInfo.java	`82.01% <0.00%> (+0.27%)`	`86.00% <0.00%> (+3.00%)`
...om/google/cloud/storage/spi/v1/HttpStorageRpc.java	`3.62% <0.00%> (+2.02%)`	`5.00% <0.00%> (+3.00%)`
...gle/cloud/storage/testing/RemoteStorageHelper.java	`64.51% <0.00%> (+2.41%)`	`9.00% <0.00%> (ø%)`
...va/com/google/cloud/storage/spi/v1/StorageRpc.java	`68.83% <0.00%> (+2.59%)`	`0.00% <0.00%> (ø%)`
...ogle/cloud/storage/spi/v1/HttpStorageRpcSpans.java	`100.00% <0.00%> (+100.00%)`	`2.00% <0.00%> (+2.00%)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0ce82cc...73b6f6e. Read the comment docs.

elharo · 2020-08-05T14:14:24Z

google-cloud-storage/src/test/java/com/google/cloud/storage/spi/v1/HttpStorageRpcTest.java

+
+  // Objects for a test case: one object per one RPC mock.
+  // A test creates as many MockData objects as many RPC call it issues.
+  private static final Queue<MockData> TEST_MOCK_DATA = new LinkedList<>();


ArrayDeque instead of LinkedList

elharo · 2020-08-05T14:17:44Z

google-cloud-storage/src/test/java/com/google/cloud/storage/spi/v1/HttpStorageRpcTest.java

+      request.getStreamingContent().writeTo(outputStream);
+      GZIPInputStream is =
+          new GZIPInputStream(new ByteArrayInputStream(outputStream.toByteArray()));
+      byte[] buffer = new byte[1000000];


This isn't safe or correct. Use ByteStreams.toByteArray(is)

Yes, agree. Forgot to fix this before PR. Done.

elharo · 2020-08-05T15:42:02Z

google-cloud-storage/src/test/java/com/google/cloud/storage/spi/v1/HttpStorageRpcTest.java

+  private static final StorageRpcFactory RPC_FACTORY_MOCK =
+      mock(StorageRpcFactory.class, UNEXPECTED_CALL_ANSWER);
+
+  private static class TestRequest extends LowLevelHttpRequest {


make this an outer class for clarity

elharo · 2020-08-05T15:42:21Z

google-cloud-storage/src/test/java/com/google/cloud/storage/spi/v1/HttpStorageRpcTest.java

+    }
+  };
+
+  private static class TestResponse extends LowLevelHttpResponse {


make this an outer class for clarity

elharo · 2020-08-05T15:43:31Z

google-cloud-storage/src/test/java/com/google/cloud/storage/spi/v1/HttpStorageRpcTest.java

+
+  // Objects for a test case: one object per one RPC mock.
+  // A test creates as many MockData objects as many RPC call it issues.
+  private static final Queue<MockData> TEST_MOCK_DATA = new ArrayDeque<>();


Why is this static? That's likely to cause coupling between tests. This can probably be an instance field.

elharo · 2020-08-05T15:43:47Z

google-cloud-storage/src/test/java/com/google/cloud/storage/spi/v1/HttpStorageRpcTest.java

+  private HttpStorageRpc rpc;
+
+  // Objects for a test case: one object per one RPC mock.
+  // A test creates as many MockData objects as many RPC call it issues.


as it issues RPC calls

elharo · 2020-08-05T15:44:15Z

google-cloud-storage/src/test/java/com/google/cloud/storage/spi/v1/HttpStorageRpcTest.java

+    }
+  }
+
+  static class MockData {


outer class

elharo · 2020-08-05T15:44:58Z

google-cloud-storage/src/test/java/com/google/cloud/storage/spi/v1/HttpStorageRpcTest.java

+  @Before
+  public void setUp() throws Exception {
+    rpc = new HttpStorageRpc(STORAGE_OPTIONS);
+    TEST_MOCK_DATA.clear();


yes, this really should be an instance variable so you don't have to worry about clearing it

refactored to be an instance variable

elharo · 2020-08-05T16:45:57Z

google-cloud-storage/src/test/java/com/google/cloud/storage/spi/v1/HttpStorageRpcTest.java

@@ -51,13 +51,122 @@
 import org.mockito.invocation.InvocationOnMock;
 import org.mockito.stubbing.Answer;

+class TestRequest extends LowLevelHttpRequest {


I meant these should be in new files.

Are you sure? These classes unlikely will be used by anyone else but HttpStorageRpcTest.
I can move them to separate files if you insist, but IMHO it will only make understanding harder.
If it is just a matter of taste, I would prefer to see them as inner classes.

Not sure if there's an explicit style rule but Google almost never uses non-public outer classes in the same .java file.

In general inner classes encourage violation of data encapsulation and the single responsibility principle.

And perhaps most importantly here, the sheer size of a file becomes a problem. I found this harder to read as one large class than as several smaller classes.

Thanks for the explanation. I'll move the classes with code to new files and, if you don't mind, keep the trivial classes (MockData and RpcRequestHolder) as inner.

All inner classes moved to new files

elharo · 2020-08-05T19:27:21Z

google-cloud-storage/src/test/java/com/google/cloud/storage/spi/v1/HttpStorageRpcTest.java

+          new GZIPInputStream(new ByteArrayInputStream(outputStream.toByteArray()));
+      return new String(ByteStreams.toByteArray(gzipInputStream), UTF_8);
+    } catch (IOException e) {
+      throw new Error(e);


RuntimeException, or better yet just bubble the IOException

I selected Error because An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch. Most such errors are abnormal conditions.
IOException thrown from this try-catch block will signal about problems in the core Java libraries, not in the Google libraries under test.

I replaced Error with AssertionError which JUnit.fail() throws. I can't replace with fail() because the method returns a value.

elharo · 2020-08-06T23:17:05Z

google-cloud-storage/src/test/java/com/google/cloud/storage/spi/v1/HttpStorageRpcTest.java

+          new GZIPInputStream(new ByteArrayInputStream(outputStream.toByteArray()));
+      return new String(ByteStreams.toByteArray(gzipInputStream), UTF_8);
+    } catch (IOException e) {
+      throw new AssertionError("Unexpected exception", e);


just bubble the IOException, no need to catch it at all

elharo · 2020-08-24T16:32:33Z

google-cloud-storage/src/test/java/com/google/cloud/storage/spi/v1/HttpStorageRpcTest.java

+    ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
+    request.getStreamingContent().writeTo(outputStream);
+    GZIPInputStream gzipInputStream =
+        new GZIPInputStream(new ByteArrayInputStream(outputStream.toByteArray()));


since you already have a byte array use an inflater, not a stream

Would you mind if I kept using GZIPInputStream? java.util.zip.Inflater introduces new DataFormatException the test should handle. The inflater requires a byte buffer of a certain size, but the size is unknown. And I think, I can't use the java.util.zip.Inflater from the box to decompress GZIP content:

To fully decode the gzip format with this class, you will need to process the gzip header as described in RFC 1952, use inflater to decompress the raw deflate data, compute the crc32 of the uncompressed data using that class, and then verify the crc and length (modulo 2^32) in the gzip trailer, again as specified in the RFC.

Or do you mean another inflater?

Hmm, it's weird to copy this into a byte array first. You should be able to go straight from the LowLevelHttpRequest to the decompressor, but LowLevelHttpRequest doesn't offer an input stream to read from, which is really strange. Maybe use a piped stream but that would need an extra thread.

elharo · 2020-08-24T16:33:07Z

google-cloud-storage/src/test/java/com/google/cloud/storage/spi/v1/HttpStorageRpcTest.java

+
+  // Objects for a test case: one object per one RPC mock.
+  // A test creates as many MockData objects as it issues RPC calls.
+  private Queue<MockData> testMockData;


can initialize the queue here

are you sure it's necessary? This queue is re-initialized before each test case.

not necessary, just a little cleaner. When possible, it's good habit to initialize and declare in the same line to avoid unexpected failure initialize or reuse of variables

elharo

LGTM

elharo · 2020-08-24T19:51:06Z

google-cloud-storage/src/test/java/com/google/cloud/storage/spi/v1/HttpStorageRpcTest.java

+
+  // Objects for a test case: one object per one RPC mock.
+  // A test creates as many MockData objects as it issues RPC calls.
+  private Queue<MockData> testMockData;


not necessary, just a little cleaner. When possible, it's good habit to initialize and declare in the same line to avoid unexpected failure initialize or reuse of variables

elharo · 2020-08-24T19:52:31Z

google-cloud-storage/src/test/java/com/google/cloud/storage/spi/v1/HttpStorageRpcTest.java

+    ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
+    request.getStreamingContent().writeTo(outputStream);
+    GZIPInputStream gzipInputStream =
+        new GZIPInputStream(new ByteArrayInputStream(outputStream.toByteArray()));


Hmm, it's weird to copy this into a byte array first. You should be able to go straight from the LowLevelHttpRequest to the decompressor, but LowLevelHttpRequest doesn't offer an input stream to read from, which is really strange. Maybe use a piped stream but that would need an extra thread.

frankyn · 2021-06-11T21:29:19Z

Closing for now, will consider in the future at a later time.

dmitry-fa added 2 commits August 4, 2020 18:04

test framework for rpc

f8f165a

test framework for rpc

5fbedf7

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 5, 2020

dmitry-fa requested review from elharo and frankyn August 5, 2020 10:47

elharo suggested changes Aug 5, 2020

View reviewed changes

test framework for rpc

d691fd8

elharo reviewed Aug 5, 2020

View reviewed changes

inner classes made outer

2206957

elharo reviewed Aug 5, 2020

View reviewed changes

dmitry-fa added 2 commits August 5, 2020 20:48

instance field instead of static

40ee59e

move inner classes to new files

b847bb7

elharo reviewed Aug 5, 2020

View reviewed changes

change Error with AssertionError

9715972

elharo reviewed Aug 6, 2020

View reviewed changes

bubble IOException

73b6f6e

dmitry-fa mentioned this pull request Aug 11, 2020

feat: Add abort() to writer #460

Closed

product-auto-label bot added the api: storage Issues related to the googleapis/java-storage API. label Aug 21, 2020

dmitry-fa requested a review from elharo August 24, 2020 16:16

elharo reviewed Aug 24, 2020

View reviewed changes

dmitry-fa requested a review from elharo August 24, 2020 20:14

dmitry-fa added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 25, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 25, 2020

frankyn closed this Jun 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: framework for testing rpc #454

test: framework for testing rpc #454

dmitry-fa commented Aug 5, 2020

codecov bot commented Aug 5, 2020 •

edited

Loading

elharo Aug 5, 2020

dmitry-fa Aug 5, 2020

elharo Aug 5, 2020

dmitry-fa Aug 5, 2020

elharo Aug 5, 2020

elharo Aug 5, 2020

elharo Aug 5, 2020

elharo Aug 5, 2020

elharo Aug 5, 2020

elharo Aug 5, 2020

dmitry-fa Aug 5, 2020

elharo Aug 5, 2020

dmitry-fa Aug 5, 2020

elharo Aug 5, 2020

dmitry-fa Aug 5, 2020

dmitry-fa Aug 5, 2020

elharo Aug 5, 2020

dmitry-fa Aug 5, 2020

dmitry-fa Aug 5, 2020

elharo Aug 6, 2020

elharo Aug 24, 2020

dmitry-fa Aug 24, 2020

elharo Aug 24, 2020 •

edited

Loading

elharo Aug 24, 2020

dmitry-fa Aug 24, 2020

elharo Aug 24, 2020

elharo left a comment

elharo Aug 24, 2020

elharo Aug 24, 2020 •

edited

Loading

frankyn commented Jun 11, 2021

test: framework for testing rpc #454

test: framework for testing rpc #454

Conversation

dmitry-fa commented Aug 5, 2020

codecov bot commented Aug 5, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elharo Aug 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elharo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elharo Aug 24, 2020 • edited Loading

Choose a reason for hiding this comment

frankyn commented Jun 11, 2021

codecov bot commented Aug 5, 2020 •

edited

Loading

elharo Aug 24, 2020 •

edited

Loading

elharo Aug 24, 2020 •

edited

Loading