Ensure all integration daemon logging happens before test exit#39197
Merged
thaJeztah merged 1 commit intomoby:masterfrom May 11, 2019
Merged
Ensure all integration daemon logging happens before test exit#39197thaJeztah merged 1 commit intomoby:masterfrom
thaJeztah merged 1 commit intomoby:masterfrom
Conversation
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>
Codecov Report
@@ Coverage Diff @@
## master #39197 +/- ##
=========================================
Coverage ? 37.05%
=========================================
Files ? 612
Lines ? 45448
Branches ? 0
=========================================
Hits ? 16842
Misses ? 26321
Partials ? 2285 |
Member
|
Thanks!! |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
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.Signed-off-by: Daniel Sweet danieljsweet@icloud.com
- What I did
Ensured
internal/test/daemonlogged exit before the controlling test completed.- How I did it
Logged daemon process completion before sending result of
d.cmd.Wait()to thed.Waitchannel.- How to verify it
Run
make test | catseveral times; ideally not foregrounded, ideally on a slow system, ideally in a VM; this would eventually fail for me after 3 repeated attempts.- Description for the changelog
Always log daemon exit before each integration test finishes