Skip to content

integration-cli: fix TestAttachDetach, rm TestAttachDetachTruncatedID#37718

Merged
thaJeztah merged 1 commit intomoby:masterfrom
kolyshkin:test-attach-detach
Aug 27, 2018
Merged

integration-cli: fix TestAttachDetach, rm TestAttachDetachTruncatedID#37718
thaJeztah merged 1 commit intomoby:masterfrom
kolyshkin:test-attach-detach

Conversation

@kolyshkin
Copy link
Contributor

(this is a continuation of #37711)

It looks like the logic of the test became wrong after commit ae0883c (PR #12292).

The original logic was:

  • (a few first steps skipped for clarity)
  • send escape sequence to "attach";
  • check "attach" is exiting (i.e. escape sequence works);
  • check the container is still alive;
  • kill the container.

Also, timeouts were big at that time, in the order of seconds.

The logic after the above mentioned commit and until now is:

  • ...
  • send escape sequence to "attach";
  • check the container is running (why shouldn't it?);
  • kill the container;
  • checks that the "attach" has exited.

So, from the "let's check detach using escape sequence is working"
the test became something like "let's check that attach is gone
once we kill the container".

Let's fix the above test, also increasing the timeout waiting
for attach to exit (which fails from time to time on power CI).

Now, the second test, TestAttachDetachTruncatedID, does the exact
same thing, except it uses a truncated container ID. It does not
seem to be of much value, so let's remove it.

It looks like the logic of the test became wrong after commit
ae0883c ("Move TestAttachDetach to integration-cli").

The original logic was:
* (a few first steps skipped for clarity)
* send escape sequence to "attach";
* check "attach" is exiting (i.e. escape sequence works);
* check the container is still alive;
* kill the container.

Also, timeouts were big at that time, in the order of seconds.

The logic after the above mentioned commit and until now is:
* ...
* send escape sequence to "attach";
* check the container is running (why shouldn't it?);
* kill the container;
* checks that the "attach" has exited.

So, from the "let's check detach using escape sequence is working"
the test became something like "let's check that attach is gone
once we kill the container".

Let's fix the above test, also increasing the timeout waiting
for attach to exit (which fails from time to time on power CI).

Now, the second test, TestAttachDetachTruncatedID, does the exact
same thing, except it uses a truncated container ID. It does not
seem to be of much value, so let's remove it.

Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin
Copy link
Contributor Author

@arm64b PTAL

@codecov
Copy link

codecov bot commented Aug 25, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@0d9d861). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #37718   +/-   ##
=========================================
  Coverage          ?   36.04%           
=========================================
  Files             ?      609           
  Lines             ?    45063           
  Branches          ?        0           
=========================================
  Hits              ?    16242           
  Misses            ?    26592           
  Partials          ?     2229

Copy link
Contributor

@arm64b arm64b left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐯

@vdemeester vdemeester requested a review from cpuguy83 August 27, 2018 11:24
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants