Include image name in RemoteDockerImage#toString #2558
Conversation
…thub.com/locationlabs/testcontainers-java into locationlabs-image_name_in_RemoteDockerImage_toString
bsideup
changed the title
| } | ||
|
|
||
| try { | ||
| return getImageName().toString(); |
There was a problem hiding this comment.
Why is try necessary after the future is done?
There was a problem hiding this comment.
you need to get the value from the future. Unless I miss something 😅
|
|
||
| @Test | ||
| public void toStringContainsOnlyImageName() { | ||
| String imageName = Base58.randomString(8).toLowerCase(); |
There was a problem hiding this comment.
Because constant example string seems more controlled to me.
|
LGTM, just general questions for better understanding. |
| try { | ||
| return getImageName().toString(); | ||
| } catch (InterruptedException | ExecutionException e) { | ||
| return e.getMessage(); |
There was a problem hiding this comment.
This caught me by surprise - in case of a failure, why would we not rethrow? At first sight it seems odd that an exception message could become the 'name' of the image...
There was a problem hiding this comment.
this is for toString() - it would be weird to rethrow from toString IMO. Instead, we return the message, so that RemoveDockerImage#toString shows that there was an exception
There was a problem hiding this comment.
Fair enough - I guess we can't do too much more than this.
There was a problem hiding this comment.
Yes, might create some nasty bugs if rethrowing.
|
Woah sorry, closed by accident from Intellij 😱 |
delete-merged-branch
bot
deleted the
locationlabs-image_name_in_RemoteDockerImage_toString
branch
| } | ||
|
|
||
| @Test | ||
| public void toStringWithExceptionContainsOnlyImageNameFuture() { |
There was a problem hiding this comment.
Looks like this method name doesn't really match the contents anymore
| public class RemoteDockerImageTest { | ||
|
|
||
| @Test | ||
| public void toStringContainsOnlyImageName() { |
There was a problem hiding this comment.
This method name came to be back when there was a choice between imageName and imageNameFuture. Maybe a better name now is toStringWithCompletedImageName ?
|
This was released in https://github.com/testcontainers/testcontainers-java/releases/tag/1.14.0 🎉 Thanks for the contribution! |
* clean up imports in GenericContainer * include image name in RemoteDockerImage.toString() to fix testcontainers#2443 * test RemoteDockerImage directly * handle failures getting the image name * use completeExceptionally instead of mocking * tweak assertions * more assertion tweaks * add isDone check to RemoteDockerImage.toString -- still figuring out why test fails * go back to using lombok @tostring, exclude dockerClient while we're at it * use `Futures.lazyTransform` that also proxies `isDone` * `imageNameToString` should not be public * Include the message on error Co-authored-by: Byron David <david.byron@avast.com> Co-authored-by: David Byron <dbyron@dbyron.com>
This PR superseeds #2450 with some minor fixes