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

[Bryce Harrington]

On Thu, Aug 27, 2015 at 10:17:30AM +0300, Pekka Paalanen wrote:
> On Wed, 26 Aug 2015 14:26:09 -0700
> Bryce Harrington <bryce at osg.samsung.com> wrote:
> 
> > 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:
> 
> > > > 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).
> 
> The problem I see is that, let's say we have a static function foo() we
> want to test. This function calls another static function bar() in the
> same .c file. If we compile that whole file into an .o and link it
> with the test program, we cannot intercept the call from foo() to bar().
> 
> So yes, you got the point.

Actually, the invasive solutions would be able to handle this case
(i.e. create a macro or wrapper function that calls bar() when UNIT_TEST
is not defined, or utilize a function pointer.)  Alternatively, if you
move bar() into its own compilation unit, then you can handle it at the
linker level, like in all the other cases.

But, I have hope this is a rare case - if bar() has it's own set of unit
tests that are adequately thorough, we should have a good handle on its
behavior and hopefully shouldn't need to intercept it.  If bar() makes
external calls, we'd likely want to intercept those, but that's no
different than if foo() was making those calls.  If bar() is
exceptionally complex, maybe it makes sense to intercept it; OTOH maybe
that's a sign that bar() should be broken up into more easily tested
units...

Where I could see this scenario become more of a problem is at higher
logical levels (i.e. foo()s that call lots and lots of bar()s and
baz()s).  But it's sort of a known issue with unit testing that it does
better with leaf functionality than trunk.  Typically one starts talking
about integration testing at this level.  But "integration testing" is
basically just a fancy name for a whole range of different kinds of
higher level testing.  Much of our existing collection of tests probably
would qualify as integration testing, for example.  So for this level of
testing I think we can just keep on doing what we've already been doing,
and not worry too much about proper unit testing of the higher logical
level functions.


> > > 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.
> 
> Sure. It's good for the cases where it works, and maybe it is the
> majority or even all.
> 
> Only if we really want to overwrite other static functions we will need
> something like the crazy.
> 
> Two ways, let's pick the appropriate per case.
> 
> I'm getting a little ahead of ourselves.
> 
> > 				 * * *
> > 
> > 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.
> 
> I don't like A or B, because they obfuscate the production code.
> 
> To me, C is the obvious choice. Overwriting library calls is easy, we
> already do it for malloc et al. for some basic leak checking in
> Wayland's tests/test-runner.c. No-one has complained of that approach
> yet, AFAIK, so presumably it works for everyone.

Yeah, I forgot to mention but Cairo also does this.  Googling about this
seems to be a fairly widely used technique.

> I hadn't heard about D. If it works on the compilers our downstreams
> care about, why not. Maybe we should add one simple test using that
> when we start the 1.10 development cycle, then we should know around
> the middle of 1.11 cycle if we can rely on it.

Sounds like a good approach.

Bryce

> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2
> 
> iQIVAwUBVd65kSNf5bQRqqqnAQi5kw//cVlIZg9tNb244Wfu35KUmpQSnRHDEOa7
> UbkyTFOrXQvCTMwzI8MYhbg6im/Fj8PzrUSFsyT7kH/Gltzmk2Y+90BN/MmYcm8e
> fs2YFaRXczVaPt/xfA6FpaXtB1HzNdQSeJE0mTI8SvY4D3ehavBCwW+OMIIg60u7
> AM1IXvWOdG7HRcXE9y688gd8DZmB7RNuzPtQJf0gATtoCGi8wWZ0daPcRGpGkSu3
> zhIkfwq5uQ0CWQYJo/icTo2oFIG8e7r3odsnV1x9EIFtX7kkR23XT1fRebcUfb5Y
> KoLzTkJGickMlfdflGPnAC19D5TtfRbg0Ofp8wwM4nYNys2B1XF9sUBUi6c2xXPO
> 6l9cWjG5i7xOeRoGlHxle9shcbVp9zMLp+lBTfFyUE2GddxECq1HP4tCxIbEoWfe
> KRN2zKwUUNNEHj5C39vZob2WD23miryg26c2wC1Pv+1/rQ3hh6uguXPHvqyWs7io
> Os4tqHOUrfP8TOPrgG3RAlqbowkHJCcpp8DBYQA/xWkQsfmZI8MSLkUrMCaqM8Kh
> FqIJnvq52hKg55Ai3G2YGSf4lipyrS4j3cCjaJ1T+Hkneii5m1ieK+GQIEqoNzxX
> Qgz4VMbqGch3HAoGVP8qgGSeCVH+BxrpR+HZjb38RWf1bAkEUd4ZQAX3c4Rcj3vp
> YTKreY3/csY=
> =ZM+9
> -----END PGP SIGNATURE-----