Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Add a base "fake" type, for using in testing. #48
Conversation
natefinch
reviewed
Jan 27, 2015
| +) | ||
| + | ||
| +// FakeCallArgs holds all the args passed in a call. | ||
| +type FakeCallArgs map[string]interface{} |
natefinch
Jan 27, 2015
Contributor
I don't think I'd bother naming this type, since it doesn't actually get used as a type anywhere. Just use map[string]interface{}.
ericsnowcurrently
Jan 29, 2015
Contributor
I did it to help in couple spots, like when calling CheckCalls(). testing.FakeCallArgs{...} is nicer to use (and read) than map[string]interface{}{...}. There are a number of other places in juju where we do the same thing (e.g. testing.Attrs).
dimitern
Feb 12, 2015
Contributor
Why do you need the name at all? Args as []interface{} for a given call should be sufficient, as well as Results []interface{}, rather than Errors. See comments below as well.
ericsnowcurrently
Feb 12, 2015
Contributor
Yeah, I see what you mean. It definitely simplifies things if you don't worry about the parameter names.
natefinch
reviewed
Jan 27, 2015
| +// embedded in another struct that will define the methods to track: | ||
| +// | ||
| +// type fakeConn struct { | ||
| +// fake |
natefinch
reviewed
Jan 27, 2015
| +// any method on the fake. It should be called for the error return in | ||
| +// all faked methods. | ||
| +func (f *Fake) Error() error { | ||
| + if len(f.calls) != f.FailOnCall+1 { |
natefinch
Jan 27, 2015
Contributor
I think this could be confusing, because this will only fail on call N, not N+1 etc.
ericsnowcurrently
Jan 29, 2015
Contributor
Yeah, I agree. I've changed things so that it's a bit more intuitive. See the latest code.
natefinch
reviewed
Jan 27, 2015
| + calls []FakeCall | ||
| + | ||
| + Err error | ||
| + FailOnCall int |
ericsnowcurrently
added some commits
Jan 29, 2015
natefinch
reviewed
Feb 10, 2015
| +// Error returns the error that should be returned on the nth call to | ||
| +// any method on the fake. It should be called for the error return in | ||
| +// all faked methods. | ||
| +func (f *Fake) Err() error { |
natefinch
Feb 10, 2015
Contributor
I think this might be more obvious if it were called NextErr(), since it actually pops one off the stack.
natefinch
reviewed
Feb 10, 2015
| +} | ||
| + | ||
| +// Reset sets the fake back to a pristine state. | ||
| +func (f *Fake) Reset() { |
natefinch
Feb 10, 2015
Contributor
This seems not super useful? Can't you just do foo.fake = &testing.Fake{}? Not sure what this really gains you.
ericsnowcurrently
Feb 11, 2015
Contributor
Agreed. Pretty sure I added it to shadow ResetCalls, but it really isn't needed.
natefinch
reviewed
Feb 10, 2015
| + | ||
| +// ResetCalls clears the history of calls. | ||
| +func (f *Fake) ResetCalls() { | ||
| + f.Calls = nil |
ericsnowcurrently
Feb 11, 2015
Contributor
I guess it isn't strictly needed. I added it in response to a concrete use case. However, I can solve that problem another way without ResetCalls.
natefinch
reviewed
Feb 10, 2015
| + | ||
| +// AddRcvrCall records a faked method call for later inspection using | ||
| +// the CheckCalls method. All faked methods should call AddCall. | ||
| +func (f *Fake) AddRcvrCall(receiver interface{}, funcName string, args FakeCallArgs) { |
natefinch
Feb 10, 2015
Contributor
This doesn't really seem useful.... if the receiver is a pointer (which they often are), then it could change between when you call this and when you check it later. I guess if you're careful to never pass a pointer to this method (even if the receiver really should be a pointer), then it's ok. But it seems like an annoying gotcha (not that I can really think of a better way to do it).
ericsnowcurrently
Feb 11, 2015
Contributor
This method is useful when you are sharing one fake between multiple types. I added it because I needed to distinguish which receiver was used for a method call where there were multiple instances of a type all sharing the same fake. I've updated the doc comment.
natefinch
reviewed
Feb 10, 2015
| +// embedded in another struct that will define the methods to track: | ||
| +// | ||
| +// type fakeConn struct { | ||
| +// *testing.Fake |
natefinch
Feb 10, 2015
Contributor
Note that you don't really need to make this a pointer. It can be a value, and the pointer functions will still do the right thing... and that way you can leave it with the zero value and not need Fake: &testing.Fake{} in the initialization. See this: http://play.golang.org/p/jzNRaXVy4d
ericsnowcurrently
Feb 11, 2015
Contributor
It needs to be a pointer so that the embedded fake can be shared between multiple fake types. Being able to do so was actually super useful in tests I've written. The doc comment for Fake mentions this.
natefinch
reviewed
Feb 10, 2015
| +// SetErrors sets the errors for the fake. Each call to Err (thus each | ||
| +// fake method call) pops an error off the front. So frontloading nil | ||
| +// here will allow calls to pass, followed by a failure. | ||
| +func (f *Fake) SetErrors(errors ...error) { |
natefinch
Feb 10, 2015
Contributor
why do you need this instead of just calling myval.fake.Errors = []error{nil, nil, foo, bar} ?
ericsnowcurrently
Feb 11, 2015
Contributor
It's simply a convenience (making it a little easier to read):
myval.fake.SetErrors(nil, nil, foo, bar)
vs.
myval.fake.Errors = []error{nil, nil, foo, bar}
natefinch
reviewed
Feb 10, 2015
| +// you want returned. The errors will be matched up to the calls made on | ||
| +// Fake methods, in order. This is facilitated by the Err method, as | ||
| +// seen in the above example. In some cases the first method call is not | ||
| +// the one you want to fail. If not then put a nil before the error in |
natefinch
Feb 10, 2015
Contributor
I think this explanation could use some work. It sort of sounds like you can only put one nil before an error return. I think it might be more clear if you just say that errors are returns in sequence according to the Errors list, and if more calls are made than values exist in the list, the remaining calls will return the Error value.
natefinch
reviewed
Feb 10, 2015
| + | ||
| + // Error is the default error (when Errors is empty). The typical | ||
| + // Fake usage will leave this nil (i.e. no error). | ||
| + Error error |
natefinch
Feb 10, 2015
Contributor
Maybe call this DefaultError? Error vs. Errors is not very obvious unless you're reading the documentation.
|
So, overall, it seems mostly fine, with just some niggles. I think that before it gets merged it should get proposed to juju-dev, to see what others think of it, since it's the kind of thing that'll start popping up everywhere, and I want to make sure it passes muster with others. |
ericsnowcurrently
added some commits
Feb 11, 2015
Will do. |
dimitern
reviewed
Feb 12, 2015
| +// } | ||
| +// | ||
| +// func (fc fakeConn) Send(request string) []byte { | ||
| +// fc.AddCall("Send", FakeCallArgs{ |
dimitern
Feb 12, 2015
Contributor
Ugh! I wish this was more like this:
fc.Call("MyFunc", request)
and
fc.MethodCall(receiver, "MyMethod", arg1, arg2)
Why the complexity of having to add long map literals for recording calls?
ericsnowcurrently
Feb 12, 2015
Contributor
That's a good point. I guess it hadn't crossed my mind to not worry about the parameter names. I'll look at that tomorrow.
ericsnowcurrently
added some commits
Feb 12, 2015
|
LGTM |
added a commit
that referenced
this pull request
Feb 12, 2015
ericsnowcurrently commentedJan 21, 2015
Fake (and its components) was developed during work on the GCE provider. I expect it has broader usefulness, so I'm proposing it here.