-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Include image name in RemoteDockerImage#toString #2558
Include image name in RemoteDockerImage#toString #2558
Conversation
…thub.com/locationlabs/testcontainers-java into locationlabs-image_name_in_RemoteDockerImage_toString
} | ||
|
||
try { | ||
return getImageName().toString(); |
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.
Why is try necessary after the future is done?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why random string?
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.
why not? :)
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough - I guess we can't do too much more than this.
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.
Yes, might create some nasty bugs if rethrowing.
Woah sorry, closed by accident from Intellij 😱 |
} | ||
|
||
@Test | ||
public void toStringWithExceptionContainsOnlyImageNameFuture() { |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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