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

[Jeff Law <law at redhat dot com>]

On 11/19/2015 10:35 AM, Bernd Schmidt wrote:
> In general I'm much happier with this approach, and I think this series
> is close to ready, but I want to bring up some questions that could use
> wider discussion.
>
> > This patch adds a selftest.h/.c to gcc, with an API loosely
> > modelled on gtest (though without the use of CamelCase): it
> > supports enough of the gtest API to enable the tests that I
> > wrote to run with minimal changes.
And just to chime in a bit on this -- when David and I talked about 
gtest being used in this way, the problem we're going to run into is 
header file ordering, dependencies and the like.  It was a bit of a 
tangled mess.  It seemed easier to pull together a gtest-ish like 
framework that wasn't going to bump into those issues.


> Here there's a question of style. I don't want to approve or reject this
> just now, I'd like to hear what others think. To my eyes this still
> looks rather seriously overengineered. Plain gcc_assert and if (cond)
> abort (); would work just fine for the tests IMO, it's what we have for
> regular testcases where we don't distinguish between all sorts of
> microscopic subtests, and any gcc_assert failure is easy enough to
> debug, just load up cc1 into the debugger and run. I don't think we need
> output for tests that are really just expected to pass always, all we
> need is the build to stop if an internal error is detected.
But having a clearer pass/fail indication avoids the problem of "does no 
output mean everything passed or nothing ran"?  Though I guess we could 
fix this by having a clear pass/fail indicator that runs last and is 
controlled by the same mechanism as the testsuite as a whole.

I guess in the end, I don't have a clear preference.    Which probably 
argues that, for me, does this stuff make it easier or harder to write a 
test?


> If I'd written it I'd also have used a somewhat lower-tech approach for
> the registration and running of tests, but once again I'd like to hear
> from others.
I was a bit surprised by David's approach, but the more I think about 
it, auto-registration is actually a damn nice feature.

However, I do worry about state from one test leaking into a spoiling 
some later test.  I haven't looked closely to see if that's avoided, but 
it is definitely a worry.

> For things like
>
> > +#define RUN_ALL_TESTS() \
> > +  ::selftest::run_all_tests ()
>
> I don't see the point of the macro. Also, in [8/15]
>
> > +class gimple_test : public ::selftest::test
> > +{
> > + protected:
> > +  void
> > +  verify_gimple_pp (const char *expected, gimple *stmt)
> > +  {
> > +    pretty_printer pp;
> > +    pp_gimple_stmt_1 (&pp, stmt, 0 /* spc */, 0 /* flags */);
> > +    EXPECT_STREQ (expected, pp_formatted_text (&pp));
> > +  }
> > +};
> > +
>
> Why have the class rather than just a function? This sort of thing makes
> me go "overuse of C++".
Conceptually the ability to subclass is one of the key ways to introduce 
testing.   I'm not sure it's buying us much here (because we're not 
subclasing anything in GCC itself), but as an approach to getting 
subsystems under testing, subclassing is a vital capability.

jeff