Defaulting tests internal to the package

[William Reade]

On Mon, Jan 25, 2016 at 4:29 AM, Nate Finch <nate.finch at canonical.com>
wrote:

> I think the idea that tests of an internal function are worthless because
> you might just not be using that function is a fairly specious argument.
> That could be applied to unit tests at any level.  What if you're not using
> the whole package?  By that logic, the only reliable tests are full stack
> tests.
>
Not quite: the most important questions you should be asking are "will a
behavioural change cause this test to fail?" and "will this test's failure
indicate a behaviour change?". When you stick to external package tests,
you are much more likely to be able to answer "yes" to those questions (and
you're much more likely to be able to *keep on* answering yes as your
package is inevitably modified by people who don't know it as well as you).

I am saying that the only reliable tests are those that exercise *the same
code paths* as can be invoked by a client; and that internal tests, which
*by definition* fail to enforce this restriction, come with subtle but
serious long-term costs, such that it's very rare for their net value to be
positive in the long run. Hence, shorthand, "worthless" :).

> I think that William's position is correct in an ideal world.  The public
> interface is the only contract you really need to worry about.  In theory,
> all your packages would be simple enough that this would not be difficult,
> and the tests would be straightforward.  However, in practice, some
> packages are fairly complex beasts, and it can literally be an order of
> magnitude easier to test a bit of logic internally than to test it
> externally.  And I don't just mean easier in terms of time it takes to
> write (although that shouldn't be ignored - we do have deadlines), but also
> in simplicity of the code in the tests... the more complicated the code in
> the tests, the harder they are to get right, the more difficult it is for
> other people to understand them, and the more difficult it is to maintain
> them.
>
Right. The point is not that you cannot ever write internal tests, I think
I've always (regretfully) agreed that sometimes, indeed, they are necessary
for the reasons you describe -- and Tim's point about model migrations is
well taken, the state package is absolutely a case where the forces in play
push you hard in that direction. But they are a *much much* weaker tool
than external unit tests, and they're ultimately trying to do the same job.
So you should be very aware of what you're giving up when you use them; to
the point, IMO, where you should explicitly consider them a tool of last
resort.

And I think it's worth reiterating my point re code quality: when it's hard
to write external tests for some piece of functionality, that is *strong*
evidence that the concept exposed is only half-baked. When this happens,
you should strongly prefer to evolve the SUT towards better coherence; and
when circumstances prevent you from so doing, you should be thinking
clearly about the characteristics of the particular technical debt you're
taking on in service of those deadlines. A complicated fixture has costs;
an internal test has costs; and  I think you're insufficiently wary of the
latter.

(And I'm not saying we should default to taking on complicated fixtures,
either -- I'm saying that the need for *either* is a problem with the
*code*, and that we need to consider the situation from that perspective
before deciding that we need to take on either form of technical debt for
the tests. And so, when we can, we should gently move the code towards the
ideal, because then we get better and simpler client code everywhere, not
just in the local tests.)

> Even if you have perfect code coverage, using inversion of control and
> dependency injection to be able to do every test externally, you can still
> have bugs that slip through, either due to weaknesses in your tests, or
> even just poorly coded mocks that don't actually behave like the production
> things they're mocking.  Isolating a piece of code down to its bare bones
> can make its intended behavior a lot easier to define and understand, and
> therefore easier to test.
>
Sure, you can screw up external tests. It's much easier to screw up
internal tests, and much harder to see when you've done it.

> I think small scope internal unit tests deliver incredible bang for the
> buck.  They're generally super easy to write, super easy to understand, and
> give you confidence that you haven't screwed up a piece of logic. They're
> certainly not ironclad proof that your application as a whole doesn't have
> bugs in it... but they are often very strong proof that this bit of logic
> does not have bugs.
>
I strongly agree that unit tests should be tightly scoped, easy to write,
easy to understand. But a unit test for a unit of *logic* is nowhere near
as valuable as a unit test for a unit of *functionality*, and is much more
vulnerable to churn over time. (I kinda feel like I'm just restating "test
behaviour, not implementation"... I thought this maxim was widely accepted?)

> While I have seen plenty of bugs that were caused by a developer not using
> a piece of code he/she was supposed to use, it has almost always been
> exported code that wasn't used - a utility function in another package, for
> example.  Certainly it's possible you could accidentally stop using an
> internal function and no have anyone notice... but I think that's fairly
> unlikely compared to the concrete benefits you get from simpler,
> faster-to-write internal tests.
>
I'd say that a mismatch between what's tested and what's actually run in
production is the single most common root cause of false test results (be
they positive or negative); and the overall value of our testing strategy
is limited by the prevalence of false results.

Confusing tests are similarly pernicious; but likewise, a test's simplicity
and speed to write/understand/modify are fundamentally limited by the
quality of the interface to the SUT. Falling back to a complicated fixture
or an internal test is essentially an admission of poor code structure;
even when it's necessary we should be aware that it's a defeat, and that we
can't afford to keep losing those battles in the name of short-term
expediency. And we certainly should not default to losing those battles
without even trying.

Cheers
William
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/juju-dev/attachments/20160125/d2d38401/attachment-0001.html>