1st draft for review #1

rnorth · 2017-11-11T16:46:53Z

As requested, I've converted this into a PR for easier review.

bsideup · 2017-11-11T22:14:49Z

README.md

+```java
+// Building a simple container object not linked to any test framework
+final Container container = Container.builder()
+  .withImage("alpine:3.5")


since image is a mandatory thing maybe we should require it as a parameter of builder() method?

Yep, makes sense.

bsideup · 2017-11-27T16:19:24Z

README.md

+public class SimpleJUnit4Test {
+
+    @Rule
+    public ContainerTestRule alpine = ContainerTestRule.builder()


If we do it this way, we will have to duplicate each and every withXXX method. What if we use something like I proposed with Networks? i.e.:

Container.builder() .withSomething() .build(asJUnit4Rule()); Container.builder() .withSomething() .build(asJUnit5Rule()); Container.builder() .withSomething() .build(); // aka GenericContainer

What about composing it the other way around?
Something like:

ContainerTestRule.builder( Container.builder() .withSomething() ).build();

@kiview I thought about it as well, but (compared to my example) it adds more code around it without providing any visible benefits IMO. Also, if .build() accepts an interface, it will be possible to easily discover all available adapters like JUnit4/5 and others

+1 for the discoverability, makes sense.

Actually as-is we don't have to duplicate the with methods... If you look, the junit.ContainerTestRuleBuilder class extends BaseContainerBuilder, which does the generic self-typed-return thing we currently do in Testcontainers 1.x. (This helps allow for a fluent interface).

containercore.ContainerBuilder also uses this same base class as well.

As such, we don't need to duplicate the methods.
The downside is that self-typed-returns are a bit confusing, but at least they're isolated to the builder classes now. I'm not sure how you feel about keeping this technique in use, though.

@bsideup I'm all for the approach you suggest if we don't go for this model above. The only reason I didn't go for that straight away is a slight discoverability worry.

TBH I myself don't really understand the type hack 😅
It's for keeping the this type when calling methods from the parent class?

Hehe, yes, it's so SomeSubclass::withSomeMethodFromParent returns as SomeSubclass rather than the parent class.
I'll consider this as a vote against 😆

bsideup · 2017-11-27T16:21:18Z

README.md

+
+    @Test
+    public void doNothing() {
+        alpine.exec("date");


re exec and others. Since the number of such "helpers" will grow, how about introducing a concept of "recipes"? i.e. ExecRecipe, GetFileRecipe, etc?

I think in general we want a OO abstraction of docker-java, so I assume we will have all basic docker API methods available, like:

cp

exec

diff

...

I always wonder how much to deviate from the known Docker API or if it would be kind of enough to conveniently expose these methods.

Of course we should name these methods in a Java idiomatic way.

Your example recipes are now basically Docker API methods, can you image more complex helper methods?

I'm not sure that this is the goal actually :D The goal is to have a high-level API to start and manipulate the containers, and cp/exec/diff/etc are too low level for that

Yes, this is a good thing to discuss. The way I see it, the library should be 'narrow but deep':

Narrow: we don't need to cover every possible method of the docker API (probably >50%, but definitely <100%)
Deep: we should aim to do as much as possible for the user to hide the 'accidental complexity' of docker - e.g. all the things around file mounting, copying, and network port mapping.

By hiding some of this complexity we do reduce the flexibility of the docker API, but this is unavoidable.

There might be people who want a 100% wide abstraction, but TBH I think these people need the finer-grained, lower-level tools that docker-java provides, so we're OK to avoid this area.

@bsideup I don't think cp and exec are too low level per se, since both of your examples are basically renamed calls to these methods, aren't they ? ;)

There might be more convenient API composed on top of the basic APIs, but I don't see any pain in exposing them in a public API, still more convenient to use in a OO manner then docker-java.

bsideup · 2017-11-27T16:23:38Z

README.md

+```java
+final Container containerWithPlugin = Container.builder()
+  .withImage("alpine:3.5")
+  .withPlugin(new FakeTimePlugin("2010-01-01 00:00:00"))


maybe extension would be a better name? So that we extend container by composing different extensions, but the system itself is not pluggable, at least not with this API

I think both terms are fine and apply here, the Wikipedia definition of plugin:

In computing, a plug-in (or plugin, add-in, addin, add-on, addon, or extension) is a software component that adds a specific feature to an existing computer program.

