Re: [PATCH] support: Introduce new subdirectory for test infrastructure

[Carlos O'Donell <carlos at redhat dot com>]

On 12/05/2016 10:57 AM, Florian Weimer wrote:
> On 12/02/2016 08:21 PM, Carlos O'Donell wrote:
> 
>> My only subconscious fear is to stay way from anything that looks as
>> complicated as TET/VSXGen from the Open Group. Your additional infrastructure
>> is nowhere near that point, but we should be conscious of an overall design
>> goal which is to remain as small as possible.
> 
> But we need something to view changes in subtest failures more easily.

Agreed.

>> Have you tested your changes with --enable-hardcoded-path-in-tests and made
>> sure nothing breaks?
> 
> Ugh. There was some breakages, fixed in the attached patch. I had
> dropped the ld.so override accidentally. I don't see any regressions
> anymore.

Thank you for testing this. This configure option is incredibly helpful in building
tests that can be easily run in gdb.
 
>> (3) Implementation details.
>>
>> You need a README.txt which:
>>
>> * Describes the goals of the infrastructure, this will provide guidance
>>   for what we should or should not consider adding.
> 
> I think it's premature for that.

OK.

>> * It should also give an example minimal template for a test that users the
>>   infrastructure.
> 
> I added a dummy test for that.

Looks good.

>> * [optional] List _all_ functions that are part of the support infrastructure
>>   in order to give a broad overview of what's available without having to
>>   crawl all the headers.
> 
> I don't like this because it will introduce additional merge
> conflicts while backporting new tests. I'm just listing the header
> files.

OK.
 
>> Notes:
>>
>> Is there any way to avoid needing to list $(libsupport) as a dependency
>> of the hand-built DSOs in the test framework? This is not a blocker, just
>> a question about how we might make it easier to use these functions from
>> all such DSOs without developers having to remember to add it as a dependency.
> 
> I don't know where the pattern rule is located which builds test DSOs.

It might default to the normal pattern rule that just builds DSOs so it might
be impossible to fix without splitting into a new rule, and that has its own
complications.

Let us leave this for now.

>> (b) ChangeLog typo.
> 
> Fixed.

OK.

>> (c) Why do you set errno in xpthread_check_return?
> 
> It's needed for %m.  Most libpthread functions do not set errno.

OK.

>> (d) I noticed that the waitpid in signal_handler has a timeout that is
>>     not in any way a function of TIMEOUTFACTOR, and it probably should be.
>>     Nothing to do with your changes but I wanted to mention it. If you're
>>     already refactoring and running tests it might be nice to make it
>>     something that scales with TIMEOUTFACTOR.
> 
> I'm not sure the timeout factor should apply in this case.
> 
> There are non-async-signal-safe functions in there, too. If the
> process is stuck in a deadlock, calling exit (not _exit) from the
> signal handler is probably what we want to do.
> 
> But I would avoid that scope creep here.

Agreed. Leave it for now. Just mentioning so if someone comes back to this
thread they have a record of the review.

>> (e) xpthread_join should take a parameter "canceled" to indicate it should
>>     verify the result of pthread_join was PTHREAD_CANCELED, and then this
>>     should be used when refactoring to tighten up the expected cancellation
>>     results e.g. tst-cancel7.c should be adjusted to use this.
> 
> But very few callers will need this, so I don't think it makes sense.
> Maybe we can aim for something like this in the future:
> 
>   TEST_VERIFY (xpthread_join (thr) == PTHREAD_CANCELED);
> 
> It's slightly more verbose, but you don't need to wonder what the
> Boolean argument to xpthread_join does.

Agreed, and the change breaks the nominal notion that 'xfoo' is just
'foo' but with internal error checking.

>>> +int
>>> +create_temp_file (const char *base, char **filename)
>>> +{
>>> +  char *fname;
>>> +  int fd;
>>> +
>>> +  fname = (char *) xmalloc (strlen (test_dir) + 1 + strlen (base)
>>> +                + sizeof ("XXXXXX"));
>>> +  strcpy (stpcpy (stpcpy (stpcpy (fname, test_dir), "/"), base), "XXXXXX");
>>
>> OK, but ugly IMO. I prefer a more verbose version. #bikeshed
> 
> It was this way in the original.  We could just use xasprintf nowadays.
 
OK. Leave it for now.
 
>>> +/* This test skeleton is to support running existing tests.  New tests
>>> +   should use <support/test-driver.c> instead; see the documentation
>>> +   in that file for instructions.  */
>>
>> OK. Should reference support/README.txt.
> 
> I've added a reference to README-testing.c.

OK. Thanks.

> I realized that I had to convert another test case
> (tst-secure-getenv). In the attached verion, I renamed signal.h to
> xsignal.h and thread.h to xthread.h, to avoid potential header name
> conflicts.

v2 patch looks good to me for checkin.

-- 
Cheers,
Carlos.