fix errors in tests by fbuecklers · Pull Request #938 · docker-java/docker-java

fbuecklers · 2017-10-24T22:29:43Z

Try to fix some outstanding test failures

This change is

KostyaSha · 2017-10-24T23:02:02Z

src/test/java/com/github/dockerjava/cmd/PushImageCmdIT.java

        } else {
            exception.expect(NotFoundException.class);
-        }
+        }*/


you will have different errors for different api versions, i spent a lot of time in this place

Yes true, but it seems that only swarm behaves differently here compared to docker. The main error here was that awaitCompletion and not awaitSuccess was called.
The error handling for the pull command was only in awaitSuccess. I have moved that to awaitCompletion since you will expect correct error handling there.

fbuecklers · 2017-10-25T16:44:40Z

Hm..., I really don't like it that all test cases leave there containers and resources there.
It will be really more clean if all test classes cleanups there created resources like images/containers/networks/volumes etc.
Currently, the potential is relatively high that one test conflicts with a previously executed test. Because a name/port/volume etc. already existst.

codecov-io · 2017-10-25T17:03:01Z

Codecov Report

Merging #938 into master will increase coverage by 0.29%.
The diff coverage is 78.84%.

@@            Coverage Diff             @@
##           master     #938      +/-   ##
==========================================
+ Coverage   61.29%   61.58%   +0.29%     
==========================================
  Files         411      411              
  Lines        8134     8172      +38     
  Branches      526      530       +4     
==========================================
+ Hits         4986     5033      +47     
+ Misses       2846     2838       -8     
+ Partials      302      301       -1

