This repository has been archived by the owner on Oct 13, 2023. It is now read-only.
[18.09 backport] Ensure all integration daemon logging happens before test exit #246
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
As of Go 1.12, the `testing` package panics if a goroutine logs to a `testing.T` after the relevant test has completed. This was not documented as a change at all; see the commit 95d06ab6c982f58b127b14a52c3325acf0bd3926 in the Go repository for the relevant change. At any point in the integration tests, tests could panic with the message "Log in goroutine after TEST_FUNCTION has completed". This was exacerbated by less direct logging I/O, e.g. running `make test` with its output piped instead of attached to a TTY. The most common cause of panics was that there was a race condition between an exit logging goroutine and the `StopWithError` method: `StopWithError` could return, causing the calling test method to return, causing the `testing.T` to be marked as finished, before the goroutine could log that the test daemon had exited. The fix is simple: capture the result of `cmd.Wait()`, _then_ log, _then_ send the captured result over the `Wait` channel. This ensures that the message is logged before `StopWithError` can return, blocking the test method so that the target `testing.T` is not marked as finished. Signed-off-by: Daniel Sweet <danieljsweet@icloud.com> (cherry picked from commit 7546322) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
|
@kolyshkin @tiborvass PTAL |
tiborvass
approved these changes
May 28, 2019
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
backport of moby#39197 for 18.09
As of Go 1.12, the
testingpackage panics if a goroutine logs to atesting.Tafter the relevant test has completed. This was notdocumented as a change at all; see the commit
95d06ab6c982f58b127b14a52c3325acf0bd3926 in the Go repository for the
relevant change.
At any point in the integration tests, tests could panic with the
message "Log in goroutine after TEST_FUNCTION has completed". This was
exacerbated by less direct logging I/O, e.g. running
make testwithits output piped instead of attached to a TTY.
The most common cause of panics was that there was a race condition
between an exit logging goroutine and the
StopWithErrormethod:StopWithErrorcould return, causing the calling test method to return,causing the
testing.Tto be marked as finished, before the goroutinecould log that the test daemon had exited. The fix is simple: capture
the result of
cmd.Wait(), then log, then send the capturedresult over the
Waitchannel. This ensures that the message islogged before
StopWithErrorcan return, blocking the test methodso that the target
testing.Tis not marked as finished.