We add functionality or behavior to the container, but we could also extend the existing feature set ;)

There is also the plugin definition by Martin Fowler in Patterns of Enterprise Application Architecture which would fit to this implementation I think (basically meaning DI).

I don't mind; 'plugin' seemed more natural to me.

One thing which occurred to me is that this isn't essential functionality from day one (it's a new, rather than existing, feature).

Perhaps let's not spend too much time worrying about the details for now, as long as we can agree we'd like this concept to exist in some form 😄

Yes, since the fluent builder API would allow us to introduce such functionality without any breaking changes we can simply defer the discussion 🙂

bsideup · 2017-11-27T16:24:18Z

README.md

+  .build();
+
+// later, while container is running
+fakeTime.setTime("2011-01-01 00:00:00");


would make it hard to reuse. How about containerWithPlugin.using(fakeTime).setTime(...)?

What's the difference, since we still need the reference to the field?

containerFoo.using(fakeTime).setTime(...) containerBar.using(fakeTime).setTime(...)

Would be nice, might be problematic for stateful Plugins (which might exist).

Or maybe we could have something like:

final Container containerWithPlugin = Container.builder() .withImage("alpine:3.5") .withPlugin(FakeTimePlugin.class); containerWithPlugin.using(FakeTimePlugin.class).setTime(...);

Stateful plugins is a pain. Even if plugin wants to store the state, it's better if it will use something like https://docs.oracle.com/javase/7/docs/api/java/util/WeakHashMap.html

bsideup · 2017-11-27T16:35:43Z

README.md

+
+---
+
+##Runtime `Container` vs define-time `ContainerBuilder`


We should take a look at https://github.com/sundrio/sundrio/blob/master/annotations/dsl/readme.md for this. It's being used in fabric8's Docker client and seems to be very powerful DSL generator

👍 Thanks for the tip - will have a look

raphw · 2017-11-28T22:08:14Z

src/main/java/org/testcontainers/containercore/Container.java

+/**
+ * TODO: Javadocs
+ */
+public class Container {


I would like to suggest making this class AutoClosable. The interface can simply invoke stop and should override the method to remove the Exception as none is thrown.

This allows for a very comfortable usage of containers such as:

try(Container container = new MyContainer()) {
container.start();
// do something with container..
}

@raphw 👍 good catch! In fact, we already have it in TestContainers and it proved to be useful :)

👍 absolutely - a small detail that adds a lot of benefit. Thanks!

kiview · 2017-11-28T22:36:03Z

README.md

+  * making sure all fluent setters are consistent
+  * adding a separation between definition-time concerns and 'run-time' concerns
+  * add a plugin model to allow *composition* to be used in addition to *inheritance*
+* **Continue to support specialized container types via specialized classes** 


Since we already started the discussion in Slack - we don't want to have Docker-Compose support as part of containercore, correct?

Worth bringing up, and we should really add to this doc as it's a 'future of testcontainers' question. In brief, I think I'd see it as:

Testcontainers / Test framework integrations (e.g. JUnit4/5, Spock, etc) \ Specialised container support (e.g. database, selenium, compose, etc) Containercore -- High level OO docker API

I'd hope there's nothing in containercore that has to be aware of docker-compose.
Does that make sense?

If we decide to exclude docker-compose from core (which makes sense), we should IMO provide a compose-core module, which itself is decoupled from testcontainers. There are really a lot of use cases for the high level docker-compose support, even outside of test frameworks. And if I think about it, not all of the specialized containers seem to be testcontainers specific.

So maybe it would be more like this?

Testcontainers -- Test framework integrations (e.g. JUnit4/5, Spock, etc) Containercore -- High level OO docker API Containercore-modules -- Specialised container support (e.g. database, selenium, compose, etc)

Fair point - trying out a couple of ways to achieve this now.

kiview · 2017-11-28T22:46:36Z

README.md

