feat: moving a testing class from java-storage-nio to java-storage #345
Conversation
googlebot
added
the
cla: yes
dmitry-fa
changed the title
Codecov Report
@@ Coverage Diff @@
## master #345 +/- ##
============================================
+ Coverage 62.73% 62.83% +0.10%
- Complexity 554 623 +69
============================================
Files 31 32 +1
Lines 5023 5263 +240
Branches 480 521 +41
============================================
+ Hits 3151 3307 +156
- Misses 1707 1768 +61
- Partials 165 188 +23
Continue to review full report at Codecov.
|
| public class FakeStorageRpc implements StorageRpc { | ||
|
|
||
| // fullname -> metadata | ||
| Map<String, StorageObject> metadata = new HashMap<>(); |
There was a problem hiding this comment.
These fields are package-private intentionally for the sake of testability. It allows covering 'the fake functionality' by stronger tests.
| // fullname -> future contents that will be visible on close. | ||
| Map<String, byte[]> futureContents = new HashMap<>(); | ||
|
|
||
| private final boolean throwIfOption; |
There was a problem hiding this comment.
not sure what this field name means
There was a problem hiding this comment.
FakeStorageRpc.java is copied almost as-is from:
https://github.com/googleapis/java-storage-nio/blob/master/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/testing/FakeStorageRpc.java
| } | ||
|
|
||
| // remove all files | ||
| public void reset() { |
There was a problem hiding this comment.
do you need this or could you just create a new object?
There was a problem hiding this comment.
I can't say for sure. This class is used by Storage-Nio tests. reset() doesn't clear all the fields, perhaps it's used for testing of resumable upload. Anyway, to provide smoother migration I think we need to keep it for now.
| throws StorageException { | ||
| // if non-null, then we check the file's at that generation. | ||
| Long generationMatch = null; | ||
| for (Option op : options.keySet()) { |
|
|
||
| @Override | ||
| public Tuple<String, byte[]> read( | ||
| StorageObject from, Map<Option, ?> options, long zposition, int zbytes) |
There was a problem hiding this comment.
zposition? zbytes? what are these?
| } | ||
| checkGeneration(key, generationMatch); | ||
| long position = zposition; | ||
| int bytes = zbytes; |
| StorageObject from, Map<Option, ?> options, long position, OutputStream outputStream) { | ||
| // if non-null, then we check the file's at that generation. | ||
| Long generationMatch = null; | ||
| for (Option op : options.keySet()) { |
| if (metadata.containsKey(uploadId)) { | ||
| StorageObject storageObject = metadata.get(uploadId); | ||
| Long generation = storageObject.getGeneration(); | ||
| if (null == generation) { |
|
@elharo, thanks for looking. Now I see that this implementation is very nio specific, unlikely it will be used by anyone else. So, it's not a good idea to expose such a class. Probably, the right way is to export a class implementing all StorageRpc methods to throw UnsupportedException and make FakeStorageRpc in Storage-Nio to extends it. So, I'm closing this PR |
Moving FakeStorageRpcTest class implementing the StorageRpc interface from java-storage-nio to java-storage. That will allow extending StorageRpc without breaking cross-project dependencies.
It was suggested to introduce a new artifact google-cloud-storage-testing, but I hope it's not necessary, because we already have a testing class included in the main java-storage artifact:
com.google.cloud.storage.testingRemoteStorageHelper. Newly introduced class FakeStorageRpc borrowed from the Storage-Nio is placed next to it.Fixes #242