<div dir="ltr">High level thoughts:<div><ol><li>Using an interface{} instead of patching function pointers sounds like a great thing</li><li>Writing a provider via faking out rather than implementing a double means that it is really hard to run the cross-provider interface tests (environs/jujutest), which means we run into what we have with Azure, where *some* of the functions return a different error than the one we expect (see the recent 'juju restore' regression). Our interface tests aren't amazing, but if we can push more on that, then we can avoid having implementations leak through their abstractions.</li><li>I do realize that time-to-something-working is a lot faster with a Fake implementation, but ongoing support and maintenance might pay off more. (I love how quickly the GCE provider was available, I'm concerned if we know that it actually acts like EC2/Openstack when it comes to edge cases.)</li></ol><div>John</div></div><div>=:-></div><div> </div></div><div class="gmail_extra"> <div class="gmail_quote">On Wed, Feb 11, 2015 at 10:53 PM, Eric Snow <<a href="mailto:eric.snow@canonical.com" target="_blank">eric.snow@canonical.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">tl;dr Using fakes for testing works well so I wrote a base fake type. [1] While working on the GCE provider, Wayne and I started taking a different approach to unit testing than the usual 1. expose an external dependency as an unexported var; 2.export it in export_test.go; 3. patch it out in tests. [2] Instead we wrote an interface for the provider's low-level API and implemented the provider relative to that. [3] Then in tests we used a fake implementation of that interface instead of the concrete one. Instead of making the actual API requests, the fake simply tracked method calls and controlled return values. Using the fake had a number of benefits: 1. our tests were very focused on what code gets exercised, meaning we don't take responsibility for what amounts to testing our dependencies at the same time as the code at hand. 2. the tests ran faster since they aren't making HTTP requests (even if just to localhost). 3. the provider code isn't cluttered up with vars-only-there-to-be-patched-out. [2] 4. the test code isn't cluttered with many separate s.PatchValue calls. [2] 5. setting up the faked return values was cleaner. 6. checking which methods were called (in order) along with the arguments, was easier and cleaner. In addition to all that, taking the fake approach required that we encapsulate our low-level dependencies in a single interface. This was good because it forced us to spell out those dependencies. That helped us write the provider better. The low-level interface also made the code more maintainable since it makes the boundary layers explicit, and formats it in a concise display (the interface definition). So I've taken the base fake type from the GCE patch and pulled it into patch against the testing repo. [1] I've made some adjustments based on use cases I've had in subsequent patches. Nate has the bright idea of getting some feedback from the team before landing anything "since it's the kind of thing that'll start popping up everywhere, and I want to make sure it passes muster with others." I'm not suggesting that fakes are the solution to all testing problems so let's please avoid that discussion. :) Neither am I proposing that any existing tests should be converted to use fakes. Nor am I suggesting the need for any policy recommending the use of fakes. As well, we still need to make sure we have enough "full stack" test coverage to be confident about integration between layers. Relatedly, the concrete implementation of low-level interfaces needs adequate test coverage to ensure the underlying APIs (remote, etc.) continue to match expected behavior. So yeah, neither fakes nor isolated unit tests meet all our testing needs. I just think the base fake type I've written will help facilitate a cleaner approach where it's appropriate. Thoughts? -eric [1] <a href="https://github.com/juju/testing/pull/48" target="_blank">https://github.com/juju/testing/pull/48</a> [2] We were still feeling out the approach so we still took the var-and-patch in some cases. [3] See "gceConnection" in <a href="http://reviews.vapour.ws/r/771/diff/#46" target="_blank">http://reviews.vapour.ws/r/771/diff/#46</a>. -- Juju-dev mailing list <a href="mailto:Juju-dev@lists.ubuntu.com">Juju-dev@lists.ubuntu.com</a> Modify settings or unsubscribe at: <a href="https://lists.ubuntu.com/mailman/listinfo/juju-dev" target="_blank">https://lists.ubuntu.com/mailman/listinfo/juju-dev</a> </blockquote></div> </div>