Container inspect and start + unit tests #56

amihaiemil · 2018-03-18T18:20:45Z

PR for #46 implemented and unit tested Container.inspect and Container.start. Left puzzle for continuing implementation.

0crat · 2018-03-18T18:21:49Z

Job #56 is now in scope, role is REV

coveralls · 2018-03-18T18:22:31Z

Pull Request Test Coverage Report for Build 84

16 of 19 (84.21%) changed or added relevant lines in 2 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+3.6%) to 81.553%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/main/java/com/amihaiemil/docker/RtDocker.java	0	3	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/main/java/com/amihaiemil/docker/RtDocker.java	1	42.86%

Totals
Change from base Build 76:	3.6%
Covered Lines:	84
Relevant Lines:	103

💛 - Coveralls

amihaiemil · 2018-03-18T18:27:00Z

@0crat assign @llorllale

0crat · 2018-03-18T18:28:17Z

@0crat assign @llorllale (here)

@amihaiemil This pull request #56 is assigned to @llorllale /z, here is why. The budget is 15 minutes, see §4. Please, read §27 and when you decide to accept the changes, inform @amihaiemil /z (the architect) right in this ticket. If you decide that this PR should not be accepted ever, also inform the architect.

0crat · 2018-03-18T18:28:57Z

Manual assignment of issues is discouraged, see §19: -5 points just awarded to @amihaiemil /z

amihaiemil · 2018-03-18T18:30:45Z

@llorllale I gave you the REV role. I think this will catch some mistakes I make and also help you learn the architecture better :D

llorllale · 2018-03-19T15:39:25Z

src/main/java/com/amihaiemil/docker/RtContainer.java

