Re: [PATCH 01/15] Selftest framework (unittests v4)

[David Malcolm <dmalcolm at redhat dot com>]

On Thu, 2015-11-19 at 19:44 +0100, Bernd Schmidt wrote:
> On 11/19/2015 07:08 PM, David Malcolm wrote:
> > gcc_assert terminates the process and no further testing is done,
> > whereas the approach the kit tries to run as much of the testsuite as
> > possible, and then fail if any errors occurred.
> 
> Yeah, but let's say someone is working on bitmaps and one of the bitmap 
> tests fail, it's somewhat unlikely that cfg will also fail (or if it 
> does, it'll be a consequence of the earlier failure). You debug the 
> issue, fix it, and run cc1 -fself-test again to see if that sorted it out.
> 
> As I said, it's a matter of taste and style and I won't claim that my 
> way is necessarily the right one, but I do want to see if others feel 
> the same.
> 
> > The patch kit does use a lot of "magic" via macros and C++.
> >
> > Taking registration/discovery/running in completely the other direction,
> > another approach could be a completely manual approach, with something
> > like this in toplev.c:
> >
> >    bitmap_selftest ();
> >    et_forest_selftest ();
> >    /* etc */
> >    vec_selftest ();
> >
> > This has the advantage of being explicit, and the disadvantage of
> > requiring a bit more typing.
> > (possibly passing in a "selftest *" param if we're doing the
> > try-to-run-everything approach, so we can count failures etc without
> > introducing globals)
> >
> > Was that what you had in mind, or something else?
> 
> It's one option, but it doesn't seem like the best one either. I was 
> thinking of something not dissimilar to your approach, but with fewer 
> bells and whistles. My class registrator would look something like this:
> 
> static list<void (*)()> test_callbacks;
> 
> class registrator
> {
> public:
>    registrator (void (*)() cb)
>    {
>      test_callbacks.push_front (cb);
>    }
> }
> 
> (or use a vec if you can do that at global constructor time)
> 
> and then you just walk the list and run the callbacks when you want to 
> run tests. The one you have implements both the registration and a 
> special case linked list, which just doesn't look right to me, 

FWIW, the reason I special-cased the linked list was to avoid any
dynamic memory allocation: the ctors run before main, so I wanted to
keep them as simple as possible.  Putting the linked list directly into
those objects means that running the ctors is a simple case of wiring up
some pointers: the memory is already statically allocated.  (also, one
thing I want to test is vec<> itself [1]).

Anything more complicated feels to me like asking for trouble - and as
noted, I already have some bugs to track down with the linker apparently
optimizing away some tests.  (Maybe auto-registration is more trouble
than it's worth?)

> and I think I'd also have avoided the runner class.

The runner class was mostly about accumulating pass/fail information,
which goes back to the abort-on-first-failure vs try-to-run-everything
question.  If we do want the latter, it could be global state if that
would be simpler (though I prefer to avoid global state).

Dave

[1] although to be fair, I used vec<> in one place within the patch,
when sorting the tests.