Framework-agnostic container & test lifecycle #702
Conversation
| @@ -0,0 +1,13 @@ | |||
| package org.testcontainers.lifecycle; | |||
|
|
|||
| public interface Startable extends AutoCloseable { | |||
There was a problem hiding this comment.
Why to we need this interface in this PR? It seems it's not used anyhwere (besides being implemented).
To use it in the testframework specific implementation?
There was a problem hiding this comment.
Yes, it defines a unified set of methods for both GenericContainer and DockerComposeContainer
|
|
||
| import java.util.Optional; | ||
|
|
||
| public interface TestDescription { |
There was a problem hiding this comment.
I wonder if we need all methods as the public interface.
The way BrowserWebDriverContainer works with it is kind of awkward, implementing the fallback if getFilesystemFriendlyName() is not implemented itself?
There was a problem hiding this comment.
What would you suggest? :)
There was a problem hiding this comment.
I think it makes sense to not have getFilesystemFriendlyName in here, since it seems oddly use case specific?
Or if we want to make it convenient, we could maybe have the default implementation here?
return getNameParts().map(it -> String.join("-", it));
And then the default implementation of getNameParts() would also become
return Optional.of(new String[]{
getTestId()
});
There was a problem hiding this comment.
oddly use case specific
But we have this use case at least in one place.
default implementation
I don't think this is a good default implementation to be honest, I would rather keep it explicit and let framework adapters think about it, what's the best way to provide a FS friendly name of the test
There was a problem hiding this comment.
What about omitting the default implementation completely then?
|
|
||
| public interface TestLifecycleAware { | ||
|
|
||
| default void beforeTestBlock(TestDescription description) { |
There was a problem hiding this comment.
Why beforeTestBlock and not beforeTest? What are the semantics of block?
There was a problem hiding this comment.
Just in case if some Test frameworks define Test as something more bigger and an atomic execution unit is some block of code. Arguable :)
There was a problem hiding this comment.
I think I would prefer removing block in order to remove the ambiguity, but your explanation is also fine, so that's a IMO thing.
There was a problem hiding this comment.
ok, I summon @rnorth as a tiebreaker 😄
| try { | ||
| imageName = image.get(); | ||
| } catch (Exception e) { | ||
| imageName = "<unknown>"; |
There was a problem hiding this comment.
I see it's just copied from the old implementation, but does this acutally happen?
There was a problem hiding this comment.
well, to give you a bit of history here:
When I was implementing https://github.com/testcontainers/testcontainers-java/pull/106/files, we wanted to keep the backward compatibility as much as possible, and one of such is Logger based on image's name :)
See #87