Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: stub implementation of StorageRpc for the sake of testing #351

Merged
merged 3 commits into from Jun 8, 2020

Conversation

dmitry-fa
Copy link
Contributor

Allows extending of the StorageRpc interface without breaking cross library dependencies.
Tests outside the Storage library might extend the StorageRpcTestBase class instead of implementing the StorageRpc interface.

Fixes #242

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 5, 2020
@codecov
Copy link

codecov bot commented Jun 5, 2020

Codecov Report

Merging #351 into master will increase coverage by 0.34%.
The diff coverage is 97.95%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #351      +/-   ##
============================================
+ Coverage     62.73%   63.07%   +0.34%     
- Complexity      554      611      +57     
============================================
  Files            31       32       +1     
  Lines          5023     5072      +49     
  Branches        480      479       -1     
============================================
+ Hits           3151     3199      +48     
- Misses         1707     1708       +1     
  Partials        165      165              
Impacted Files Coverage Δ Complexity Δ
...ogle/cloud/storage/testing/StorageRpcTestBase.java 97.95% <97.95%> (ø) 48.00 <48.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 a3b04dd...1b95a56. Read the comment docs.

import java.util.Map;

/**
* A stub implementation of {@link StorageRpc} which could be used outside of the Storage module for
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could --> can


/**
* A stub implementation of {@link StorageRpc} which could be used outside of the Storage module for
* testing purposes. All the methods are implemented either to return the default value or to throw
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"are implemented either to return the default value or to throw" --> "either return the default value or throw"

* Creates an instance with methods either returning the default value or throwing {@code
* UnsupportedOperationException}, depending on the {@code isImplemented} parameter.
*
* @param isImplemented {@code true} to return a value, {@code false} to throw an exception.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: no period since not a complete sentence

@Override
public StorageObject compose(
Iterable<StorageObject> sources, StorageObject target, Map<Option, ?> targetOptions) {
if (isImplemented) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is "isImplemented" needed? If you're just returning null here and elsewhere and not expecting these methods to be called, unconditionally throwing the exception might be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that was my original intention to throw the exception unconditionally. Then I decided that adding "isImplemented" feature might simplify the work on extending that class.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't seem like returning null helps a lot in this case. Subclassers who invoke a method like this will still need to override this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to agree with you, but I will think more about cases when returning null is useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed as you suggested. If someone needs an implementation returning null, an IDE can help to produce such class.

@dmitry-fa dmitry-fa added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 8, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 8, 2020
Copy link
Member

@frankyn frankyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is much cleaner @dmitry-fa, thanks for driving this forward.

@frankyn frankyn merged commit dd58025 into googleapis:master Jun 8, 2020
@dmitry-fa
Copy link
Contributor Author

@frankyn, to move forward with googleapis/java-storage-nio#124 we need to release the Storage (1.108.1). What should we do to initiate the release process?

gcf-merge-on-green bot pushed a commit that referenced this pull request Jun 11, 2020
🤖 I have created a release \*beep\* \*boop\* 
---
## [1.109.0](https://www.github.com/googleapis/java-storage/compare/v1.108.0...v1.109.0) (2020-06-11)


### Features

* adopt flatten-maven-plugin and java-shared-dependencies ([#325](https://www.github.com/googleapis/java-storage/issues/325)) ([209cae3](https://www.github.com/googleapis/java-storage/commit/209cae322932a4f87729fe4c5176a4f11962cfae))
* stub implementation of StorageRpc for the sake of testing ([#351](https://www.github.com/googleapis/java-storage/issues/351)) ([dd58025](https://www.github.com/googleapis/java-storage/commit/dd5802555eb0351a5afa2f2f197cb93ca6d3b66e))


### Bug Fixes

* blob.reload() does not work as intuitively expected ([#308](https://www.github.com/googleapis/java-storage/issues/308)) ([a2bab58](https://www.github.com/googleapis/java-storage/commit/a2bab58ccd89f48e8d4a8ee2dd776b201598420d))


### Documentation

* **fix:** update client documentation link ([#324](https://www.github.com/googleapis/java-storage/issues/324)) ([eb8940c](https://www.github.com/googleapis/java-storage/commit/eb8940cc6a88b5e2b3dea8d0ab2ffc1e350ab924))
* Add doc for equals method in blob ([#311](https://www.github.com/googleapis/java-storage/issues/311)) ([91fc36a](https://www.github.com/googleapis/java-storage/commit/91fc36a6673e30d1cfa8c4da51b874e1fd0b0535))
* catch actual exception in java doc comment ([#312](https://www.github.com/googleapis/java-storage/issues/312)) ([9201de5](https://www.github.com/googleapis/java-storage/commit/9201de559fe4218abd2e4fac47beac62454547cf)), closes [#309](https://www.github.com/googleapis/java-storage/issues/309)
* update CONTRIBUTING.md to include code formatting ([#534](https://www.github.com/googleapis/java-storage/issues/534)) ([#315](https://www.github.com/googleapis/java-storage/issues/315)) ([466d08f](https://www.github.com/googleapis/java-storage/commit/466d08f9835a0f1dd00b5c9b3a08551be68d03ad))
* update readme to point client libarary documentation ([#317](https://www.github.com/googleapis/java-storage/issues/317)) ([8650f80](https://www.github.com/googleapis/java-storage/commit/8650f806736beec7bf7ab09a337b333bbf144f7b))


### Dependencies

* update dependency com.google.api.grpc:proto-google-common-protos to v1.18.0 ([#301](https://www.github.com/googleapis/java-storage/issues/301)) ([ff2dee2](https://www.github.com/googleapis/java-storage/commit/ff2dee2ce41d37787f0866ae740d3cd7f3b2bbd6))
* update dependency com.google.apis:google-api-services-storage to v1-rev20200410-1.30.9 ([#296](https://www.github.com/googleapis/java-storage/issues/296)) ([2e55aa2](https://www.github.com/googleapis/java-storage/commit/2e55aa2c8b9c78df9eebfe748fe72dcaae63ff81))
* update dependency com.google.apis:google-api-services-storage to v1-rev20200430-1.30.9 ([#319](https://www.github.com/googleapis/java-storage/issues/319)) ([3d03fa3](https://www.github.com/googleapis/java-storage/commit/3d03fa3381cfbb76d1501ec3d2ad14742a8a58dd))
* update dependency com.google.cloud:google-cloud-conformance-tests to v0.0.11 ([#320](https://www.github.com/googleapis/java-storage/issues/320)) ([6c18c88](https://www.github.com/googleapis/java-storage/commit/6c18c882cfe0c35b310a518e6044847e6fbeab94))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please).
@dmitry-fa dmitry-fa deleted the nio-merge2 branch June 26, 2020 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Merge Storage-NIO library into this repository.
5 participants