Testing static functions in testsuite (was Re: [PATCH weston 2/3] drm: port the drm backend to the new init api)

[Bryce Harrington]

On Wed, Aug 26, 2015 at 04:35:59PM +0300, Pekka Paalanen wrote:
> On Tue, 25 Aug 2015 14:24:23 -0700
> Bryce Harrington <bryce at osg.samsung.com> wrote:
> > On Tue, Aug 25, 2015 at 09:03:36AM -0700, Jon A. Cruz wrote:
> > > On 08/25/2015 03:08 AM, Pekka Paalanen wrote:
> > > > How would you suggest we arrange the source code such that we can pick
> > > > arbitrary functions for unit tests without making a copy of that
> > > > function?
> > > > 
> > > > It should be non-disruptive for the source code used for production,
> > > > meaning that we cannot e.g. put #ifdef wrappers around every function.
> > > > I think putting one function per file is also too much.
> > > 
> > > One good point is that if we use a framework that does not require a
> > > single executable per test then linking and source code arrangement is
> > > greatly simplified.
> 
> > > Upon occasion I've used a #define of UNIT_TEST to avoid exposing certain
> > > things during normal builds. This doesn't necessarily need to expose
> > > functions directly, but could be some hook/proxy function. However, my
> > > preference has been to minimize such #ifdef use.
> > 
> > To expand on Jon's comment here, there's several different solutions I
> > know about.
> > 
> > 1.  Hard core Java folk / testing enthusists advocate just avoiding
> >     static[1], so all functions are linkable and unit testable.  Tends
> >     to make good sense for OO code, but with C code that seems a bit too
> >     extreme to me, where static is serving an important need.
> > 
> >     // project.h
> >     #ifdef UNIT_TEST
> >     #  define static
> >     #endif
> 
> This particular piece of code breaks code that relies on static
> variables defined inside functions. Not that those are common or a good
> design at all, but it would be a nasty surprise.
> 
> Not using static in the first place seems like a bad idea.

Agreed.

> > 2.  Another approach is to #define unit_static (or debug_export, or
> >     similar) to be static for normal builds, and empty for test builds,
> >     so tests can link the internal functions.
> > 
> >     // project.h
> >     #ifdef UNIT_TEST
> >     #  define unit_static
> >     #else
> >     #  define unit_static static
> >     #endif
> 
> This could work. In fact, I already used this pattern in matrix.c.
> 
> The downside of this is that one still needs the whole .c file built
> into the test, and you cannot control everything called from the
> function to be tested.

Not sure what you mean by the whole .c file?

You'd link the test against the foo.o module, and that'd link in any of
the functions that were labeled unit_static.  I don't see that there's a
big problem with that, though?

If you're thinking this will bloat the test executable, well maybe but
disk space is cheap, who cares?

As to controlling things to be called from the function under test, 

As to controlling the things that the function to be tested calls, this
is true but that's kind of orthogonal.  First we have to solve the
static function problem, in order to get higher granuarity in
testability of *internal* function calls.  As to the external functions,
let's consider options for that in a bit (see below).

> > 3.  You can add a non-static proxy function that calls the static
> >     function, then use the non-static function in the test.  E.g.
> > 
> >     // foo.c
> >     static int foo() { return 42; }
> > 
> >     #ifdef UNIT_TEST
> >     int test_foo() { return foo(); }
> >     #endif
> 
> Ouch, lots of duplication in declarations and uninteresting code
> cluttering the sources. No thanks.
> 
> > 
> > 4.  A more brute force, yet less invasive approach is to have the unit
> >     test #include the .c file it's going to test.  This seems a bit
> >     clunky to me but this seems to be not that uncommon[2].
> > 
> >     // test-foo.c
> >     #include "foo.c"
> 
> Right. Somewhat similar to number 2.
> 
> > 
> > 5.  There exist some compilation-level techniques, although I don't know
> >     a lot about it.  For instance, objcopy's --globalize-symbol[3] will
> >     force a given symbol to be visible outside the file its' defined in.
> >     There's also a way to give functions shared-library-visibility
> >     instead of file-visibility[4], and just ensure the tests get
> >     compiled as part of the given library when you're doing test builds.
> > 
> >     I've never seen any of these compiler-level techniques in practice
> >     myself, and might worry about platform compatibility issues, but 
> 
> Seems complicated and with the same caveat as number 2, in that you
> cannot (or can you?) control the functions that the tested function is
> calling.

