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: Fix TestConcurrentLogging race test #43666

Merged
merged 1 commit into from May 31, 2022

Conversation

vvoland
Copy link
Contributor

@vvoland vvoland commented May 31, 2022

Follow up to:

The recent fix for #43647 changed the
signature of the NewLogFile and WriteLogEntry functions. Due to the
logfile_race_test having a race tag, my editor's language server didn't
process this file, so I didn't notice that it was broken. The CI also doesn't
seem to run tests with the -race flag, so it didn't fail on it either.

This made the logfile_race_test.go not possible to build.

Build error
# github.com/docker/docker/daemon/logger/loggerutils [github.com/docker/docker/daemon/logger/loggerutils.test]
./logfile_race_test.go:48:30: too many arguments in call to NewLogFile
        have (string, number, number, bool, func(*logger.Message) ([]byte, error), func(io.Reader) Decoder, number, func(context.Context, SizeReaderAt, int) (io.Reader, int, error))
        want (string, int64, int, bool, MakeDecoderFn, fs.FileMode, GetTailReaderFunc)
./logfile_race_test.go:73:38: not enough arguments in call to logfile.WriteLogEntry
        have (*logger.Message)
        want (time.Time, []byte)
FAIL    github.com/docker/docker/daemon/logger/loggerutils [build failed]
FAIL

- What I did
Fix the build errors in logfile_race_test.go

- How I did it
Adjust to the new LogFile API

- How to verify it

cd daemon/logger/loggerutils
go test -race -v -run TestConcurrentLogging

- Description for the changelog

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

The recent fix for log corruption changed the signature of the
NewLogFile and WriteLogEntry functions and the test wasn't adjusted to
this change.

Fix the test by adjusting to the new LogFile API.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
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

@thaJeztah
Copy link
Member

@corhere PTAL

@thaJeztah
Copy link
Member

Two failures on Jenkins (Windows RS5)

Known flaky: #41561

=== RUN   TestRenameAnonymousContainer
    main_test.go:32: assertion failed: error is not nil: Error response from daemon: Could not kill running container 2823212c3a6e3c5cb62fab7f1749e56b3b9c143820d4d81d76ddbeb6b14a592f, cannot remove - tried to kill container, but did not receive an exit event: failed to remove 2823212c3a6e3c5cb62fab7f1749e56b3b9c143820d4d81d76ddbeb6b14a592f
--- FAIL: TestRenameAnonymousContainer (46.96s)

This one is not known to be flaky, but is unrelated (passed on GitHub actions)

=== RUN   TestKillContainer/killing_signal
    kill_test.go:73: timeout hit after 10s: waiting for container to be one of (exited), currently running
    --- FAIL: TestKillContainer/killing_signal (13.08s)

@thaJeztah thaJeztah merged commit bc541ba into moby:master May 31, 2022
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