test: convert remaining StorageImpl tests from easymock to mockito #667
Conversation
product-auto-label
bot
added
the
api: storage
google-cla
bot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #667 +/- ##
============================================
+ Coverage 64.26% 64.76% +0.50%
Complexity 621 621
============================================
Files 32 32
Lines 5322 5322
Branches 521 521
============================================
+ Hits 3420 3447 +27
+ Misses 1740 1713 -27
Partials 162 162
Continue to review full report at Codecov.
|
| @@ -16,27 +16,38 @@ | |||
|
|
|||
| package com.google.cloud.storage; | |||
|
|
|||
| import static com.google.cloud.storage.SignedUrlEncodingHelper.Rfc3986UriEncode; | |||
There was a problem hiding this comment.
IMHO, this isn't familiar enough to static import.
| assertEquals("https://storage.googleapis.com/my-bucket/", policy.getUrl()); | ||
|
|
||
| // test fields, no conditions | ||
| policy = |
There was a problem hiding this comment.
Reassigning local variables is a code smell that suggests this could be broken up into multiple tests.
There was a problem hiding this comment.
agree, done.
Please take a look.
There was a problem hiding this comment.
@elharo, it would be awesome if you take another look at this change before the New Year
|
@frankyn, If you could find time to look and merge this change before conflicts arise, it would be awesome. |
There was a problem hiding this comment.
https://engdoc.corp.google.com/eng/doc/devguide/java/testing/mocking.md?cl=head#google-style suggests you don't want both doReturn and doThrow on one mock.
| public void testUpdateBucket() { | ||
| BucketInfo updatedBucketInfo = BUCKET_INFO1.toBuilder().setIndexPage("some-page").build(); | ||
| doReturn(updatedBucketInfo.toPb()) | ||
| .doThrow(UNEXPECTED_CALL_EXCEPTION) |
There was a problem hiding this comment.
I don't see why you're throwing this exception here. Is this testing the bucket is still updated when an exception is thrown?
| @Test | ||
| public void testUpdateBucketWithOptions() { | ||
| BucketInfo updatedBucketInfo = BUCKET_INFO1.toBuilder().setIndexPage("some-page").build(); | ||
| doReturn(updatedBucketInfo.toPb()) |
There was a problem hiding this comment.
I think I don't understand using both doReturn and doThrow in the same chain. What's happening here?
There was a problem hiding this comment.
@elharo doThrow(UNEXPECTED_CALL_EXCEPTION) after doReturn() makes unit tests stronger. It guarantees that the method is calling only once(or the expected number of times). If someone updates the code to call the method twice (by mistake) the test will discover that.
| public void testUpdateBlob() { | ||
| BlobInfo updatedBlobInfo = BLOB_INFO1.toBuilder().setContentType("some-content-type").build(); | ||
| doReturn(updatedBlobInfo.toPb()) | ||
| .doThrow(UNEXPECTED_CALL_EXCEPTION) |
There was a problem hiding this comment.
Is this supposed to fail the test if there's a second call? I'm not sure that's something that should be tested here. Mockito tries to be more forgiving than EasyMock.
There was a problem hiding this comment.
doThrow() is not necessary of course. Removing .doThrow will make the tests weaker.
eaball35
added
the
kokoro:force-run
|
Closing this PR out. We will revisit in the future. |
Fixes #270
For the convenience of reviewers this PR comprises two commits, the second commit allows to see the converted tests as if there were changed, not added.