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

daemon/logger: refactor followLogs and replace flaky TestFollowLogsHandleDecodeErr #43105

Merged
merged 2 commits into from
Jan 12, 2022

Conversation

kzys
Copy link
Member

@kzys kzys commented Dec 24, 2021

- What I did

This PR refactors followLogs and replaces flaky TestFollowLogsHandleDecodeErr by adding a small more focused test.

- How I did it

The first commit is refactoring. The second commit is removing the flaky test and adding a new one.

- How to verify it

- Description for the changelog

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

(from https://www.bbc.co.uk/programmes/p05rkdmp)

followLogs() is getting really long (170+ lines) and complex.
The function has multiple inner functions that mutate its variables.

To refactor the function, this change introduces follow{} struct.
The inner functions are now defined as ordinal methods, which are
accessible from tests.

Signed-off-by: Kazuyoshi Kato <katokazu@amazon.com>
Signed-off-by: Kazuyoshi Kato <katokazu@amazon.com>
@thaJeztah
Copy link
Member

Failure on windows looks like a flaky test; #38521 (I'll kick ci)

 === FAIL: github.com/docker/docker/integration-cli TestDockerSuite/TestStartReturnCorrectExitCode (4.97s)
[2021-12-27T18:43:44.681Z]     docker_cli_start_test.go:209: assertion failed: expected an error, got nil
[2021-12-27T18:43:44.681Z]     --- FAIL: TestDockerSuite/TestStartReturnCorrectExitCode (4.97s)

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.

@thaJeztah thaJeztah added this to the 21.xx milestone Jan 5, 2022
@thaJeztah
Copy link
Member

(added cherry-pick label, as this is a follow-up / fix for changes in #43043)

@thaJeztah
Copy link
Member

@cpuguy83 ptal 🤗

Copy link
Member

@cpuguy83 cpuguy83 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.

None yet

3 participants