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
test: convert remaining StorageImpl tests from easymock to mockito #667
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, this isn't familiar enough to static import.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
assertEquals("https://storage.googleapis.com/my-bucket/", policy.getUrl()); | ||
|
||
// test fields, no conditions | ||
policy = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reassigning local variables is a code smell that suggests this could be broken up into multiple tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree, done.
Please take a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doThrow() is not necessary of course. Removing .doThrow will make the tests weaker.
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.