Impacted Files	Coverage Δ
...b/dockerjava/api/command/InspectImageResponse.java	`100% <ø> (ø)`	⬆️
...com/github/dockerjava/netty/InvocationBuilder.java	`90.86% <100%> (ø)`	⬆️
...ckerjava/core/command/PushImageResultCallback.java	`76.47% <66.66%> (+16.47%)`	⬆️
.../dockerjava/core/async/ResultCallbackTemplate.java	`76.74% <66.66%> (ø)`	⬆️
...ckerjava/core/command/PullImageResultCallback.java	`80.76% <80.95%> (+24.51%)`	⬆️
...ava/netty/handler/FramedResponseStreamHandler.java	`84.48% <0%> (ø)`	⬆️
...hub/dockerjava/core/async/JsonStreamProcessor.java	`80% <0%> (+3.33%)`	⬆️
...com/github/dockerjava/api/model/ContainerPort.java	`6.25% <0%> (+6.25%)`	⬆️
.../dockerjava/core/command/CreateNetworkCmdImpl.java	`74.19% <0%> (+6.45%)`	⬆️
.../github/dockerjava/api/model/PullResponseItem.java	`62.5% <0%> (+12.5%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fce9df9...7e24a13. Read the comment docs.

# Conflicts: # src/main/java/com/github/dockerjava/api/model/PullResponseItem.java # src/main/java/com/github/dockerjava/core/command/PullImageResultCallback.java

KostyaSha · 2017-10-25T18:33:06Z

.travis/travis-before-install.sh

-        "--name=swarm_join" \
-        "swarm:${SWARM_VERSION}" \
-        join --advertise="${HOST_IP}:${HOST_PORT}" --delay="0s" --heartbeat "5s" "nodes://${HOST_IP}:${HOST_PORT}"
+    # join engine to swarm, is not required if fixed ip addresses are used


because then you will have huge default delays

KostyaSha · 2017-10-25T18:34:52Z

src/test/java/com/github/dockerjava/cmd/CommitCmdIT.java

        LOG.info("Committing container: {}", container.toString());
        String imageId = dockerRule.getClient().commitCmd(container.getId()).exec();

+        //swarm needs some time to refelct new images


KostyaSha · 2017-10-25T18:35:13Z

src/test/java/com/github/dockerjava/cmd/CommitCmdIT.java

        String imageId = dockerRule.getClient().commitCmd(container.getId()).exec();

+        //swarm needs some time to refelct new images
+        wait(5000);


use awaitility with query to what needed?

What do you exactly mean?

I mean that awaitility with client query for some good to go criteria is right way. https://github.com/awaitility/awaitility

KostyaSha · 2017-10-25T18:38:16Z

src/test/java/com/github/dockerjava/cmd/CreateContainerCmdIT.java


    @Test
    public void createContainerWithPortBindings() throws DockerException {
+        int baseport = dockerRule.getKind().equals("jersey")? 11000: 12000;


getKind() is string value, test itself is parametrized with enum, so you can do it easy with getFactoryType() == JERSEY

KostyaSha · 2017-10-25T18:39:35Z

src/test/java/com/github/dockerjava/cmd/PushImageCmdIT.java


        // we have to block until image is pushed
-        dockerRule.getClient().pushImageCmd(username + "/busybox").exec(new PushImageResultCallback()).awaitSuccess();
+        dockerRule.getClient().pushImageCmd(username + "/busybox").exec(new PushImageResultCallback()).awaitCompletion();


it may complete with not pushed r tests needs awaitSuccess() everywhere as clear success of operation.

I think that it is not a good choice to have an awaitComplete and an awaitSuccess. Since awaitComplete does not validate all error cases on pull and push

Then it will hide bugs in .awaitSuccess logic. In any case success should be success for test even the next code do single checks of success criteria.

KostyaSha · 2017-10-25T18:40:30Z

src/test/java/com/github/dockerjava/cmd/PushImageCmdIT.java

        dockerRule.getClient().pushImageCmd(username + "/xxx")
                .exec(new PushImageResultCallback())
-                .awaitCompletion(20, TimeUnit.SECONDS); // exclude infinite await sleep
+                .awaitCompletion(); // exclude infinite await sleep


comment is wrong

without timeout test may stuck forever, test needs some timeout.

KostyaSha · 2017-10-25T18:41:16Z

src/test/java/com/github/dockerjava/cmd/PushImageCmdIT.java

-            exception.expect(DockerClientException.class);
-        } else {
+        //swarms throws a different error here
+        if (TestUtils.isSwarm(dockerRule.getClient())) {


it not related to swarm. Run locally 2 DINDs with different version and run test against - you will see different behaviour

The Problem was that sometimes awaitSuccess and sometimes awaitComplete was used. And they behaves inconsistent while handling the error.
Therefore i propose that awaitComplete does the correct error handling and we deprecate awaitSuccess.

i not sure at all why we parse messages because http response code should indicate success or failure... But it could be changed in future, now i want end swarm & tests scope of changes and make major release.

Not really since docker send always an success status code and will later then indicate the error status in the body. That is not so unknown pattern for long running asynchronous operations, since a http request will otherwise be timeout by clients or intermediate proxies.

fbuecklers · 2017-10-25T20:36:32Z

Yeah all seems to be green
What dou you think about the changes?

KostyaSha · 2017-10-25T20:44:50Z

src/test/java/com/github/dockerjava/cmd/CommitCmdIT.java

        dockerRule.getClient().startContainerCmd(container.getId()).exec();

+        Integer status = dockerRule.getClient().waitContainerCmd(container.getId())
+                .exec(new WaitContainerResultCallback()).awaitStatusCode();


on one line client.command()
on other lines fluent calls .exec()
.await()
That's for eyes redability

KostyaSha · 2017-10-25T20:45:17Z

src/test/java/com/github/dockerjava/cmd/LoadImageCmdIT.java


+        //swarm needs some time to refelct new images
+        synchronized (this) {
+            wait(5000);


what is the criteria that it swarm is ready?

I don't know but we use the same trick in our production swarm cluster. I guess that swarm manager keeps an image cache in memory or something like that.
And this cache is lazy reinitialized after a new image is in place.

KostyaSha · 2017-10-25T20:46:36Z

src/test/java/com/github/dockerjava/cmd/PushImageCmdIT.java

+                    .exec(new PushImageResultCallback())
+                    .awaitCompletion(30, TimeUnit.SECONDS);
+
+            Assert.fail("An exception is expected.");


Assert.fail() is not junit Expected Rule style

Yes sure is there a different easy way to define two different expectations?

fast lookup shows that ExpectedException.expect(Class) uses org.hamcrest.CoreMatchers#instanceOf and there is org.junit.rules.ExpectedException#expect(org.hamcrest.Matcher<?>)
so i guess you can construct org.hamcrest.CoreMatchers#anyOf(java.lang.Iterable<org.hamcrest.Matcher<? super T>>) + instanceOf()

But again, you are changing the existing behaviour of the test. Test was about success + different api versions, so you shouldn't combine and hide this errors.
This tests not only about green status, they like documentation examples. Usually when i have questions i go to tests and this conditionals will show what may be wrong.

That will make only sense for me if I can always expect the same exception for the same error. But that is not the case. If I upgrade my docker version, for example, I will get no a different Exception. Thats not a really cool behavior. And since docker behaves like that, it should be more straightforward to throw an exception that I can always handle. For instance a common DockerException base class for all Exception which the docker client will throw.

KostyaSha · 2017-10-25T20:47:46Z

src/test/java/com/github/dockerjava/cmd/PullImageCmdIT.java

+                    .awaitCompletion(30, TimeUnit.SECONDS);
+
+            fail("An exception is expected.");
+        } catch (NotFoundException | DockerClientException e) {


that seems a wrong junit exception usage. test shouldn't suppress exceptions, it need only configure exception (that is Rule).

https://github.com/junit-team/junit4/wiki/Exception-testing#expectedexception-rule

But thats an or exception condition
maybe we let DockerClientException extend DockerException?

no idea, how it influence on this test case? Different docker versions return different error codes so client returns different exceptions. That's why Exception Rule was conditionally configured before call that should throw error.

It will change that each method will at least throw a DockerException. So you can always expect a DockerException regardless which docker version you exactly use.
Currently you must expect a DockerException or a DockerClientException.

KostyaSha · 2017-10-27T23:42:18Z

src/test/java/com/github/dockerjava/cmd/PullImageCmdIT.java

-            exception.expect(DockerClientException.class);
-        }
+        //different docker version throws different errors here
+        exception.expect(anyOf(instanceOf(NotFoundException.class), instanceOf(DockerClientException.class)));


Please restore original logic, original code tests for expected exceptions, not or.

There was a bug, in the error handling logic it will always throw an error now. But in my tests docker behaves different also on same versions by just switching to swarm. And again I do not like that, that you must handle different exception types depending on the used docker/swarm version.
If we at least let DockerClientException extend DockerException we can say it will always throw an DockerException independent of the used docker / swarm version.

library is transparent to docker api/daemon bugs. We don't do magic, NotFoundException is handled in different way by library users because it means that image not found and user may be wanting to switch to other image, all other errors - 500, 501 etc are not recoverable for application usage. And if swarm and docker behaves differently then it should be library user problem. In fact i don't see a problem, in library you can catch it with oneEx | secondEx and tests catches exactly like it is.

The case why I do not like this behavior is, that the docker daemon indicates an error asynchronously and we make a docker client exception out of it.
My expectation was that the DockerClientException is only thrown if a client-side error occurred, not a remote one.

KostyaSha · 2017-10-27T23:42:49Z

src/test/java/com/github/dockerjava/cmd/PushImageCmdIT.java

                .exec(new PushImageResultCallback())
-                .awaitCompletion(20, TimeUnit.SECONDS); // exclude infinite await sleep
-
+                .awaitCompletion(30, TimeUnit.SECONDS);


restore comment

KostyaSha · 2017-10-27T23:43:52Z

src/test/java/com/github/dockerjava/cmd/CommitCmdIT.java

+        //swarm will attach additional labels here
+        assertThat(responseLabels.size(), greaterThanOrEqualTo(2));
+        assertThat(responseLabels.get("label1"), equalTo("abc"));
+        assertThat(responseLabels.get("label2"), equalTo("123"));


but why it work before?

It works before because, on some docker versions, the labels were copied to the containerConfig. That was the reason why this test fails on some test configurations. We can only expect the labels on the config object since the container was not started with any labels.

KostyaSha · 2017-10-31T20:58:15Z

Thanks!

hawky-4s- and others added 2 commits October 9, 2015 21:54

Improve PullImageCmd for pulling an image through docker swarm.

a39e52d

fix errors in tests

8d3c4f8

KostyaSha reviewed Oct 24, 2017

View reviewed changes

fix errors in pull tests will also fix docker-java#939

a028b16

fbuecklers force-pushed the test-fixes branch from 15dfe16 to a028b16 Compare October 25, 2017 14:02

Fix more concurrent test issues

fb1dc88

fbuecklers added 2 commits October 25, 2017 19:18

Some more swarm fixes

b176cd2

Merge remote-tracking branch 'swarm-pull/swarm-pull' into test-fixes

946d2a8

# Conflicts: # src/main/java/com/github/dockerjava/api/model/PullResponseItem.java # src/main/java/com/github/dockerjava/core/command/PullImageResultCallback.java

KostyaSha reviewed Oct 25, 2017

View reviewed changes

fbuecklers added 2 commits October 25, 2017 20:49

Fix pull handling for swarm

681309f

More fixes

477b9fc

KostyaSha reviewed Oct 25, 2017

View reviewed changes

Suggest test code fixes

798117f

KostyaSha reviewed Oct 27, 2017

View reviewed changes

Restore original pull/push expected exception logic

7e24a13

KostyaSha merged commit 8902e32 into docker-java:master Oct 31, 2017

KostyaSha added the area/swarm-classic label Oct 31, 2017

KostyaSha added this to the 3.1.0 milestone Oct 31, 2017

fbuecklers deleted the test-fixes branch November 21, 2017 11:45

Conversation

fbuecklers commented Oct 24, 2017 • edited by marcuslinke Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fbuecklers Oct 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fbuecklers commented Oct 25, 2017

Uh oh!

codecov-io commented Oct 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fbuecklers commented Oct 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KostyaSha Oct 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KostyaSha Oct 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fbuecklers commented Oct 24, 2017 •

edited by marcuslinke

Loading

fbuecklers Oct 25, 2017 •

edited

Loading

codecov-io commented Oct 25, 2017 •

edited

Loading

KostyaSha Oct 25, 2017 •

edited

Loading

KostyaSha Oct 25, 2017 •

edited

Loading

fbuecklers Oct 31, 2017 •

edited

Loading

fbuecklers Oct 28, 2017 •

edited

Loading