Feedback on a base "fake" type in the testing repo

[Andrew Wilkins]

On Thu, Feb 12, 2015 at 2:53 AM, Eric Snow <eric.snow at canonical.com> wrote:

> 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.
>

Sounds good. The less patching the better.

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.
>

Looks okay in principal, though I'm not sure how much benefit it would
add to the existing, tailored fakes in the codebase. It seems a little bit
weird that error results are encapsulated, but not non-error results.

Thoughts?
>
> -eric
>
> [1] https://github.com/juju/testing/pull/48
> [2] We were still feeling out the approach so we still took the
> var-and-patch in some cases.
> [3] See "gceConnection" in http://reviews.vapour.ws/r/771/diff/#46.
>
> --
> Juju-dev mailing list
> Juju-dev at lists.ubuntu.com
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/juju-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/juju-dev/attachments/20150212/9a6b18f9/attachment.html>