Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Tweaks to fix test races #7376
Conversation
| @@ -13,6 +15,8 @@ import ( | ||
| // MemStore is an in-memory implementation of ClientStore. | ||
| type MemStore struct { | ||
| + mu sync.Mutex |
axw
May 23, 2017
Member
While I'm mostly OK with adding a mutex here, the MemStore wasn't necessarily meant to be goroutine safe. Are the tests doing inappropriate things with it? Can you please link to the race that this fixes?
wallyworld
May 23, 2017
Owner
http://reports.vapour.ws/releases/5260/job/run-unit-tests-race/attempt/2753
RemoveController() and AccountDetails() stomping on c.Accounts
| @@ -103,6 +103,10 @@ type event struct { | ||
| revno int64 | ||
| } | ||
| +// Period is the delay between each sync. | ||
| +// It must not be changed when any watchers are active. |
axw
May 23, 2017
Member
if there was a race, wouldn't it imply that a test is changing Period while a watcher is running? shouldn't we fix that?
wallyworld
May 23, 2017
Owner
The race is because a test patches the Period value. This is done outside the watcher and is normally ok, except when race test are run. So this is the minimal change to allow the many tests which rely on reducing the period to run ok, but also make everything thread safe.
axw
approved these changes
May 23, 2017
LGTM with the PatchValue of Period moved before the watcher is started in TestDowngradeOnMasterWhenOtherControllerDoesntStartUpgrade.
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
wallyworld commentedMay 23, 2017
Description of change
As seen in recent CI test runs, there are test race failures.
This PR fixes the ones I could reproduce locally.
QA steps
go test -race