Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Fixed races in cleaner test #2621
Conversation
dimitern
reviewed
Jun 22, 2015
| @@ -49,6 +49,34 @@ type CheckArgs struct { | ||
| VersionIsZero bool | ||
| } | ||
| +func checkArgs(c *gc.C, args *CheckArgs, facade string, version int, id, method string, inArgs, outResults interface{}) { | ||
| + if args != nil { |
dimitern
Jun 22, 2015
Contributor
How about?
if args == nil {
c.Logf("checkArgs: args is nil!")
return
}
if args.Facade != "" {
...
}
dooferlad
Jun 22, 2015
Contributor
I just moved existing code - didn't want to change the functionality. I agree that moving the if args != nil out of that function and putting it in the caller would make it more obvious that args checking is only done if you pass in expected args. It seems like the original intent was for args = nil to be OK.
dimitern
reviewed
Jun 22, 2015
| +// time it recives a call, as well as check if any of the arguments passed to the APICall() method match | ||
| +// the values given in args (if args itself is not nil, otherwise no arguments are checked). The final | ||
| +// error result of the APICall() will be set to err. | ||
| +func PingingCheckingAPICaller(c *gc.C, args *CheckArgs, called chan struct{}, err error) base.APICaller { |
dimitern
Jun 22, 2015
Contributor
I don't like "Pinging" - it might imply it has something to do with apiserver.Ping(). How about dropping the original CheckingAPICaller and using only this one instead? It shouldn't be too difficult to change existing tests?
dooferlad
Jun 22, 2015
Contributor
It would be a much larger change to switch over the existing tests (api/instancepoller) outside of api/cleaner. The instance poller tests are perfectly served by CheckingAPICaller, so changing them would be a nuisance.
How about CheckingAPICallPoller as a better name? NotifyingCheckingAPICaller? I don't mind overloading ping with yet another implementation myself, but don't have a strong opinion.
|
LGTM with the suggested changes. |
|
I still think 'if args != nil' should be checked first (I wrote it, so I should've done it like this in retrospect :/) to save some indentation. I'm OK with renaming PingingCheckingAPICaller to NotifyingCheckingAPICaller. |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
added a commit
that referenced
this pull request
Jun 22, 2015
dooferlad commentedJun 22, 2015
(Review request: http://reviews.vapour.ws/r/1995/)