[Top][All Lists]

[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.



> [PATCH 01/17] build: use 'check-local' to extend the 'check' target
> * tests/ (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/ (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 from vc_exe_in_TESTS
> * tests/ (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

> 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/ (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/
> skipped test: must be run as root
>    SKIP: misc/
>    PASS: misc/
>    ...
>    PASS: df/
> skipped test: very expensive: disabled by default
>    SKIP: du/
>    ...
> 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/' (because that makefile contains no $(SUBDIRS)
> definition).

I'd replace the last two lines with this:

    'tests/', because that file contains no $(SUBDIRS)


> Subject: [PATCH 08/17] maint: remove anachronistic syntax-check
> * (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

> check guarded against.

> [PATCH 15/17] maint: fix syntax checks 'sc_root_tests'
> * Don't work by trying to parse the (now gone) file
> 'tests/'; 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


reply via email to

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