Allow to start daemon database container #360

inikolaev · 2017-06-07T19:57:03Z

This PR fixes #359 by supporting new boolean option TC_DAEMON which prevents container from stopping when all connections are closed.

asafm · 2017-06-07T22:06:07Z

modules/jdbc/src/main/java/org/testcontainers/jdbc/ContainerDatabaseDriver.java

@@ -199,7 +203,7 @@ private Connection wrapConnection(final Connection connection, final JdbcDatabas

        return new ConnectionWrapper(connection, () -> {
            finalConnections.remove(connection);
-            if (finalConnections.isEmpty()) {
+            if (!isDaemon && finalConnections.isEmpty()) {


Shouldn't you avoid starting and reuse the existing container if it exists?

This line only prevents running container from stopping if TC_DAEMON=true was specified in connection string.

ContainerDatabaseDriver maintains a mapping jdbcUrlContainerCache which maps JDBC connection string to the container instance. New container won't start If such a mapping exists.

My change basically ensures that this mapping is not removed once all connections are closed.

kiview · 2017-06-08T07:56:26Z

Do you think you can provide a test for this feature as well?

inikolaev · 2017-06-08T08:12:46Z

I tried making a test in jdbc module but it turned out it doesn't actually start container and there's no access to container instance, unless I pass it into ConnectionWrapper or add some helper static method into ContainerDatabaseDriver.

I can try to use Mockito and I guess I need to register some mock JdbcDatabaseContainerProvider which will return mock JdbcDatabaseContainer and then assert whether stop() method has been called or not.

bsideup · 2017-06-08T08:18:41Z

@inikolaev JDBC tests are here:
https://github.com/testcontainers/testcontainers-java/tree/master/modules/jdbc-test

inikolaev · 2017-06-08T09:14:11Z

Added test, please have a look

bsideup · 2017-06-08T09:20:06Z

modules/jdbc-test/src/test/java/org/testcontainers/jdbc/JDBCDriverTest.java

@@ -70,6 +77,7 @@ public static void testCleanup() {
    @Test
    public void test() throws SQLException {
        performSimpleTest(jdbcUrl);
+        performContainerStatusCheck(jdbcUrl);


Since we test just a single case of TC_DAEMON here, maybe it worth to create a separate test, without Parameterized runner?

I thought about that, but at the same time now it tests that container is stopped for other tests without TC_DAEMON set.

Can do either way, don't have a strong opinion on it.

I would go with a separate test where you test both scenarios (with and without TC_DAEMON), just to make it clear if it's broken or not. IMO Parameterized runner seems to be misused in general in JDBCDriverTest :)
Thanks!

Ok! One more thing, I see I'm getting a PMD violation about using package private method: ContainerDatabaseDriver.getContainer().
Would it be fine to make it public? I saw some classes use VisibleForTesting annotation, but I'm not sure if it makes any difference.

Yes, VisibleForTesting should be used here

bsideup · 2017-06-08T12:05:02Z

modules/jdbc-test/src/test/java/org/testcontainers/jdbc/JDBCDriverTest.java

@@ -84,6 +88,27 @@ public void test() throws SQLException {
        }
    }

+    @Test
+    public void shouldStopContainerWhenAllConnectionsClosed() throws SQLException {


please move these two tests to a separate Test class, otherwise, they will execute for every parameterized data entry

Ouch, didn't realize that :)

bsideup · 2017-06-09T08:45:10Z

Looks good to me.
@rnorth can you take a look?

rnorth

Looks good to me - only very minor suggestions, and a good contribution.
Thanks also for updating the docs!

Please could you also update the CHANGELOG file? I apologise that you're the first person I'm asking to do this, but we're trying to automate the release process a bit more and having changelog updates done in advance would probably be a good idea.

Please could you add something like this near the top of the changelog?

## UNRELEASED
### Changed
- Added `TC_DAEMON` JDBC URL flag to prevent `ContainerDatabaseDriver` from shutting down containers at the time all connections are closed.

rnorth · 2017-06-10T08:23:02Z

modules/jdbc-test/src/test/java/org/testcontainers/jdbc/DatabaseDriverTest.java

+/**
+ * Created by inikolaev on 08/06/2017.
+ */
+public class DatabaseDriverTest {


Please could you rename to be slightly more specific? i.e., this could be called DatabaseDriverShutdownTest or something like that.

rnorth · 2017-06-10T08:24:14Z

modules/jdbc/src/main/java/org/testcontainers/jdbc/ContainerDatabaseDriver.java

+     * @return an instance of database container or <code>null</code> if no container associated with JDBC URL
+     */
+    @VisibleForTesting
+    public static JdbcDatabaseContainer getContainer(String jdbcUrl) {


If this is only for use in testing, couldn't we make it package-scoped? That way it doesn't become part of the public API.

It was package-private first, but that causes PMD violation when Codacy runs different checks. Any ideas on how to fix that?

I would ignore the Codacy check and use package-scope.

Is there a way to adjust PMD rules? Else a more serious issue might be missed due to the noise this issue will add to every build.

There are lot's of places in the existing codebase, where we're violating this rule. Since it's not a really useful rule if you want to use package-scope, I'd say we simply deactivate this pattern in Codacy. @rnorth are you okay with this?

inikolaev · 2017-06-10T21:14:21Z

Could someone trigger Travis build - seems like it was terminated for some reason, but is passes locally.

rnorth · 2017-06-11T05:16:07Z

@inikolaev have restarted the build, sorry. Not quite sure why it hung there..

bsideup · 2017-06-11T08:55:37Z

Green now :)
/cc @inikolaev @rnorth

rnorth · 2017-06-11T08:58:26Z

Merged, thank you @inikolaev !

inikolaev · 2017-06-11T08:59:14Z

Awesome! 😄

inikolaev added 2 commits June 7, 2017 22:38

Allow to start daemon database container

3b7830b

Add documentation for TC_DAEMON parameter

27d7ef7

inikolaev mentioned this pull request Jun 7, 2017

Keep container running for as long as JVM is running #359

Closed

asafm reviewed Jun 7, 2017

View reviewed changes

bsideup reviewed Jun 8, 2017

View reviewed changes

inikolaev mentioned this pull request Jun 8, 2017

Support TestContainers JDBC connection string flyway/flyway#1663

Merged

inikolaev force-pushed the daemon-container branch from 55f7ff6 to d95abae Compare June 8, 2017 11:24

bsideup reviewed Jun 8, 2017

View reviewed changes

inikolaev force-pushed the daemon-container branch 2 times, most recently from f429d07 to fdf33ed Compare June 8, 2017 18:56

rnorth requested changes Jun 10, 2017

View reviewed changes

inikolaev added 2 commits June 10, 2017 16:57

Add tests for TC_DAEMON parameter

69c8736

Update changelog

c07e31d

inikolaev force-pushed the daemon-container branch from fdf33ed to c07e31d Compare June 10, 2017 13:58

bsideup added this to the 1.3.1 milestone Jun 10, 2017

bsideup added the enhancement label Jun 10, 2017

rnorth approved these changes Jun 11, 2017

View reviewed changes

rnorth merged commit 05192e6 into testcontainers:master Jun 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to start daemon database container #360

Allow to start daemon database container #360

inikolaev commented Jun 7, 2017

asafm Jun 7, 2017

inikolaev Jun 8, 2017

kiview commented Jun 8, 2017

inikolaev commented Jun 8, 2017

bsideup commented Jun 8, 2017

inikolaev commented Jun 8, 2017

bsideup Jun 8, 2017

inikolaev Jun 8, 2017

bsideup Jun 8, 2017

inikolaev Jun 8, 2017 •

edited

Loading

bsideup Jun 8, 2017

bsideup Jun 8, 2017

inikolaev Jun 8, 2017

bsideup commented Jun 9, 2017

rnorth left a comment

rnorth Jun 10, 2017

rnorth Jun 10, 2017

inikolaev Jun 10, 2017

kiview Jun 10, 2017

inikolaev Jun 10, 2017

kiview Jun 10, 2017 •

edited

Loading

inikolaev commented Jun 10, 2017

rnorth commented Jun 11, 2017

bsideup commented Jun 11, 2017

rnorth commented Jun 11, 2017

inikolaev commented Jun 11, 2017

Allow to start daemon database container #360

Allow to start daemon database container #360

Conversation

inikolaev commented Jun 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kiview commented Jun 8, 2017

inikolaev commented Jun 8, 2017

bsideup commented Jun 8, 2017

inikolaev commented Jun 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inikolaev Jun 8, 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

bsideup commented Jun 9, 2017

rnorth left a 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 Jun 10, 2017 • edited Loading

Choose a reason for hiding this comment

inikolaev commented Jun 10, 2017

rnorth commented Jun 11, 2017

bsideup commented Jun 11, 2017

rnorth commented Jun 11, 2017

inikolaev commented Jun 11, 2017

inikolaev Jun 8, 2017 •

edited

Loading

kiview Jun 10, 2017 •

edited

Loading