STORAGE-2798 Updated B2StorageClientWebifierImpl methods to improve extensibility #205
Conversation
jsha-bbz
force-pushed
the
jsha/STORAGE-2798-b2-download-by-name
branch
from
120b749 to
578493d
Compare
| @@ -1201,7 +1204,7 @@ public void testDownloadById() throws B2Exception { | |||
| " X-Bz-Test-Mode: force_cap_exceeded\n" | |||
| ); | |||
|
|
|||
| assertEquals(expectedUrl, webifier.getDownloadByIdUrl(ACCOUNT_AUTH, request)); | |||
| assertEquals(expectedUrl, webifierSpy.getDownloadByIdUrl(ACCOUNT_AUTH, request)); | |||
There was a problem hiding this comment.
why does this need to use the spy'ed object?
There was a problem hiding this comment.
It doesn't need to be. Is it a good practice to only use the spy reference when using Mockito calls and the non spy in cases like this? I was thinking it might be less confusing if using the same spy reference throughout the code instead of mixing. Don't mind either way. I'll change it back
| webifier.downloadById(ACCOUNT_AUTH, request, noopContentHandler); | ||
| final B2StorageClientWebifierImpl webifierSpy = spy(webifier); | ||
| webifierSpy.downloadById(ACCOUNT_AUTH, request, noopContentHandler); | ||
| verify(webifierSpy).getDownloadByIdUrl(ACCOUNT_AUTH, request); |
There was a problem hiding this comment.
hmm... I think this tests that getDownloadByIdUrl is called but my brain is wondering how useful this verify actually is beyond that. isn't that just an implementation detail? how does verifying it actually tell us more about whether downloadById is working as designed?
There was a problem hiding this comment.
I was thinking that the combination of the verify and the assertion in the test would test that downloadById() is working as designed.
The verify checks that the getDownloadByIdUrl() is being called, which is an implementation detail, but an important one as that was the PR change
This verify alone doesn't test that the method is working properly however along with the assertion that the value from the verify call (expectedUrl) is the same as the one being passed to the web client call, we test that we call the right thing and that the right thing is passed.
|
Closing this PR. As discussed, we don't want clients to create functionality based off of this implementation detail and we will create a better way (pass an http header) to do this |
Updated
downloadByName()anddownloadById()to use their respectivegetDownloadByNameUrl()andgetDownloadByIdUrl()methods for improved extensibility