migrate service update integration tests from integration-cli to integration/service package by adshmh · Pull Request #37564 · moby/moby

adshmh · 2018-07-30T20:59:10Z

This is a follow up PR on #37492 (closed and reverted due to 2 flaky tests). This PR attempts to fix that by polling the update state of the service.

- What I did
Refactored and moved service update integration tests from the integration-cli package to integration/service

- How I did it
Removed the integration test file integration-cli//docker_cli_service_update_test.go and added new test file integration/service/update_test.go with corresponding tests.

- How to verify it

- Description for the changelog
Service update integration tests have been moved from integration-cli to integration/service package.

- A picture of a cute animal (not mandatory but encouraged)

boaz0

LGTM 💯

boaz0 · 2018-07-31T06:35:56Z

CI failed on TestServiceUpdateConfigs and TestServiceUpdateSecrets which are related to this change. 😞

adshmh · 2018-07-31T13:48:51Z

Thank you for the reviews, I will look into failures.

vdemeester

LGTM 🐯

vdemeester · 2018-09-12T13:34:14Z

@adshmh needs a rebase though 👼

AkihiroSuda

LGTM but needs rebase

yongtang

LGTM. @adshmh Can you do a rebase?

adshmh · 2018-10-15T15:36:09Z

@yongtang thank you for the review. I think 2 of the tests are flaky and it is not clear why yet, so I am looking into the cause. So far the test failures have not happened on my local builds over several re-runs.

olljanat · 2018-12-21T17:51:18Z

@adshmh this one needs to be rebased

thaJeztah · 2018-12-24T19:25:36Z

Conflict is due to 83363fb#diff-dc96b4992b77c3e1cd181b2fd9162b5eR17 (#37635) which made changes to integration-cli/docker_cli_service_update_test.go (which is being removed in this PR)

…rvice_update_test.go to integration/service Signed-off-by: Arash Deshmeh <[email protected]> Signed-off-by: Sebastiaan van Stijn <[email protected]>

codecov · 2018-12-24T19:32:13Z

Codecov Report

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

@@            Coverage Diff            @@
##             master   #37564   +/-   ##
=========================================
  Coverage          ?   35.62%           
=========================================
  Files             ?      610           
  Lines             ?    44896           
  Branches          ?        0           
=========================================
  Hits              ?    15992           
  Misses            ?    26697           
  Partials          ?     2207

thaJeztah

rebased; changes LGTM

let's merge on green 👍

olljanat · 2018-12-26T09:51:09Z

z looks to be failed to:

07:40:41 --- FAIL: TestServiceUpdateConfigs (2.90s)
07:40:41     daemon.go:296: [d954a5785d31c] waiting for daemon to start
07:40:41     daemon.go:328: [d954a5785d31c] daemon started
07:40:41     update_test.go:188: assertion failed: error is not nil: Error response from daemon: rpc error: code = Unknown desc = update out of sequence
07:40:41     daemon.go:283: [d954a5785d31c] exiting daemon
07:40:41 FAIL
07:40:41 ---> Making bundle: .integration-daemon-stop (in bundles/test-integration)
07:40:44 Clearing AppArmor profiles cache:.

which I have not seen earlier but let's see if rebuild helps

thaJeztah · 2018-12-26T10:48:20Z

Same failure again; odd 🤔

adshmh · 2018-12-28T15:11:37Z

Sorry for the delay in response. The service update tests still seem flaky, even though I have added a polling function to check on the status of the service. This time, TestServiceUpdateConfigs passed, but TestServiceUpdateSecrets failed, both of which use the same polling function to verify service status, before issuing subsequent update calls.

Currently looking at the service update command on docker CLI repo, as well as swarmkit repo, to find out if there are any other conditions to check after a service update API call before issuing the next one.

olljanat · 2018-12-28T15:17:25Z

These have not been flaky earlier. Maybe some temporary issue on server. Let's try once more...

AkihiroSuda · 2018-12-31T18:53:55Z

ready to merge?

olljanat · 2018-12-31T19:09:43Z

@AkihiroSuda afaik, yes.
@thaJeztah said week ago

let's merge on green +1

adshmh requested a review from vdemeester as a code owner July 30, 2018 20:59

GordonTheTurtle added the status/0-triage label Jul 30, 2018

boaz0 reviewed Jul 31, 2018

View reviewed changes

vdemeester added status/2-code-review and removed status/0-triage labels Jul 31, 2018

GordonTheTurtle assigned duglin Aug 14, 2018

vdemeester requested review from AkihiroSuda, thaJeztah and yongtang September 12, 2018 13:33

vdemeester approved these changes Sep 12, 2018

View reviewed changes

AkihiroSuda approved these changes Sep 18, 2018

View reviewed changes

yongtang approved these changes Oct 15, 2018

View reviewed changes

derek bot added the status/failing-ci Indicates that the PR in its current state fails the test suite label Dec 22, 2018

thaJeztah added the area/testing label Dec 24, 2018

migrated service integration tests from integration-cli/docker_cli_se…

be151a7

…rvice_update_test.go to integration/service Signed-off-by: Arash Deshmeh <[email protected]> Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah force-pushed the migrate-docker_cli_service_update_test-to-integration-service-fixed-flake branch from 2d0e475 to be151a7 Compare December 24, 2018 19:32

thaJeztah approved these changes Dec 24, 2018

View reviewed changes

thaJeztah added status/4-merge and removed status/2-code-review status/failing-ci Indicates that the PR in its current state fails the test suite labels Dec 24, 2018

vdemeester added the rebuild/z label Dec 26, 2018

GordonTheTurtle removed the rebuild/z label Dec 26, 2018

derek bot added the rebuild/z label Dec 26, 2018

GordonTheTurtle removed the rebuild/z label Dec 26, 2018

derek bot added the rebuild/z label Dec 28, 2018

GordonTheTurtle removed the rebuild/z label Dec 28, 2018

AkihiroSuda merged commit 27234ff into moby:master Dec 31, 2018

thaJeztah mentioned this pull request Jan 2, 2019

integration: wait for service update to be completed #38474

Closed

thaJeztah mentioned this pull request May 9, 2019

[18.09 backport] backport some integration test changes docker-archive/engine#204

Merged

Conversation

adshmh commented Jul 30, 2018

Uh oh!

boaz0 left a comment

Choose a reason for hiding this comment

Uh oh!

boaz0 commented Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adshmh commented Jul 31, 2018

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

vdemeester commented Sep 12, 2018

Uh oh!

AkihiroSuda left a comment

Choose a reason for hiding this comment

Uh oh!

yongtang left a comment

Choose a reason for hiding this comment

Uh oh!

adshmh commented Oct 15, 2018

Uh oh!

olljanat commented Dec 21, 2018

Uh oh!

thaJeztah commented Dec 24, 2018

Uh oh!

codecov bot commented Dec 24, 2018

Codecov Report

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

olljanat commented Dec 26, 2018 • edited by thaJeztah Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Dec 26, 2018

Uh oh!

adshmh commented Dec 28, 2018

Uh oh!

olljanat commented Dec 28, 2018

Uh oh!

AkihiroSuda commented Dec 31, 2018

Uh oh!

olljanat commented Dec 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

boaz0 commented Jul 31, 2018 •

edited

Loading

olljanat commented Dec 26, 2018 •

edited by thaJeztah

Loading