Merge pull request #760 from spotify/mattbrown/encourage-unit-tests

add note pointing contributors to DefaultDockerClientUnitTest

CONTRIBUTING.md

@@ -34,9 +34,28 @@ before writing code to check we're on the same page.
 
 You can build and test by following [instructions here][1].
 
+### Unit tests and integration tests
+When adding new functionality to DefaultDockerClient, please consider and
+prioritize adding unit tests to cover the new functionality in
+[DefaultDockerClientUnitTest][] rather than integration tests that require a
+real docker daemon in [DefaultDockerClientTest][].
+
+DefaultDockerClientUnitTest uses a [MockWebServer][] where we can control the
+HTTP responses sent by the server and capture the HTTP requests sent by the
+DefaultDockerClient, to ensure that it is communicating with the Docker Remote
+API as expected.
+
+While integration tests are valuable, they are more brittle and harder to run
+than a simple unit test that captures/asserts HTTP requests and responses, and
+they end up testing both how docker-client behaves and how the docker daemon
+itself behaves.
+
   [1]: https://github.com/spotify/docker-client#testing
   [2]: https://github.com/spotify/docker-maven-plugin
   [3]: https://github.com/spotify/helios
   [4]: https://github.com/spotify/docker-gc
   [5]: https://github.com/spotify/helios-skydns
   [6]: https://github.com/spotify/helios-consul
+  [DefaultDockerClientTest]: src/test/java/com/spotify/docker/client/DefaultDockerClientTest.java
+  [DefaultDockerClientUnitTest]: src/test/java/com/spotify/docker/client/DefaultDockerClientUnitTest.java
+  [MockWebServer]: https://github.com/square/okhttp/tree/master/mockwebserver

src/test/java/com/spotify/docker/client/DefaultDockerClientTest.java

@@ -279,6 +279,15 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+/**
+ * Integration tests for DefaultDockerClient that assume a docker daemon is available to connect to
+ * at the DOCKER_HOST environment variable.
+ * <p>
+ * When adding new functionality to DefaultDockerClient, <b>please consider adding new unit tests
+ * to {@link DefaultDockerClientUnitTest} rather than integration tests to this file</b>, for all of
+ * the reasons outlined in that class.
+ * </p>
+ */
 public class DefaultDockerClientTest {
 
   private static final String BUSYBOX = "busybox";

src/test/java/com/spotify/docker/client/DefaultDockerClientUnitTest.java

@@ -54,8 +54,14 @@
  * responses sent by the server and capture the HTTP requests sent by the class-under-test is far
  * simpler that attempting to mock the {@link javax.ws.rs.client.Client} instance used by
  * DefaultDockerClient, since the Client has such a rich/fluent interface and many methods/classes
- * that would need to be mocked. Ultimately for testing DefaultDockerClient all we care about is the
- * HTTP requests it sends, rather than what HTTP client library it uses.</p>
+ * that would need to be mocked. Ultimately for testing DefaultDockerClient all we care about is
+ * the HTTP requests it sends, rather than what HTTP client library it uses.</p>
+ * <p>
+ * When adding new functionality to DefaultDockerClient, please consider and prioritize adding unit
+ * tests to cover the new functionality in this file rather than integration tests that require a
+ * real docker daemon in {@link DefaultDockerClientTest}. While integration tests are valuable,
+ * they are more brittle and harder to run than a simple unit test that captures/asserts HTTP
+ * requests and responses.</p>
  *
  * @see <a href="https://github.com/square/okhttp/tree/master/mockwebserver">
  * https://github.com/square/okhttp/tree/master/mockwebserver</a>