-
-
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
Implement dependsOn
for cross-container dependencies
#1404
Conversation
@bsideup instead of depending on containers with the actual An example of what I had in mind: public class MyTest {
@Container
public static GenericContainer a = new GenericContainer(...).withID("a");
@Container
public static GenericContainer b = new GenericContainer(...).dependsOn("a");
} WDYT? |
@aguibert I have a really bad feeling about it.
public class MyTest {
@Container
public static GenericContainer a = new GenericContainer(...);
@Container
public static GenericContainer b = new GenericContainer(...).dependsOn(a);
} |
ah, that code example you posted makes sense. I forgot that was an option because I'm used to not being able to reference other container fields like this: public class MyTest {
@Container
public static GenericContainer a = new GenericContainer(...);
@Container
public static GenericContainer b = new GenericContainer(...)
.withEnv("A_PORT", a.getFirstMappedPort()); // blows up because we call `getFirstMappedPort()` too early -- before `a` is started and has its port assigned.
} |
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 good! I have some minor comments but nothing big; feel free to merge when you're satisfied.
} | ||
|
||
VisibleAssertions.assertEquals("Started once", 1, startable.getStartInvocationCount().intValue()); | ||
VisibleAssertions.assertEquals("Does not trigger .stop()", 0, startable.getStopInvocationCount().intValue()); |
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'm sure there's a good reason, but why do we not propagate a stop()
call through the dependencies?
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 there is no guarantee that the dependencies will be a part of the same "group".
We may introduce deepStop
in future
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 a deepStop
would be useful for when you know these are the semantics you want. I end up having to manually create one in several projects. Thoughts on adding an API?
Btw, are both stop
and start
required to be idempotent? I think the current implementation of deepStart
depends on this.
} | ||
|
||
@Test | ||
public void shouldStartTransitiveDependencies() { |
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.
Just eyeballing the code, it looks like we're safe from circular dependencies, and also 'diamond' dependencies should be OK too.
Do you think it might be good to have tests for these scenarios in case a regression is introduced later?
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.
Circular dependencies won't work (we have "hard depends on")
I will add a test for the diamond case 👍
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 great!
No description provided.