This repository has been archived by the owner on Mar 21, 2022. It is now read-only.
simplify DefaultDockerClientUnitTest #753
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
refactor this test to use a MockWebServer rather than mocking the Jersey/jax-rs Client interface used by the DefaultDockerClient. Using a (Mockito) mock Client is problematic for a test since it has such a rich and fluent interface - the test needs to know exactly which methods are called on the Client. Ultimately, this is not what a unit test of DefaultDockerClient should care about - a good test would only care about what HTTP requests it sends to the Docker Remote API and how it behaves when it gets certain HTTP responses back. The MockWebServer allows us to control the HTTP responses and capture the HTTP requests much easier than using a Mockito mock of the particular HTTP client library used by DefaultDockerClient. With this approach, it would be possible to one day change DefaultDockerClient to use a different HTTP client library without having to change one line of this test. The same cannot be said of using Mockito. This change was initially driven by a desire to make PR #749 easier to test, to check that it sends expected authentication headers, but I think it will be useful and help improve testing in docker-client across the board. https://github.com/square/okhttp/tree/master/mockwebserver
mattnworb
force-pushed
the
mattbrown/unit-test-mockhttpserver
branch
from
4b410f4 to
e4aaf61
Compare
there is no need to customize how the Jersey Client is built
|
with this, the unit test becomes much simpler to read, with less code, and is also testing more of what we actually care about - HTTP request/response content - instead of which specific methods are called on the jax-rs Client. cc @johnflavin |
This was referenced May 20, 2017
Closed
hamcrest-jackson requires Java 8; but we still build docker-client for Java 7. Remove the usage of this library in the test until we upgrade docker-client to Java 8.
mattnworb
force-pushed
the
mattbrown/unit-test-mockhttpserver
branch
from
45aa52b to
0f021bb
Compare
Codecov Report
@@ Coverage Diff @@
## master #753 +/- ##
============================================
+ Coverage 65.59% 68.48% +2.89%
+ Complexity 656 655 -1
============================================
Files 153 153
Lines 2883 3268 +385
Branches 335 340 +5
============================================
+ Hits 1891 2238 +347
- Misses 845 880 +35
- Partials 147 150 +3 |
davidxia
reviewed
May 22, 2017
| Assert.assertEquals(1, valueArgument.getValue()); | ||
| assertThat(recordedRequest.getHeader("int"), is("1")); | ||
| assertThat(recordedRequest.getHeader("string"), is("2")); | ||
| // TODO (mbrown): this seems like incorrect behavior - the client should send 3 headers with |
There was a problem hiding this comment.
Is this an issue with our code or upstream?
There was a problem hiding this comment.
it is an existing bug in this Builder's "Add header" method which I didn't want to fix as a part of this, but I think should be fixed at some point
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
refactor this test to use a MockWebServer rather than mocking the
Jersey/jax-rs Client interface used by the DefaultDockerClient.
Using a (Mockito) mock Client is problematic for a test since it has
such a rich and fluent interface - the test needs to know exactly which
methods are called on the Client.
Ultimately, this is not what a unit test of DefaultDockerClient should
care about - a good test would only care about what HTTP requests it
sends to the Docker Remote API and how it behaves when it gets certain
HTTP responses back.
The MockWebServer allows us to control the HTTP responses and capture
the HTTP requests much easier than using a Mockito mock of the
particular HTTP client library used by DefaultDockerClient.
With this approach, it would be possible to one day change
DefaultDockerClient to use a different HTTP client library without
having to change one line of this test. The same cannot be said of using
Mockito.
This change was initially driven by a desire to make PR #749 easier to
test, to check that it sends expected authentication headers, but I
think it will be useful and help improve testing in docker-client across
the board.
https://github.com/square/okhttp/tree/master/mockwebserver