This repository has been archived by the owner on Jul 16, 2020. It is now read-only.
Unittests: locking fixes and race hunting #437
Merged
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
added 7 commits
August 4, 2016 17:22
A Shutdown() helper is added to the testutil clients and server in order to handle teardown of the SSNTP session and closing the testutil results channels. This should prevent channel writers that are blocked from causing goroutine leaks across test cases. Signed-off-by: Tim Pepper <[email protected]>
The prior commit added a CloseChans() helper, which didn't have explicit coverage. Adding coverage for it points out that it's actually important to not leave the channels closed in unit tests. And then too these helpers do not need exported generally, but do need exported for test. Signed-off-by: Tim Pepper <[email protected]>
The code to put a result on a channel checks to see that something has posted as awaiting a result for the given command/event/error/status before writing to the channel to avoid writing to a non-existing channel. That can still write to a closed channel if the closed channel is not removed from the map. Signed-off-by: Tim Pepper <[email protected]>
The other testutil ssntp actors are all dealt with via references. This should be consistent and insures something isn't inadvertantly acting on a copy of the server state. Signed-off-by: Tim Pepper <[email protected]>
The locking on the testutil results channels was incorrect. The channel writers block and do so with a lock held. That's a major no-no. Defering a lock unlock seems nice a clean...unlock noted immediately after a lock. But then you might block and sleep. To straighten this out, the locks need initialized once at the time the ssntp test actor is created. Only the channel maps get modified inside the channel close/open helpers, and those only do such modificiations with the appropriate lock held. Signed-off-by: Tim Pepper <[email protected]>
Controller still has races, but I think other things are mostly clean. Let's test that theory with data by adding -race to the options for non-controller tests. Signed-off-by: Tim Pepper <[email protected]>
The -race option is useful and should pass through to the 'go test' invocation if specified on the test-cases command line. Signed-off-by: Tim Pepper <[email protected]>
|
Failure rate as of 8/11: 4/30 Failures occurred 100% using go 1.6 (vs. tip). Log of Failures:
https://travis-ci.org/kaccardi/ciao/jobs/151574113
https://travis-ci.org/kaccardi/ciao/jobs/151574946
https://travis-ci.org/kaccardi/ciao/jobs/151574430
|
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.
This PR enables -race in the test-utils helper and toggles it on for non-controller tests in travis (controller has known races). Some observed bugs in testutil's results channels' locking is cleaned up.
Fixes Issue #235