DockerSuite.TestAttachDetach: increase timeout#37711
Closed
kolyshkin wants to merge 1 commit intomoby:masterfrom
Closed
DockerSuite.TestAttachDetach: increase timeout#37711kolyshkin wants to merge 1 commit intomoby:masterfrom
kolyshkin wants to merge 1 commit intomoby:masterfrom
Conversation
The following failure was observed on power CI:
> FAIL: docker_cli_attach_unix_test.go:127: DockerSuite.TestAttachDetach
>
> docker_cli_attach_unix_test.go:174:
> c.Fatal("timed out waiting for container to exit")
> ... Error: timed out waiting for container to exit
Allegedly it is caused by a very small timeout of 10ms,
and a general slowness of power today. Also note that
the timeout used to be 500ms before commit ae0883c
("Move TestAttachDetach to integration-cli").
Increase the timeout to 100ms to avoid false failures.
Signed-off-by: Kir Kolyshkin <[email protected]>
arm64b
reviewed
Aug 24, 2018
| select { | ||
| case <-ch: | ||
| case <-time.After(10 * time.Millisecond): | ||
| case <-time.After(100 * time.Millisecond): |
Contributor
There was a problem hiding this comment.
Ah, I remember that I tweak the timeout value to avoid some false positive test failure, but some concerns raised to worry about this behavior probably paper cover some potential issue, e.g, performance 😄 .
Contributor
Author
There was a problem hiding this comment.
This is interesting. I checked git history, the only meaningful change to this test was done in commit ae0883c when it was moved to integration-cli.
The original test (before the above commit) was
- ...
- sending escape sequence to "attach";
- checking that "attach" is exiting (after an escape sequence is sent);
- checking that the container is still alive;
- kills the containers.
Also, timeouts were big at that time :)
The new test (after the commit):
- sends the escape sequence to "attach";
- checks that the container is running;
- kills the container;
- checks that the "attach" has exited.
It seems that the order is very broken. I'll send another PR.
Contributor
Author
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
The following failure was observed on power CI:
Allegedly it is caused by a very small timeout of 10ms,
and a general slowness of power today. Also note that
the timeout used to be 500ms before commit ae0883c
("Move TestAttachDetach to integration-cli").
Increase the timeout to 100ms to avoid false failures.