Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix TestSwarmClusterRotateUnlockKey #39616

Merged
merged 1 commit into from
Jul 26, 2019

Conversation

dperny
Copy link
Contributor

@dperny dperny commented Jul 26, 2019

TestSwarmClusterRotateUnlockKey had been identified as a flaky test. It turns out that the test code was wrong: where we should have been checking the string output of a command, we were instead checking the value of the error. This means that the error case we were expecting was not being matched, and the test was failing when it should have just retried.

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.

heh 😂 that was the issue all this time? LGTM

Improving CI automation moved this from In progress to Reviewer approved Jul 26, 2019
@tonistiigi
Copy link
Member

that was the issue all this time

@thaJeztah No, this retry for allowing this error (that shouldn't really be allowed to happen) was added last week

@dperny There's an identical check in TestSwarmRotateUnlockKey

@thaJeztah
Copy link
Member

Ah, I see; added in 52e0dfe #39531

Copy link
Member

@andrewhsu andrewhsu left a comment

Choose a reason for hiding this comment

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

LGTM

@andrewhsu
Copy link
Member

@dperny this PR looks good to go, but could you also check the other retry logic added in #39531.

Also, could we log the retry attempts in the test? If we're expecting retry to be a thing that a user would expect to experience, would be good to see if retry was tried.

@thaJeztah
Copy link
Member

Is it worth logging the first 5 attempts if we know converging takes some time? (it already logs a failure if 5 attempts failed)

Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

Improving CI automation moved this from Reviewer approved to Review in progress Jul 26, 2019
TestSwarmClusterRotateUnlockKey had been identified as a flaky test. It
turns out that the test code was wrong: where we should have been
checking the string output of a command, we were instead checking the
value of the error. This means that the error case we were expecting was
not being matched, and the test was failing when it should have just
retried.

Signed-off-by: Drew Erny <drew.erny@docker.com>
@dperny dperny force-pushed the fix-cluster-rotate-unlock-key branch from db55ead to b79adac Compare July 26, 2019 20:52
@dperny
Copy link
Contributor Author

dperny commented Jul 26, 2019

Fixed the other test in the same way.

Improving CI automation moved this from Review in progress to Reviewer approved Jul 26, 2019
@thaJeztah
Copy link
Member

Looks like this test is still flaky #39883 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Improving CI
  
Done
Development

Successfully merging this pull request may close these issues.

Flaky test: DockerSwarmSuite.TestSwarmClusterRotateUnlockKey
6 participants