Skip to content

Conversation

llorllale
Copy link
Contributor

as per #38:

This PR:

  • Added primary ctor to LocalDocker
  • Added AssertRequest, a mock HttpClient useful for testing requests
  • Fixed typo in interface Docker
  • Reduces the scope of the puzzle of "create a unix socket server" to just the testing of UnixHttpClient
  • Leaves a puzzle to advance the design of AssertRequest

* Added primary ctor to `LocalDocker`
* Added `AssertRequest`, a mock HttpClient useful for testing requests
* Fixed typo in interface `Docker`
@coveralls
Copy link

Pull Request Test Coverage Report for Build 59

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.3%) to 72.727%

Totals Coverage Status
Change from base Build 58: 1.3%
Covered Lines: 32
Relevant Lines: 44

💛 - Coveralls

@coveralls
Copy link

coveralls commented Mar 13, 2018

Pull Request Test Coverage Report for Build 60

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.7%) to 72.093%

Totals Coverage Status
Change from base Build 58: 0.7%
Covered Lines: 31
Relevant Lines: 43

💛 - Coveralls

Copy link
Owner

@amihaiemil amihaiemil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@llorllale looks great, just one comment

* @param client The http client to use.
* @param baseUri Base URI.
*/
LocalDocker(final HttpClient client, final URI baseUri) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@llorllale Could you accept String version here, instead of URI baseUri, and move URI.create(...) in the call to super? I think it's more uniform, to have all ctors accepting the version and letting the object build the URI with the needed host :D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per PR review:
* The String version is now specified all the way up to the primary ctor
  of LocalDocker
@llorllale
Copy link
Contributor Author

@amihaiemil please check now

@amihaiemil
Copy link
Owner

@rultor merge it pls

@rultor
Copy link
Collaborator

rultor commented Mar 13, 2018

@rultor merge it pls

@amihaiemil OK, I'll try to merge now. You can check the progress of the merge here

@rultor rultor merged commit 23aa92a into amihaiemil:master Mar 13, 2018
@rultor
Copy link
Collaborator

rultor commented Mar 13, 2018

@rultor merge it pls

@amihaiemil Done! FYI, the full log is here (took me 2min)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants