Skip to content

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

Merged
cpuguy83 merged 2 commits intomoby:masterfrom
kzys:follow-struct
Jan 12, 2022
Merged

daemon/logger: refactor followLogs and replace flaky TestFollowLogsHandleDecodeErr#43105
cpuguy83 merged 2 commits intomoby:masterfrom
kzys:follow-struct

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)

Kazuyoshi Kato added 2 commits December 27, 2021 09:11
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 <[email protected]>
@kzys kzys marked this pull request as ready for review December 27, 2021 17:12
@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.

3 participants