[PATCH weston 2/3] drm: port the drm backend to the new init api

Tue Aug 25 14:24:23 PDT 2015

On Tue, Aug 25, 2015 at 09:03:36AM -0700, Jon A. Cruz wrote:
> On 08/25/2015 03:08 AM, Pekka Paalanen wrote:
> > On Mon, 24 Aug 2015 19:26:48 -0700
> > Bryce Harrington <bryce at osg.samsung.com> wrote:
> > 
> >> On Thu, Aug 13, 2015 at 01:53:05PM +0300, Pekka Paalanen wrote:
> >>> From: Giulio Camuffo <giuliocamuffo at gmail.com>
> >>
> >> This is a big patch with a lot of changes, and I'm worried about landing
> >> it right as we're on the eve of beta.  If it could be broken up into
> >> smaller easy-to-review bits, it might make it more digestible...
> >>
> >> That said, I like where this is going.  I'd be totally okay with landing
> >> it post-release.
> >>  
> >>> ---
> >>>  Makefile.am          |   3 +
> >>>  src/compositor-drm.c | 234 ++++++++++++++++-----------------------------------
> >>>  src/compositor-drm.h |  89 ++++++++++++++++++++
> >>>  src/main.c           | 127 +++++++++++++++++++++++++++-
> >>>  4 files changed, 292 insertions(+), 161 deletions(-)
> >>>  create mode 100644 src/compositor-drm.h
> > 
> > ...
> > 
> >> Nice refactoring.  With this change, this function also becomes quite
> >> simple to write a test case for... just pass in various strings and
> >> verify the correct int is returned.
> >>
> >>> @@ -2127,31 +2086,29 @@ get_gbm_format_from_section(struct weston_config_section *section,
> >>>   * Find the most suitable mode to use for initial setup (or reconfiguration on
> >>>   * hotplug etc) for a DRM output.
> >>>   *
> >>> + * @param backend The DRM backend object
> >>>   * @param output DRM output to choose mode for
> >>> - * @param kind Strategy and preference to use when choosing mode
> >>> - * @param width Desired width for this output
> >>> - * @param height Desired height for this output
> >>> + * @param config Desired configuration for the output
> >>>   * @param current_mode Mode currently being displayed on this output
> >>> - * @param modeline Manually-entered mode (may be NULL)
> >>>   * @returns A mode from the output's mode list, or NULL if none available
> >>>   */
> >>>  static struct drm_mode *
> >>> -drm_output_choose_initial_mode(struct drm_output *output,
> >>> -			       enum output_config kind,
> >>> -			       int width, int height,
> >>> -			       const drmModeModeInfo *current_mode,
> >>> -			       const drmModeModeInfo *modeline)
> >>> +drm_output_choose_initial_mode(struct drm_backend *backend,
> >>> +			       struct drm_output *output,
> >>> +			       struct weston_drm_backend_output_config *config,
> >>> +			       const drmModeModeInfo *current_mode)
> >>
> >> This function has a number of exit points, making it a really good
> >> candidate for writing a thorough collection of test cases.  The tricky
> >> bits would be the two drm_output_add_mode() calls, which will need
> >> mocks; initially though a first cut test could leave those two branches
> >> as TODO.
> > 
> > Hi Bryce and Jon,
> > 
> > leaving all the other comments for later, there is one thing I'm
> > curious about:
> > 
> > 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.
> > 
> > I wouldn't want to make a copy and then later find out that we have
> > been testing the copy while the code actually used in production has
> > already changed. Maintaining a copy manually is a no-go.
> 
> Well... there are a few approaches in general that could help. First of
> all, you are correct about avoiding duplication.
> 
> 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.
> 
> I've found it very helpful to have source directories structured to
> match the deployed layout. That is, if there are header files that will
> be exposed to included by end users then those should be in an include
> folder in the source tree to begin with (as opposed to some that have
> all source files together and then have the build process rearrange them
> for installation).
> 
> Then it ends up easy to add headers that declare functions that might be
> used in tests. Since they do not go into the include folders, the chance
> of external code using them is minimized. These can also have
> "_internal" or such as part of their name.
> 
> 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

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

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

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"

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 

Of these, I would probably pick #2, but I could live with 3-5 too.
I swear I've seen #2 used in other FOSS projects but I'm not finding
a good example.

Unit testing is, by definition, drilling down deeper into the codebase
than the regular interface, so to some degree you have to give it
special access of some sort or other.  It doesn't have to be done
invasively, but sometimes is cleaner if some allowance is made.
Mocks often benefit from being allowed to be a little invasive into the
code anyway (e.g. generalizing direct calls to external libraries to be
overridden by the test with the mock implementation).

> With C++ code arrangement gets a bit more regular, with one .cpp per
> class and one or two corresponding .h (foo.h and foo_internal.h) files
> becoming the norm. Since we are not in that arena it is not a big factor.
> 
> Since we are dealing with C code, required layouts are much more liberated.
> 

Bryce

1: http://googletesting.blogspot.com/2008/12/static-methods-are-death-to-testability.html

2: http://www.rolandstigge.de/track/Electronica06.pdf
   c.f. section 3.2.3

3: https://sourceware.org/binutils/docs-2.23/binutils/objcopy.html

4: https://gcc.gnu.org/wiki/Visibility

> -- 
> Jon A. Cruz - Senior Open Source Developer
> Samsung Open Source Group
> jonc at osg.samsung.com