Right, again these are just solutions to making the internal functions
visible for tests.  Handling external function calls is its own
problem.

> I had a wacky idea.
> 
> Our coding style is quite strict. We could have e.g. an awk program or
> something that filters the given functions from a .c file into a new
> file. Find a line that begins with the given function name and '(',
> include also the preceding line, and include all lines up to the first
> one that is "}\n". This way the build system could make an isolated
> copy of the function to be tested into the file. Then the test does
> #include "file" to get the function, and would be in complete control
> of everything around it. Add some hand-waving on how to deal with
> struct declarations etc.
> 
> Completely crazy?

Not crazy, although trade-off here is it adds some brittleness to the
test system since we'd be involving code parsers and such.  This sounds
like essentially a refined version of #4.

> Maybe I'd prefer number 2 as far as it works...

I like it because it's simple.  We could also roll it out function by
function as we write the unit tests.


				 * * *

Functions that call to external services is a separate problem from
accessing the internal functions.  Like we discussed earlier, this often
is handled with mocks - basically we somehow substitute the normal call
for one of our own definition.  We have a few different options
here.  stackoverflow summarizes several approaches:

http://stackoverflow.com/questions/2924440/advice-on-mocking-system-calls

A.  Use a wrapper function, which calls our mock functionality when
    UNIT_TEST is defined, or the real system call otherwise.

B.  Dependency injection.  Instead of calling the system call directly,
    we have a function pointer that we reference.  For testing we have
    it point to our test routine; for production we hook to the actual
    system call.

C.  Use "link seam".  Essentially we use the linker to substitute our
    own definitions of the external function calls.

D.  gcc compiler magic.  gcc has a --wrap option that allows tests to
    replace specific functions with their own wrappers.

Option A adds an extra abstraction layer, but is conceptually
straightforward; maybe the layer can be avoided if the wrappers are
implemented purely as preprocessor macros.  Option B seems powerful but
maybe overly complex.  Option C seems the least invasive and probably
fine if we're dealing with just simple system calls, but I'm not sure it
covers cases where there's some external state we're trying to model in
the test (e.g. monitor configuration).  Option D looks pretty slick but
we're maybe dealing with compiler compatibility issues, but maybe an
equivalent feature is available on other compilers?

I'm less sure what the right approach is for this, but I lean towards
Option D.  It seems least invasive and most maintainable (apart from
platform compatibility issues), and compartmentalizes all the mocking
junk to within the tests, which is probably where it should belong
anyway.  If it ends up being too reliant on gcc-specific features,
Option C is like the manually implemented version of D and might be a
suitable fallback if D proves too limiting.

Bryce

> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2
> 
> iQIVAwUBVd3AzCNf5bQRqqqnAQi5ng//c0OrfoQVYtn1AcstwonPj/wuGwtn+Gtl
> aAE+LYSmubrWuG4bLDfe5Hhq+Ep3s9+06TlnFdiHyi48kc4KSSqgnRjuJFBBEZGp
> yelw9HOBcPoy0VVKaR/d/iK5Z4v+V4Zs9q7JUFphkTibW5FvstRa9R1llx7H+PdW
> glfWvFbNQZ617bW/AfwVyWdXDiysN8SEQqMcnZdkrIbnPfNS9l/6Wya+o+irHrZl
> Bwpj5k+5MIRq0W5S0LMX77Duo7rypTSpTH6z+NOYXXL2FRaLx4GqJGpBTNRTRNKb
> sFnTm+OEZ6PWG0zl6TtMCNRmYgaJW4n/vdvuaEY4gd9eWBCyWwWcFpJPRqPMZNTZ
> 4dwwKpg9GMZTw35MOvo2BaTnqdFcwsxINf+A2ai9WyDC6MT6L8V1kFkDVkj7jEbB
> sSv4Tes+7U8eVgsfiwzE/8X2SENlDWegS78mj96/jXYKVGetvy2fGrgKrusb0+Io
> 6R1xtTQgtggmnOZDvBVCDxb6mml8/iYOy7QP6QF9NlrkOq5jFtD6TIRb3TDCmlhG
> mph8koCrjs2HfCR+hXgNBy27x2fAZXzKf1TUa2uE9L/PKVif3LvbULUNLW0J7ey3
> 7yOG33qcSnmHBOZEpQP8YQVh1wlSrJLq6TMvlblCbEuZJBCftKmG3oRIiN0MfCO9
> M2Bv+kXIYls=
> =cdXr
> -----END PGP SIGNATURE-----