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

[Jeff Law <law at redhat dot com>]

On 11/19/2015 10:04 AM, David Malcolm wrote:
> 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.
I know it's small, but avoiding CamelCase is a plus in my book :-)

> Like gtest, tests are automatically discovered, using
> some global object ctor magic.  Sadly this doesn't work
> for some reason for source files which are purely tests, so
> as a nasty workaround I #include those files from toplev.c
That needs to be tracked down.


> I didn't document -fself-test; should I?  (maybe just in the
> internal docs?)
I'd document in the internals.

> This iteration of the patchkit doesn't yet do anything to
> automatically invoke -fself-test; I've been running it by hand
> when compiling an empty .c file, so it's been running inside cc1.
> How should this be hooked up?
It has to be hooked up within the gcc directory.    Presumably it's just 
a new Makefile target that "all" depends upon.    If the self-test 
fails, I think we should stop the build.  And when host != build I don't 
think we can run the self-tests.

The other option, and I'm hesitant to suggest it would be to run the 
selftest within the stanza that builds cc1.  I don't think this is a 
good idea.

> Currently -fself-test requires an input file; should it?  (But if not,
> what should the driver run?  All of them?).  Requiring an input file,
> and providing an empty one may be simplest.
I would think that -fself-test is a driver option.  Presumably this is 
going to be fast enough that the Makefile target just runs all of them.

> I anticipate a discussion about output formats; the current output
> format is deliberately very verbose, to ensure that we're capturing
> everything of interest.  It contains file/line info of each
> EXPECT/ASSERT so that it's easy in Emacs to click in the compilation
> buffer to go to specific assertions.
I guess I'll look at the line number as a feature rather than an 
annoyance for diffing.

> Potentially we could have some kind of verbosity flag for the
> selftests, I guess.
My inclination would be to keep the verbosity as long as its logging to 
a file by default.

> I did attempt to use "inform" to report results, but I don't think
> this is appropriate for the most verbose form of output:
> (A) some tests manipulate cfun and hence lead to strange
> "In function ..." messages as they run based on the changes to cfun
> (B) I want to write selftests for the diagnostics subsystem itself
Right.  I think we largely want to avoid using GCC's internals for test 
reporting.  So this seems right to me.

> >
> Notes on porting tests from gtest:
> * Fixtures inherit from ::selftest::test, rather than
>    from gtest's ::testing::Test
> * There's no support yet for Setup and Teardown vfuncs (which
>    presumably would be "setup" and "teardown")
> * I only implemented what I needed
> * I didn't implement type-parametrized testing, so for now
>    I've dropped test-wide-int.c (which is the only test I had
>    that was parametrized over multiple types).
>
> gcc/ChangeLog:
> 	* Makefile.in (OBJS-libcommon): Add selftest.o.
> 	(OBJS-libcommon-target): Add selftest.o.
> 	(COLLECT2_OBJS): Add selftest.o.
> 	* common.opt (fself-test): New.
> 	* selftest.c: New file.
> 	* selftest.h: New file.
> 	* toplev.c: Include selftest.h.  Add includes of function-tests.c,
> 	hash-map-tests.c, hash-set-tests.c, rtl-tests.c as a workaround
> 	for a linker issue.
> 	(toplev::run_self_tests): New.
> 	(toplev::main): Handle -fself-test.
> 	* toplev.h (toplev::run_self_tests): New.

 +#if CHECKING_P
> +
> +namespace selftest {
> +
> +class test;
> +class runner;
> +class registrator;
> +
> +/* The entrypoint for running all tests.  */
> +
> +extern int run_all_tests ();
> +
> +/* The class ::selftest::runner is responsible for gathering results,
> +   and for output.  */
> +
> +class runner
> +{
> +public:
> +  runner ();
> +  ~runner ();
> +
> +  void begin_test (test *t);
> +  void pass (const char *file, int line, test *t, const char *msg);
> +  void fail (const char *file, int line, test *t, const char *msg);
> +  void end_test (test *t);
Presumably we can extend this with an xfail if the need arises.  I'm 
thinking of stuff like "you can't change the underlying state of a 
bitmap while you're iterating on it".   I'm pretty sure I could write a 
test for this if I dug into my archives.  I've always considered it a 
bug in our implementation.

> +
> +/* Evaluate EXPECTED and ACTUAL and compare them with strcmp, issuing
> +   PASS if they are equal, FAIL if they are non-equal.  */
> +
> +#define EXPECT_STREQ(EXPECTED, ACTUAL)			       \
> +  SELFTEST_BEGIN_STMT					       \
> +  const char *desc = "EXPECT_STREQ (" #EXPECTED ", " #ACTUAL ")"; \
> +  const char *expected_ = (EXPECTED);				  \
> +  const char *actual_ = (ACTUAL);				  \
> +  if (0 == strcmp (expected_, actual_))				  \
> +    pass (__FILE__, __LINE__, desc);			       \
> +  else							       \
> +    fail (__FILE__, __LINE__, desc);			       \
> +  SELFTEST_END_STMT
nit, don't we typically write strcmp () == 0?

> @@ -1995,6 +1997,52 @@ toplev::start_timevars ()
>     timevar_start (TV_TOTAL);
>   }
>
> +/* For some tests, there's a natural source file to place them in.
> +   For others, they can live in their own "foo-tests.c" file.
> +   Ideally, these "foo-tests.c" files would be added to OBJS in
> +   Makefile.in.  However, for some reason that approach doesn't
> +   work: the tests don't get run..  The linker appears to be discarding
> +   the global "registrator" instances in files which are purely
> +   test cases (apart from ggc-tests.c, which works for some
> +   reason; perhaps the GC roots is poking the linker in such a way
> +   as to prevent the issue).
> +
> +   Hence as a workaround, we instead directly include the files here.  */
I'd really like to know the whys here before agreeing to the workaround.

Bernd has some stylistic comments as well.  I won't repeat them.

Jeff