+```java
+final Container containerWithPlugin = Container.builder()
+  .withImage("alpine:3.5")
+  .withPlugin(new FakeTimePlugin("2010-01-01 00:00:00"))


I think both terms are fine and apply here, the Wikipedia definition of plugin:

In computing, a plug-in (or plugin, add-in, addin, add-on, addon, or extension) is a software component that adds a specific feature to an existing computer program.

We add functionality or behavior to the container, but we could also extend the existing feature set ;)

There is also the plugin definition by Martin Fowler in Patterns of Enterprise Application Architecture which would fit to this implementation I think (basically meaning DI).

kiview · 2017-11-28T22:49:07Z

README.md

+  .build();
+
+// later, while container is running
+fakeTime.setTime("2011-01-01 00:00:00");


What's the difference, since we still need the reference to the field?

LMnet · 2018-01-23T04:36:16Z

README.md

+### Implementation
+
+* `Container` holds configuration data in fields, plus methods for 'run-time' interactions such as `start`, `stop` , `exec`, etc.
+* `ContainerBuilder` (and its parent class `BaseContainerBuilder`)  exposes the define-time `withXYZ` fluent setter API


What do you guys think about the immutable builder? With the current proposal builder is mutable and it could lead to some surprising behavior. For example, I want to have 2 PosgtreSQL containers who shares some common settings:

final ContainerBuilder commonBuilder = PostgreSQLContainer.builder().withImage("posgtresql:9.6"); final Container pg1 = commonBuilder.withExposedPort(1234).build(); final Container pg2 = commonBuilder.withExposedPort(2345).build();

But this code would not work as expected because of mutable nature of builders.

I think immutability is always a good default for the libraries. In testcontainers we don't need high-performant builders, so extra allocations is not a problem.

LMnet · 2018-01-23T04:53:01Z

The new proposal doesn't take into account tests with multiple dependent containers. With the current implementation, I had some issues and had to use some hacks. testcontainers currently expose api only for test<->container interoperability, but there is no convenient container<->container interface. And I think one of the most exciting use case for testcontainers is tests with multiple dependent containers.

I suggest adding some methods to Container to expose ports, ips and other network information for linking containers.

bsideup · 2018-01-23T07:05:46Z

@LMnet actually, we have Networks support in TestContainers for the container <-> container communication:
https://github.com/testcontainers/testcontainers-java/blob/master/core/src/test/java/org/testcontainers/containers/NetworkTest.java

LMnet · 2018-01-23T07:18:19Z

@bsideup Yes, I know about networks support. But it looks like container <-> container communication features are not given enough attention. For example, to obtain an IP for a container inside a network I need to do something like this:

container.getContainerInfo().getNetworkSettings().getNetworks().get(myNetworkId).getIpAddress()

Looks a bit verbose for such simple operation.

bsideup · 2018-01-23T07:21:56Z

@LMnet why would you do it at all? Just use a network alias (i.e. my-super-container) and use it instead of IP

LMnet · 2018-01-23T07:32:11Z

@bsideup It was just an example of API usage.

Another real-life example. In my tests, I start an HTTP mock on my host machine (from the test code). The application is inside a container, and this container is inside a custom network. The application should be able to do HTTP request to the mock. To link mock with the application I need to get network's gateway address inside the container - and it would host's address. So, to get a gateway address I need something like this:

container.getContainerInfo().getNetworkSettings().getNetworks().get(myNetworkId).getGateway()

ATM I don't know any better way to do this.

1st draft for review

d810eb9

bsideup mentioned this pull request Nov 27, 2017

Extract Container runners from JUnit specific code testcontainers/testcontainers-java#506

Closed

bsideup reviewed Nov 27, 2017

View reviewed changes

raphw reviewed Nov 28, 2017

View reviewed changes

kiview reviewed Nov 28, 2017

View reviewed changes

kiview mentioned this pull request Nov 29, 2017

Junit 5 support testcontainers/testcontainers-java#490

Closed

kiview mentioned this pull request Dec 9, 2017

Issue when testcontainers is used from within a Docker container testcontainers/testcontainers-java#502

Closed

rnorth added 2 commits December 10, 2017 15:37

Split out MySqlContainerTestRuleBuilder into separate file for clarity

378fe7e

Implement AutoCloseable for easier standalone usage

b730a28

LMnet mentioned this pull request Jan 22, 2018

Dependent containers testcontainers/testcontainers-scala#12

Closed

LMnet reviewed Jan 23, 2018

View reviewed changes

1st draft for review #1

Are you sure you want to change the base?

1st draft for review #1

Conversation

rnorth commented Nov 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kiview Nov 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnorth Nov 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LMnet commented Jan 23, 2018

bsideup commented Jan 23, 2018

LMnet commented Jan 23, 2018

bsideup commented Jan 23, 2018

LMnet commented Jan 23, 2018

kiview Nov 28, 2017 •

edited

Loading

rnorth Nov 29, 2017 •

edited

Loading