coreutils
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 00/17] De-recursion for the 'tests' subdirectory.


From: Stefano Lattarini
Subject: Re: [PATCH 00/17] De-recursion for the 'tests' subdirectory.
Date: Tue, 04 Sep 2012 21:11:37 +0200

On 09/04/2012 06:03 PM, Jim Meyering wrote:
> Stefano Lattarini wrote:
>> On 09/04/2012 05:25 PM, Jim Meyering wrote:
>>>
>>> Hi Stefano,
>>>
>>> I'm done reviewing.
>>> Here's the 17-cset series I'm ready to push.
> 
> [For anyone wondering about the message to which Stefano has
> just replied, it was delayed due to size >100KiB, then rejected, since
> the only one who really needed it was Stefano, and he was a recipient. ]
> 
>> I'll (re-)review it this evening, if you can wait.  Otherwise, feel
>> free to just push it -- I trust you enough anyway :-)
> 
> I can wait, but do not expect to get back to it until tomorrow.
> I've re-inserted this patch:
> 
>     tests: more resilient about tainted absolute srcdir path
>
Find my final nits below.

Thanks,
  Stefano

-*-*-*-

> [PATCH 01/17] build: use 'check-local' to extend the 'check' target
>
> * tests/Makefile.am (check-local): Here, by making this depend
> on 'vc_exe_in_TESTS' ...
> (check): ... rather than this.  While the old usage worked, it relied
> on an implementation detail rather than on documented behavior.
> * src/local.mk (check-local): Similarly, make this depend on
> 'check-README' and 'check-duplicate-no-install' ...
> (check): ... rather than on this.
>
s/rather than this/rather than making this depend on them/

> [PATCH 02/17] maint: avoid parsing of Makefile.am from vc_exe_in_TESTS
>
> * tests/Makefile.am (TESTS): Rename ...
> (all_tests): ... like this, so that we'll still be able to know the
> complete list of our tests even if the user override
>
s/override/overrides/

> TESTS from the command line (which he's allowed to do by the test
> harness API).
>
> [SNIP]
>
> +# Indirections required so that we'll still be able to know the
> +# complete list of our tests even if the user override TESTS from the
>
s/override TESTS/overrides TESTS/

> +# command line (which he's allowed to do by the test harness API).
> +TESTS = $(all_tests)
> +root_tests = $(all_root_tests)
> +
>
> -# Ensure that all version-controlled executable files are listed
> +# Ensure that all version-controlled filestable files are listed
>  # in $(all_tests).
>
Uh?  I've introduced a botched comment.  It should read:

    Ensure that all version-controlled files in 'tests/' that have a
    suffix specified in $(TEST_EXTENSIONS) are listed in $(all_tests).

Sorry for the confusion.

> [PATCH 06/17] tests: remove the unused 'root-hint' target
>
> * tests/Makefile.am (root-hint): Here.  The interested user can see
> the reasons why some tests are skipped by looking at the messages
> they display on the console; here's an excerpt:
>
>    ..
>
"...", not merely ".."

>    PASS: misc/id-groups.sh
>    id-setgid.sh: skipped test: must be run as root
>    SKIP: misc/id-setgid.sh
>    PASS: misc/md5sum.pl
>    ...
>    PASS: df/total-verify.sh
>    2g.sh: skipped test: very expensive: disabled by default
>    SKIP: du/2g.sh
>    ...
>
> Clear enough, and more specific and precise that a generic "some tests
> might need to be run as root" message.  An if that user is interested
>
s/An if/And if/.

> in making those tests run anyway, he'll just take a look to the README
> files to look for info.  So there's no reason to pollute the stdout
> with another "hint" that is subsumed by those messages, and that might
> go unnoticed anyway.
>
> Moreover, and possibly more importantly, that hint wasn't being
> displayed anyway, even before this change!  That's because the
> 'root-hint' target was listed as prerequisite for the 'check-recursive'
> target, which however was not a dependency of the 'check' target in
> 'tests/Makefile.am' (because that makefile contains no $(SUBDIRS)
> definition).

I'd replace the last two lines with this:

    'tests/Makefile.am', because that file contains no $(SUBDIRS)
    definition.

OK?

> Subject: [PATCH 08/17] maint: remove anachronistic syntax-check
>
> * cfg.mk (sc_no_exec_perl_coreutils): This.  Our new testsuite
> layout (perl tests having '.pl' suffix, shell tests having '.sh'
> suffix) makes it basically impossible to run into the issues this
>
s/issues/issue/

> check guarded against.


> [PATCH 15/17] maint: fix syntax checks 'sc_root_tests'
>
> * cfg.mk: Don't work by trying to parse the (now gone) file
> 'tests/Makefile.am'; rather, use the contents of the new make
> variable $(all_root_tests).
>
Rather than:

     of the new make variable $(all_root_tests)

I think it would be better to use:

     of the make variable $(all_root_tests), introduced few commits ago

WDYT?



reply via email to

[Prev in Thread] Current Thread [Next in Thread]