+        final HttpResponse response = this.client.execute(start);
+        final int status = response.getStatusLine().getStatusCode();
+        if(status!= HttpStatus.SC_NO_CONTENT) {
+            throw new IllegalStateException(


@amihaiemil why not throw IOException?

@llorllale Because, as I understand it, IOException should be used only when there are real issues on the network. We have a puzzle for that, we would need an UnexpectedResponseException, which would provide the status and reason phrase to the user.

So, once that puzzle is fixed, this throw will be changed as well, it won't be IllegalStateException.

If we simply throw IOException the user won't know whether there was a real networking issue (e.g. timeout connection) or something else.

What do you think?

@amihaiemil It's a design decision. We are currently not declaring any other checked exception, so I assume this new UnexpectedResponseException would be runtime? I'd throw an IOException (or a subclass) because it's already in the API's contract.

I think this is a design call and that you have final say.

llorllale

@amihaiemil see above

llorllale · 2018-03-19T15:47:32Z

src/main/java/com/amihaiemil/docker/RtContainer.java

+        } else {
+            info = null;
+        }
+        inspect.releaseConnection();


@amihaiemil see this. I think we should leave a puzzle for abstracting away all the low-level apache http handling stuff into a single point. Something like "Encapsulate HTTP connection/request/response handling" I guess. We'd gain:

Reduced risk per PR since we won't have to keep implementing/reviewing correct handling of HTTP resources

Reduce procedural code throughout the code base

llorllale · 2018-03-19T15:57:01Z

src/test/java/com/amihaiemil/docker/RtContainerTestCase.java

@@ -25,12 +25,17 @@
 */


@amihaiemil you're leaving a puzzle for finishing implementation of the rest of the Container API, but I feel we need another one specifically for adding more test cases for start and inspect

llorllale · 2018-03-19T16:04:18Z

src/main/java/com/amihaiemil/docker/RtContainer.java

+        final HttpResponse response = this.client.execute(start);
+        final int status = response.getStatusLine().getStatusCode();
+        if(status!= HttpStatus.SC_NO_CONTENT) {
+            throw new IllegalStateException(


@amihaiemil It's a design decision. We are currently not declaring any other checked exception, so I assume this new UnexpectedResponseException would be runtime? I'd throw an IOException (or a subclass) because it's already in the API's contract.

I think this is a design call and that you have final say.

amihaiemil · 2018-03-19T17:19:35Z

@llorllale I wrote more unit tests and left a puzzle for integration tests.

About refactoring of HTTP calls, yes, should be done eventually (I also noticed we already have a little duplication, especially with reading the Json payload). But I am not sure what you have in mind about it, could you open an Issue and explain in more detail?

And yes, UnexpectedResponseException would be a RuntimException, mostly because users may be sure that the calls will be succesful (container exists, can be started etc), it makes no sense to polute their code with try/catch always.

llorllale · 2018-03-19T17:21:54Z

@amihaiemil

it makes no sense to polute their code with try/catch always.

But Container.start() declares a checked exception (IOException)...

amihaiemil · 2018-03-19T17:25:59Z

@llorllale Yes, but IOException is another story; as I said above, that is for networking problems. Receiving a 404 response from Container.inspect() for instance, is not a networking problem, that's how the API works since the Container was not found anymore... we just don't know how to handle that situation so we throw an exception.

They try/catch for IOException, but it should be clear what is its purpose. If we make UnexpectedResponseException extend IOException, then they will have to check the type of IOException in every catch and make sure it's not actually a valid, but unexpected response. Makes sense now? :D

llorllale · 2018-03-19T17:38:45Z

src/main/java/com/amihaiemil/docker/RtContainer.java

+            info = Json
+                .createReader(response.getEntity().getContent()).readObject();
+        } else {
+            info = null;


@amihaiemil why are we returning null? Why not throw an error? Or why not have the return type be Optional<JsonObject>?

I really think we should throw an error. At the very least, not return null in any case.

llorllale · 2018-03-19T17:43:33Z

src/test/java/com/amihaiemil/docker/RtContainerTestCase.java

+     * RtContainer throws ISE if it cannot start.
+     * @throws Exception If something goes wrong.
+     */
+    @Test(expected = IllegalStateException.class)


@amihaiemil RtContainer.start() missing tests on receiving 404 and 304 from the docker API

llorllale · 2018-03-19T17:47:59Z

@amihaiemil left a couple more comments

Makes sense now?

I understood you from the start, we just happen to disagree, but that's OK.

amihaiemil · 2018-03-19T18:11:20Z

@llorllale fixed the null; throws ISE same as start(), for now. I also added more tests for those statuses :)

llorllale · 2018-03-19T18:16:45Z

src/test/java/com/amihaiemil/docker/RtContainerTestCase.java

+     * @throws Exception If something goes wrong.
+     */
+    @Test(expected = IllegalStateException.class)
+    public void startsWithServerError() throws Exception {


@amihaiemil can you remove the Conditions on the startsWith*Error tests in order to remove some noise? Those conditions are already tested for in the startsOk test.

@llorllale haha, you're good at this. Sure :)

llorllale · 2018-03-19T18:17:36Z

@amihaiemil just one more minor comment and we're golden

amihaiemil · 2018-03-19T18:24:13Z

@rultor merge it

rultor · 2018-03-19T18:25:06Z

@rultor merge it

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

rultor · 2018-03-19T18:27:09Z

@rultor merge it

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

0crat · 2018-03-19T18:41:29Z

Order was finished: +15 points just awarded to @llorllale /z

0crat · 2018-03-19T18:41:31Z

The job #56 is now out of scope

0crat · 2018-03-19T18:41:36Z

Payment to ARC for a closed pull request, as in §28: +10 points just awarded to @amihaiemil /z

amihaiemil added 2 commits March 18, 2018 20:15

inspect and start + unit tests

d71b7a5

puzzle

8ee6558

llorllale reviewed Mar 19, 2018

View reviewed changes

CR fixes and puzzle

fb214cb

llorllale reviewed Mar 19, 2018

View reviewed changes

more tests

5582f92

llorllale reviewed Mar 19, 2018

View reviewed changes

removed noise

ca2e586

rultor merged commit ca2e586 into master Mar 19, 2018

amihaiemil deleted the 46 branch March 19, 2018 18:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Container inspect and start + unit tests #56

Container inspect and start + unit tests #56

amihaiemil commented Mar 18, 2018

0crat commented Mar 18, 2018

coveralls commented Mar 18, 2018 •

edited

amihaiemil commented Mar 18, 2018

0crat commented Mar 18, 2018

0crat commented Mar 18, 2018

amihaiemil commented Mar 18, 2018

llorllale Mar 19, 2018

amihaiemil Mar 19, 2018

llorllale Mar 19, 2018

llorllale left a comment

llorllale Mar 19, 2018

llorllale Mar 19, 2018

llorllale Mar 19, 2018

amihaiemil commented Mar 19, 2018

llorllale commented Mar 19, 2018

amihaiemil commented Mar 19, 2018

llorllale Mar 19, 2018

llorllale Mar 19, 2018

llorllale commented Mar 19, 2018

amihaiemil commented Mar 19, 2018

llorllale Mar 19, 2018

amihaiemil Mar 19, 2018

llorllale commented Mar 19, 2018

amihaiemil commented Mar 19, 2018

rultor commented Mar 19, 2018

rultor commented Mar 19, 2018

0crat commented Mar 19, 2018

0crat commented Mar 19, 2018

0crat commented Mar 19, 2018

Container inspect and start + unit tests #56

Container inspect and start + unit tests #56

Conversation

amihaiemil commented Mar 18, 2018

0crat commented Mar 18, 2018

coveralls commented Mar 18, 2018 • edited

Pull Request Test Coverage Report for Build 84

💛 - Coveralls

amihaiemil commented Mar 18, 2018

0crat commented Mar 18, 2018

0crat commented Mar 18, 2018

amihaiemil commented Mar 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llorllale 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

amihaiemil commented Mar 19, 2018

llorllale commented Mar 19, 2018

amihaiemil commented Mar 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llorllale commented Mar 19, 2018

amihaiemil commented Mar 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llorllale commented Mar 19, 2018

amihaiemil commented Mar 19, 2018

rultor commented Mar 19, 2018

rultor commented Mar 19, 2018

0crat commented Mar 19, 2018

0crat commented Mar 19, 2018

0crat commented Mar 19, 2018

coveralls commented Mar 18, 2018 •

edited